[Spice-devel] [spice-gtk 2/3] coroutine: Track idle notification/emission in struct coroutine

[Marc-André Lureau]

----- Original Message -----
> This allows to catch situations when we try to reinit the coroutine
> using coroutine_init() while a signal/notify idle is pending.
> 
> This can actually happen with cancelled migration and connect_delayed
> (which reinits the current coroutine).
> If channel_disconnect is called from coroutine context when state is
> SPICE_CHANNEL_STATE_SWITCHING:
>     - channel_connect() is called and queues an idle which will call
>       connect_delayed()
>     - then spice_session_set_migration_state() is called and will call
>       g_coroutine_object_notify(), which will queue an idle to emit the
>       notification, and yield to the main context
>     - connect_delayed() gets called in an idle, resets the current
>       coroutine, and then yield back to the coroutine context
>     - g_coroutine_object_notify() resumes and is confused as the
>       notification did not happen.
> 
> This can be triggered by adding a g_usleep(10000000); at the beginning
> of spice_session_start_migrating(), and by migrating an oVirt VM after
> connecting to it.

Can it still be triggered?

> 
> This is based on a patch suggestion from Marc-André.
> ---
>  gtk/coroutine.h          |  1 +
>  gtk/coroutine_ucontext.c |  5 +++++
>  gtk/gio-coroutine.c      | 16 ++++++++++++----
>  3 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/gtk/coroutine.h b/gtk/coroutine.h
> index 78dc467..5525fd3 100644
> --- a/gtk/coroutine.h
> +++ b/gtk/coroutine.h
> @@ -53,6 +53,7 @@ struct coroutine
>  	GThread *thread;
>  	gboolean runnable;
>  #endif
> +	guint idle_id;
>  };

With GCoroutine we won't be able to add fields to the structs.

In general, all coroutine specific data must be allocated by the coroutine itself, either on the stack or by it's own. You'll have to create your own interaction protocol to exchange (get/set) these kind of data.

>  void coroutine_init(struct coroutine *;
> diff --git a/gtk/coroutine_ucontext.c b/gtk/coroutine_ucontext.c
> index d709a33..6580fd7 100644
> --- a/gtk/coroutine_ucontext.c
> +++ b/gtk/coroutine_ucontext.c
> @@ -65,6 +65,11 @@ static void coroutine_trampoline(struct continuation *cc)
>  
>  void coroutine_init(struct coroutine *co)
>  {
> +	if (co->idle_id != 0) {
> +		g_warn_if_reached();
> +		g_source_remove(co->idle_id);
> +		co->idle_id = 0;
> +	}
>  	if (co->stack_size == 0)
>  		co->stack_size = 16 << 20;
>  
> diff --git a/gtk/gio-coroutine.c b/gtk/gio-coroutine.c
> index 1458d05..7527138 100644
> --- a/gtk/gio-coroutine.c
> +++ b/gtk/gio-coroutine.c
> @@ -197,6 +197,7 @@ static gboolean emit_main_context(gpointer opaque)
>  {
>      struct signal_data *signal = opaque;
>  
> +    g_warn_if_fail(signal->caller->idle_id != 0);
>      g_signal_emit_valist(signal->instance, signal->signal_id,
>                           signal->detail, signal->var_args);
>      signal->notified = TRUE;
> @@ -207,12 +208,14 @@ static gboolean emit_main_context(gpointer opaque)
>  }
>  
>  static void
> -run_in_idle(GSourceFunc idle_callback, gpointer user_data)
> +run_in_idle(GSourceFunc idle_callback, gpointer opaque)
>  {
> -    struct signal_data *data = (struct signal_data *)user_data;
> +    struct signal_data *data = opaque;
> +
> +    g_warn_if_fail(data->caller->idle_id == 0);
>  
>      g_object_ref(data->instance);
> -    g_idle_add(idle_callback, data);
> +    data->caller->idle_id = g_idle_add(idle_callback, data);
>      /* This switches to the system coroutine context, lets
>       * the idle function run to dispatch the signal, and
>       * finally returns once complete. ie this is synchronous
> @@ -220,7 +223,11 @@ run_in_idle(GSourceFunc idle_callback, gpointer
> user_data)
>       * an idle function involved
>       */
>      coroutine_yield(NULL);
> -    g_warn_if_fail(data->notified);
> +    if (!data->notified) {
> +        g_warn_if_reached();
> +        g_source_remove(data->caller->idle_id);
> +    }
> +    data->caller->idle_id = 0;
>      g_object_unref(data->instance);
>  }
>  
> @@ -251,6 +258,7 @@ static gboolean notify_main_context(gpointer opaque)
>  {
>      struct signal_data *signal = opaque;
>  
> +    g_warn_if_fail(signal->caller->idle_id != 0);
>      g_object_notify(signal->instance, signal->propname);
>      signal->notified = TRUE;
>  
> --
> 2.1.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>