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

[Victor Toso]

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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel