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

Submitted by Victor Toso on May 2, 2016, 1:16 p.m.

Details

Message ID 1462194965-27641-1-git-send-email-victortoso@redhat.com
State New
Headers show
Series "spicevmc: don't disconnect on "cancel"" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Victor Toso May 2, 2016, 1:16 p.m.
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(-)

Patch hide | download patch | download mbox

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 */
     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);
 }
 

Comments

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@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);
>  }
>
Hi,

On Mon, May 02, 2016 at 11:56:54AM -0500, Jonathon Jongsma wrote:
> 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?

Sure!

>
> Otherwise I think this patch looks fine.
> 
> Acked-by: Jonathon Jongsma <jjongsma@redhat.com>

Thanks, pushed!

> 
> 
> >      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);
> >  }
> >  
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel