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

[Frediano Ziglio]

> 
> On Tue, Jun 5, 2018 at 10:40 PM, Jonathon Jongsma <jjongsma at redhat.com>
> wrote:
> > 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.
> > ---
> > Changes in v2:
> >  - remove ABI change ('disconnected' vfunc in SpiceSession struct
> >  - use an integer to track disconnecting channels instead of a list of
> >    objects
> 
> "why not move the ring_remove() of
> channel here and check it is empty?"
> 

I think would be potentially a regression.
The ring is used to scan the channels and potentially perform some
operations on them which could be potentially unsafe is the channel
is being destroyed.

> >  - Add a warning to spice_session_dispose() if there are channels
> >    disconnecting when the session is destroyed.
> >
> >  src/spice-session.c | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/src/spice-session.c b/src/spice-session.c
> > index 57acc63..1731904 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;
> > +    guint             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,
> >  };
> >
> > @@ -343,6 +345,7 @@ spice_session_dispose(GObject *gobject)
> >      g_warn_if_fail(s->migration_left == NULL);
> >      g_warn_if_fail(s->after_main_init == 0);
> >      g_warn_if_fail(s->disconnecting == 0);
> > +    g_warn_if_fail(s->channels_destroying == 0);
> >
> >      g_clear_object(&s->audio_manager);
> >      g_clear_object(&s->usb_manager);
> > @@ -1313,6 +1316,22 @@ 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.
> > +     **/
> 
> missing since tag
> 
> > +    signals[SPICE_SESSION_DISCONNECTED] =
> > +        g_signal_new("disconnected",
> > +                     G_OBJECT_CLASS_TYPE(gobject_class),
> > +                     G_SIGNAL_RUN_FIRST,
> > +                     0, 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--;
> > +    if (s->channels_destroying == 0) {
> > +        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));
> > @@ -2302,6 +2332,12 @@ static void
> > spice_session_channel_destroy(SpiceSession *session, SpiceChannel *c
> >
> >      g_clear_object(&channel->priv->session);
> >      spice_channel_disconnect(channel, SPICE_CHANNEL_NONE);
> > +
> > +    /* Wait until the channel is properly freed so that we can emit a
> > +     * 'disconnected' signal */
> > +    s->channels_destroying++;
> > +    g_object_weak_ref(G_OBJECT(channel), channel_finally_destroyed,
> > g_object_ref(session));
> > +
> >      g_object_unref(channel);
> >  }
> >

Frediano