[Spice-devel] [PATCH spice-gtk 2/2] Add SpiceSession::disconnected signal

[Frediano Ziglio]

> 
> Previously, an application was required to determine whether a session
> had been disconnected by listening for the SpiceSession::channel-destroy
> signal for each individual channel. The Application had to keep track of
> whether all channels had been destroyed.
> 
> An additional difficulty is that when a SpiceSession::channel-destroy
> signal is emitted, the channel object has not been freed yet, so any
> shutdown/de-allocation actions have not yet happened. This became
> significant since virt-viewer exits the application in response to the
> last 'channel-destroy' signal, which means that the channel objects are
> never properly freed. This is particularly problematic for the usbredir
> channel, which might need to disconnect USB devices asynchronously, and
> if the application exits before that process has completed, the USB
> devices may not be available on the client machine.
> 
> With this patch, the SpiceSession still emits the 'channel-destroy'
> signal for each channel when it is disconnected. But it also internally
> tracks which channels have been destroyed, and waits for them to be
> freed. When all channels are freed, it emits an additional
> 'disconnected' signal.  An application can be confident that all
> channels have been freed by this point, and the application can exit
> safely.
> ---
>  src/spice-session.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  src/spice-session.h |  2 ++
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/src/spice-session.c b/src/spice-session.c
> index 57acc63..de1128a 100644
> --- a/src/spice-session.c
> +++ b/src/spice-session.c
> @@ -94,6 +94,7 @@ struct _SpiceSessionPrivate {
>      int               protocol;
>      SpiceChannel      *cmain; /* weak reference */
>      Ring              channels;
> +    GList             *channels_destroying;
>      guint32           mm_time;
>      gboolean          client_provided_sockets;
>      guint64           mm_time_at_clock;
> @@ -210,6 +211,7 @@ enum {
>      SPICE_SESSION_CHANNEL_NEW,
>      SPICE_SESSION_CHANNEL_DESTROY,
>      SPICE_SESSION_MM_TIME_RESET,
> +    SPICE_SESSION_DISCONNECTED,
>      SPICE_SESSION_LAST_SIGNAL,
>  };
>  
> @@ -1313,6 +1315,23 @@ static void spice_session_class_init(SpiceSessionClass
> *klass)
>                       1,
>                       SPICE_TYPE_CHANNEL);
>  
> +    /**
> +     * SpiceSession::disconnected:
> +     * @session: the session that emitted the signal
> +     *
> +     * The #SpiceSession::disconnected signal is emitted when all channels
> have been destroyed.
> +     **/
> +    signals[SPICE_SESSION_DISCONNECTED] =
> +        g_signal_new("disconnected",

why not "session-disconnected" ?

> +                     G_OBJECT_CLASS_TYPE(gobject_class),
> +                     G_SIGNAL_RUN_FIRST,
> +                     G_STRUCT_OFFSET(SpiceSessionClass, disconnected),
> +                     NULL, NULL,
> +                     g_cclosure_marshal_VOID__VOID,
> +                     G_TYPE_NONE,
> +                     0,
> +                     NULL);
> +
>      /**
>       * SpiceSession::mm-time-reset:
>       * @session: the session that emitted the signal
> @@ -2269,6 +2288,17 @@ void spice_session_channel_new(SpiceSession *session,
> SpiceChannel *channel)
>      g_signal_emit(session, signals[SPICE_SESSION_CHANNEL_NEW], 0, channel);
>  }
>  
> +static void channel_finally_destroyed(gpointer data, GObject *channel)
> +{
> +    SpiceSession *session = SPICE_SESSION(data);
> +    SpiceSessionPrivate *s = session->priv;
> +    s->channels_destroying = g_list_remove(s->channels_destroying, channel);

So you insert before adding the weak reference and you just remove when
the object is freed checking when the list is empty. Why not using a
simple counter checking when is 0 ?

> +    if (!s->channels_destroying) {
> +        g_signal_emit(session, signals[SPICE_SESSION_DISCONNECTED], 0);
> +    }
> +    g_object_unref(session);
> +}
> +
>  static void spice_session_channel_destroy(SpiceSession *session,
>  SpiceChannel *channel)
>  {
>      g_return_if_fail(SPICE_IS_SESSION(session));
> @@ -2298,10 +2328,17 @@ static void
> spice_session_channel_destroy(SpiceSession *session, SpiceChannel *c
>      ring_remove(&item->link);
>      g_free(item);
>  
> -    g_signal_emit(session, signals[SPICE_SESSION_CHANNEL_DESTROY], 0,
> channel);
>  
>      g_clear_object(&channel->priv->session);
>      spice_channel_disconnect(channel, SPICE_CHANNEL_NONE);
> +
> +    g_signal_emit(session, signals[SPICE_SESSION_CHANNEL_DESTROY], 0,
> channel);
> +
> +    /* Wait until the channel is properly freed so that we can emit a
> +     * 'disconnected' signal */
> +    s->channels_destroying = g_list_prepend(s->channels_destroying,
> channel);
> +    g_object_weak_ref(G_OBJECT(channel), channel_finally_destroyed,
> g_object_ref(session));
> +
>      g_object_unref(channel);

isn't it possible that the channel reference reach 0 before we enlisted/counted
all channels so we will think that all channels were freed?

>  }
>  
> diff --git a/src/spice-session.h b/src/spice-session.h
> index cfe02b1..4a2f0ce 100644
> --- a/src/spice-session.h
> +++ b/src/spice-session.h
> @@ -84,6 +84,7 @@ struct _SpiceSession
>   * @parent_class: Parent class.
>   * @channel_new: Signal class handler for the #SpiceSession::channel_new
>   signal.
>   * @channel_destroy: Signal class handler for the
>   #SpiceSession::channel_destroy signal.
> + * @disconnected: Signal class handler for the #SpiceSession::disconnected
> signal.
>   *
>   * Class structure for #SpiceSession.
>   */
> @@ -94,6 +95,7 @@ struct _SpiceSessionClass
>      /* signals */
>      void (*channel_new)(SpiceSession *session, SpiceChannel *channel);
>      void (*channel_destroy)(SpiceSession *session, SpiceChannel *channel);
> +    void (*disconnected)(SpiceSession *session);
>  

Do we really think this ABI change?
If yes we need to bump the version.

>      /*< private >*/
>      /*

Frediano