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

[Jonathon Jongsma]

On Thu, 2018-05-31 at 16:41 -0400, Frediano Ziglio 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.
> > ---
> >  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" ?

Because the object is SpiceSession. So you would expect
SpiceSession::disconnected to mean that the session has been
disconnected. The only reason that the other signal is called
SpiceSession::channel-destroy is because it's not referring to the
session, but the channel instead.

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

I could, I suppose. It would certainly be simpler. Keeping them in a
list gives us a little bit of potential debugging help. For example, if
not all channels were freed, we could check which channel was in the
channels_destroying list and preventing us from emitting the signal.
Not sure that it would be useful very often though, so I don't feel
strongly either way.

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

Not quite sure I understand the question.

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

No. I was just thinking about that after sending this patch. I will
remove this ABI change.

> 
> >      /*< private >*/
> >      /*
> 
> Frediano