[Spice-devel] [PATCH spice-server 5/5] red-client: Prevent some nasty threading problems disconnecting channels

[Christophe Fergeau]

On Wed, Aug 30, 2017 at 04:19:47AM -0400, Frediano Ziglio wrote:
> > > > >          // some channels may be in other threads. However we currently
> > > > >          // assume disconnect is synchronous (we changed the dispatcher
> > > > >          // to wait for disconnection)
> > > > >          // TODO: should we go back to async. For this we need to use
> > > > >          // ref count for channel clients.
> > > > >          red_channel_disconnect_client(channel, rcc);
> > > > > +
> > > > >          spice_assert(red_channel_client_pipe_is_empty(rcc));
> > > > >          spice_assert(red_channel_client_no_item_being_sent(rcc));
> > > > > +
> > > > >          red_channel_client_destroy(rcc);
> > > 
> > > Actually I think that now maybe this call is not necessary...
> > > but better safe than sorry.
> > 
> > Yes, it seems very redundant as this call is doing disconnect +
> > set_destroying. This could be a nice preparatory commit, makes things
> > simpler :)
> > 
> 
> Do not understand the suggestion here...

Ah, I was agreeing with you, red_channel_client_destroy() is not
necessary here, so I would drop it in a commit before or after this
patch we are discussing.

> 
> > > 
> > > Basically the real disconnection can happen either in
> > > red_channel_disconnect_client (which potentially run in a
> > > separate thread) or in red_channel_client_destroy, they
> > > both call red_channel_client_disconnect.
> > > 
> > > > > +        g_object_unref(rcc);
> > > > > +        pthread_mutex_lock(&client->lock);
> > > > >      }
> > > > > +    pthread_mutex_unlock(&client->lock);
> > > > >      g_object_unref(client);
> > > > >  }
> > > > >  
> > > > > @@ -254,6 +275,16 @@ gboolean red_client_add_channel(RedClient *client,
> > > > > RedChannelClient *rcc, GError
> > > > >      pthread_mutex_lock(&client->lock);
> > > > >  
> > > > >      g_object_get(channel, "channel-type", &type, "id", &id, NULL);
> > > > > +    if (client->disconnecting) {
> > > > > +        g_set_error(error,
> > > > > +                    SPICE_SERVER_ERROR,
> > > > > +                    SPICE_SERVER_ERROR_FAILED,
> > > > > +                    "Client %p got disconnected while connecting
> > > > > channel
> > > > > type %d id %d",
> > > > > +                    client, type, id);
> > > > > +        result = FALSE;
> > > > > +        goto cleanup;
> > > > > +    }
> > 
> > The client->disconnecting stuff in this commit could also go in a
> > separate commit. It seems like the goal is to prevent new
> > RedChannelClient creation when the RedClient is being destroyed?
> > 
> 
> Yes, the question suggest that the message 
> "Client %p got disconnected while connecting channel type %d id %d"
> is not clear. Suggestion on how to rewrite it?

Hmm I'd say the message is fine, I did not re-read through the commit
while writing that answer, so I just forgot about the message, and just
wanted to make sure I understood your goal :)

> 
> This happens as the connection is asynchronous so (MT main thread,
> CT channel thread):
> - MT you get a new connection;
> - MT a connection is sent to CT
> - MT you get a disconnection of main channel
> - MT red_client_destroy is called
> - CT you attempt to add the RCC to RedClient

Yes, I got this one, and while reviewing the changes related to
accessing the 'channels' list, I was wondering what would happen if we
kept trying to create new RCC while iterating over the list, so it was
good to see this 'client->disconnecting' change ;)

This short description you just gave belongs in one of the commit logs
in my opinion.

Christophe