[Spice-devel] [spice-gtk v1] spicevmc: don't disconnect on "cancel"

[Jonathon Jongsma]

On Mon, 2016-05-02 at 15:16 +0200, Victor Toso wrote:
> spicevmc was designed so its _finish functions should always be called;
> With the gtask integration both _finish functions are trying to
> disconnect the GCancellabe even in the 'cancel' context which leads to
> a deadlock as explained in the documentation.
> 
> Resolves: https://bugs.freedesktop.org/show_bug.cgi?id=95032
> ---
>  src/vmcstream.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/src/vmcstream.c b/src/vmcstream.c
> index 5dd2799..a8bedf5 100644
> --- a/src/vmcstream.c
> +++ b/src/vmcstream.c
> @@ -183,13 +183,10 @@ read_cancelled(GCancellable *cancellable,
>                              G_IO_ERROR, G_IO_ERROR_CANCELLED,
>                              "read cancelled");
>  
> +    /* With GTask, we don't need to deal with GCancellable when task is
> +     * cancelled within cancellable callback as it could lead to deadlocks
> +     * e.g: https://bugzilla.gnome.org/show_bug.cgi?id=705395 */

the phrase "deal with" is a bit too vague. I'd say something like "we don't need
to disconnect..." instead perhaps?

Otherwise I think this patch looks fine.

Acked-by: Jonathon Jongsma <jjongsma at redhat.com>


>      g_clear_object(&self->task);
> -
> -    /* See FIXME */
> -    /* if (self->cancellable) { */
> -    /*     g_cancellable_disconnect(self->cancellable, self->cancel_id); */
> -    /*     g_clear_object(&self->cancellable); */
> -    /* } */
>  }
>  
>  G_GNUC_INTERNAL void
> @@ -230,13 +227,14 @@ spice_vmc_input_stream_read_all_finish(GInputStream
> *stream,
>  {
>      GTask *task = G_TASK(result);
>      SpiceVmcInputStream *self = SPICE_VMC_INPUT_STREAM(stream);
> +    GCancellable *cancel;
>  
>      g_return_val_if_fail(g_task_is_valid(task, self), -1);
> -
> -    /* FIXME: calling _finish() is required. Disconnecting in
> -       read_cancelled() causes a deadlock. #705395 */
> -    g_cancellable_disconnect(g_task_get_cancellable(task), self->cancel_id);
> -
> +    cancel = g_task_get_cancellable(task);
> +    if (!g_cancellable_is_cancelled(cancel)) {
> +         g_cancellable_disconnect(cancel, self->cancel_id);
> +         self->cancel_id = 0;
> +    }
>      return g_task_propagate_int(task, error);
>  }
>  
> @@ -276,13 +274,15 @@ spice_vmc_input_stream_read_finish(GInputStream *stream,
>  {
>      GTask *task = G_TASK(result);
>      SpiceVmcInputStream *self = SPICE_VMC_INPUT_STREAM(stream);
> +    GCancellable *cancel;
>  
>      g_return_val_if_fail(g_task_is_valid(task, self), -1);
>  
> -    /* FIXME: calling _finish() is required. Disconnecting in
> -       read_cancelled() causes a deadlock. #705395 */
> -    g_cancellable_disconnect(g_task_get_cancellable(task), self->cancel_id);
> -
> +    cancel = g_task_get_cancellable(task);
> +    if (!g_cancellable_is_cancelled(cancel)) {
> +         g_cancellable_disconnect(cancel, self->cancel_id);
> +         self->cancel_id = 0;
> +    }
>      return g_task_propagate_int(task, error);
>  }
>