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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel