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

[Marc-André Lureau]

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.

I fail to see why GTask would be culprit here. The coroutine code here wants to resume the stream in the "main" coroutine context. There was an idle for that, now that task is being used this is no longer explicit and is left to task heuristic, which makes sense in a non-coroutine case. So it's fine for coroutine code to deal with that explicitly again and for gtask to not bother.

> 
> [0]: https://bugs.freedesktop.org/show_bug.cgi?id=94662
> 
> Signed-off-by: Fabiano Fidêncio <fidencio at redhat.com>
> ---
> Changes since v1, as per Marc-André review:
> - Use g_idle_add() instead of GSource ... which worries me a bit about the
>   context used when using g_idle_add(), as previously the context used was
>   the one from the GTask.
> ---
>  src/vmcstream.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/src/vmcstream.c b/src/vmcstream.c
> index 5ebf15a..54bf4d9 100644
> --- a/src/vmcstream.c
> +++ b/src/vmcstream.c
> @@ -97,6 +97,32 @@ 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);

could be moved to idle_cb imho

> +}
> +
> +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);
> +
> +    complete_in_idle_cb_data_free(data);
> +
> +    return FALSE;
> +}
> +
>  /* coroutine */
>  /**
>   * Feed a SpiceVmc stream with new data from a coroutine
> @@ -116,6 +142,8 @@ spice_vmc_input_stream_co_data(SpiceVmcInputStream *self,
>      self->coroutine = coroutine_self();
>  
>      while (size > 0) {
> +        complete_in_idle_cb_data *cb_data;
> +
>          SPICE_DEBUG("spicevmc co_data %p", self->task);
>          if (!self->task)
>              coroutine_yield(NULL);
> @@ -137,10 +165,19 @@ 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
> +         */

This comment is misleading, imho the reasons are pretty clears.

> +        cb_data = g_new(complete_in_idle_cb_data , 1);
> +        cb_data->task = g_object_ref(self->task);
> +        cb_data->pos = self->pos;
> +        g_idle_add(complete_in_idle_cb, cb_data);
>  

vmcstream prevents from concurrent reads explicitely, so in fact this could probably be simplified, but as such it looks okay to me.

>          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
>