[Spice-devel] [spice-gtk] vmcstream, gtask: Do idle ourself as GTask's return gets messed up

[Fabiano Fidêncio]

On Tue, Mar 22, 2016 at 8:40 PM, Marc-André Lureau <mlureau at redhat.com> wrote:
> Hi
>
> ----- Original Message -----
>> Seems that our coroutines can mess up with GTask's return, ending up
>> in an early return from g_task_return_now() instead of doing a return
>> in idle.
>> As I still don't know if the bug is on spice-gtk or on GTask itself,
>> I've opened a bug[0] in spice-gtk's bugzilla and also made a reference
>> to the very same bug in the code.
>>
>> [0]: https://bugs.freedesktop.org/show_bug.cgi?id=94662
>>
>> Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
>
> Is it easy to reproduce the issue? Just use webdav channel? Is there a crash & backtrace?

There is no crash with the patch that I just pushed.
webdav channel is where it's really clear. Bur I'm pretty sure that
the fix done on 7774b8c0dd85ce2bb311d8bbe1c25deb73970b6e is for
exactly the same issue.

>
>> ---
>>  src/vmcstream.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 44 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/vmcstream.c b/src/vmcstream.c
>> index 5ebf15a..4db94c7 100644
>> --- a/src/vmcstream.c
>> +++ b/src/vmcstream.c
>> @@ -97,6 +97,30 @@ spice_vmc_input_stream_new(void)
>>      return self;
>>  }
>>
>> +typedef struct _complete_in_idle_cb_data {
>> +    GTask *task;
>> +    gssize pos;
>> +} complete_in_idle_cb_data;
>> +
>> +static void
>> +complete_in_idle_cb_data_free (gpointer user_data)
>> +{
>> +    complete_in_idle_cb_data *data = user_data;
>> +
>> +    g_object_unref (data->task);
>> +    g_free (data);
>> +}
>> +
>> +static gboolean
>> +complete_in_idle_cb(gpointer user_data)
>> +{
>> +    complete_in_idle_cb_data *data = user_data;
>> +
>> +    g_task_return_int(data->task, data->pos);
>> +
>> +    return FALSE;
>> +}
>> +
>>  /* coroutine */
>>  /**
>>   * Feed a SpiceVmc stream with new data from a coroutine
>> @@ -116,6 +140,9 @@ spice_vmc_input_stream_co_data(SpiceVmcInputStream *self,
>>      self->coroutine = coroutine_self();
>>
>>      while (size > 0) {
>> +        complete_in_idle_cb_data *cb_data;
>> +        GSource *source;
>> +
>>          SPICE_DEBUG("spicevmc co_data %p", self->task);
>>          if (!self->task)
>>              coroutine_yield(NULL);
>> @@ -137,10 +164,25 @@ spice_vmc_input_stream_co_data(SpiceVmcInputStream
>> *self,
>>          if (self->all && min > 0 && self->pos != self->count)
>>              continue;
>>
>> -        g_task_return_int(self->task, self->pos);
>> +        /* Somehow we are messing up with GTask return. As it's not known
>> +         * for now if it's a spice-gtk issue due to the coroutine usage or
>> +         * a GTask issue itself, let's do the idle ourself.
>> +         *
>> +         * For a future reference, see:
>> +         * https://bugs.freedesktop.org/show_bug.cgi?id=94662
>> +         */
>> +        cb_data = g_new(complete_in_idle_cb_data , 1);
>> +        cb_data->task = g_object_ref(self->task);
>> +        cb_data->pos = self->pos;
>> +
>> +        source = g_idle_source_new ();
>> +        g_source_set_priority (source, G_PRIORITY_DEFAULT);
>> +        g_source_set_callback (source, complete_in_idle_cb, cb_data,
>> complete_in_idle_cb_data_free);
>> +        g_source_attach (source, g_task_get_context(self->task));
>> +        g_source_set_name (source, "[spice-gtk] complete_in_idle_cb");
>> +        g_source_unref (source);
>
> Why not use just g_idle_add()?

Hmm. Okay, works for me. I'll submit a v2.

>
>>
>>          g_cancellable_disconnect(g_task_get_cancellable(self->task),
>>          self->cancel_id);
>> -
>>          g_clear_object(&self->task);
>>      }
>>
>> --
>> 2.5.0
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel



-- 
Fabiano Fidêncio