On Tue, Aug 02, 2016 at 11:48:49AM +0200, Victor Toso wrote:
> This patch avoids a race condition. The race happens when we mark the
> xfer-task as completed by receiving VD_AGENT_FILE_XFER_STATUS_SUCCESS
> message while the flush callback was not yet called.
>
> The flush callback is file_xfer_data_flushed_cb() and it might not be
> called immediately after data is flushed.
>
> The race can be verified while transferring several small files at
> once. I can see it often with more then 50 files in one transfer
> operation.
I've also been able to reliably hit this running in valgrind.
>
> This fix implies that SpiceMainChannel should check in its async
> callbacks if given SpiceFileTransferTask is completed.
>
> This patch introduces spice_file_transfer_task_is_completed (internal)
> to help check if spice_file_transfer_task_completed() was called or
> not.
I wondered if we could cancel the pending async callbacks rather than
doing things this way, but it's probably more complicated to do that.
> ---
> src/channel-main.c | 17 +++++++++++------
> src/spice-file-transfer-task-priv.h | 1 +
> src/spice-file-transfer-task.c | 14 ++++++++++++++
> 3 files changed, 26 insertions(+), 6 deletions(-)
>
> diff --git a/src/channel-main.c b/src/channel-main.c
> index bc7349f..14ad6cf 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -1766,10 +1766,12 @@ static void file_xfer_data_flushed_cb(GObject *source_object,
> return;
> }
>
> - file_transfer_operation_send_progress(xfer_task);
> -
> - /* Read more data */
> - spice_file_transfer_task_read_async(xfer_task, file_xfer_read_async_cb, NULL);
> + /* task might be completed while on idle */
> + if (!spice_file_transfer_task_is_completed(xfer_task)) {
> + file_transfer_operation_send_progress(xfer_task);
> + /* Read more data */
> + spice_file_transfer_task_read_async(xfer_task, file_xfer_read_async_cb, NULL);
> + }
> }
>
> static void file_xfer_queue_msg_to_agent(SpiceMainChannel *channel,
> @@ -1814,13 +1816,15 @@ static void file_xfer_read_async_cb(GObject *source_object,
> }
>
> file_xfer_queue_msg_to_agent(channel, spice_file_transfer_task_get_id(xfer_task), buffer, count);
> - if (count == 0) {
> - /* on EOF just wait for VD_AGENT_FILE_XFER_STATUS from agent */
> + if (count == 0 || spice_file_transfer_task_is_completed(xfer_task)) {
> + /* on EOF just wait for VD_AGENT_FILE_XFER_STATUS from agent
> + * in case the task was completed, nothing to do. */
> return;
> }
>
> task_id = spice_file_transfer_task_get_id(xfer_task);
> xfer_op = g_hash_table_lookup(channel->priv->file_xfer_tasks, GUINT_TO_POINTER(task_id));
> + g_return_if_fail(xfer_op != NULL);
> xfer_op->stats.total_sent += count;
>
> cancellable = spice_file_transfer_task_get_cancellable(xfer_task);
> @@ -1841,6 +1845,7 @@ static void main_agent_handle_xfer_status(SpiceMainChannel *channel,
>
> switch (msg->result) {
> case VD_AGENT_FILE_XFER_STATUS_CAN_SEND_DATA:
> + g_return_if_fail(spice_file_transfer_task_is_completed(xfer_task) == FALSE);
> spice_file_transfer_task_read_async(xfer_task, file_xfer_read_async_cb, NULL);
> return;
> case VD_AGENT_FILE_XFER_STATUS_CANCELLED:
> diff --git a/src/spice-file-transfer-task-priv.h b/src/spice-file-transfer-task-priv.h
> index 1ceb392..a7b58d8 100644
> --- a/src/spice-file-transfer-task-priv.h
> +++ b/src/spice-file-transfer-task-priv.h
> @@ -52,6 +52,7 @@ gssize spice_file_transfer_task_read_finish(SpiceFileTransferTask *self,
> GError **error);
> guint64 spice_file_transfer_task_get_file_size(SpiceFileTransferTask *self);
> guint64 spice_file_transfer_task_get_bytes_read(SpiceFileTransferTask *self);
> +gboolean spice_file_transfer_task_is_completed(SpiceFileTransferTask *self);
>
> G_END_DECLS
>
> diff --git a/src/spice-file-transfer-task.c b/src/spice-file-transfer-task.c
> index eb943c4..4e51b7e 100644
> --- a/src/spice-file-transfer-task.c
> +++ b/src/spice-file-transfer-task.c
> @@ -42,6 +42,7 @@ struct _SpiceFileTransferTask
> GObject parent;
>
> uint32_t id;
> + gboolean completed;
> gboolean pending;
> GFile *file;
> SpiceMainChannel *channel;
> @@ -270,6 +271,8 @@ G_GNUC_INTERNAL
> void spice_file_transfer_task_completed(SpiceFileTransferTask *self,
> GError *error)
> {
> + self->completed = TRUE;
> +
Maybe add a g_warn_if_fail(self->completed == FALSE); or
g_return_if_fail() at the beginning of thee function? Or is it meant to
be called multiple times?
> /* In case of multiple errors we only report the first error */
> if (self->error)
> g_clear_error(&error);
> @@ -478,6 +481,16 @@ gssize spice_file_transfer_task_read_finish(SpiceFileTransferTask *self,
> return nbytes;
> }
>
> +G_GNUC_INTERNAL
> +gboolean spice_file_transfer_task_is_completed(SpiceFileTransferTask *self)
> +{
> + g_return_val_if_fail(self != NULL, FALSE);
> +
> + /* File transfer is considered over at the first time it is marked as
> + * complete with spice_file_transfer_task_completed. */
> + return self->completed;
Just to be sure, the race we are talking about is not caused by
multithtreading, but only by the order in which things happen with
respect to network/main loop? (checking whether we need mutexes here or
not, I guess not).
> +}
> +
> /*******************************************************************************
> * External API
> ******************************************************************************/
> @@ -735,4 +748,5 @@ static void
> spice_file_transfer_task_init(SpiceFileTransferTask *self)
> {
> self->buffer = g_malloc0(FILE_XFER_CHUNK_SIZE);
> + self->completed = FALSE;
> }
The instance will be cleared to 0 upon creation, I don't think you need
the explicit FALSE assignment there.
Acked-by: Christophe Fergeau <cfergeau@xxxxxxxxxx>
Christophe
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/spice-devel