[PATCH wayland] client: ensure thread safety for wl_display_roundtrip_queue()

[Jonas Ådahl]

On Tue, Feb 16, 2016 at 10:46:11AM +0000, Ucan, Emre (ADITG/SW1) wrote:
> Hi Jonas,
> 
> What are the shortcomings of my approach ? Because we want to use this approach internally for thread safety.

It blocks all other threads from reading while you are inside
prepare_read() cancel_read(). The reason it works is you block other
threads from dispatching anything at all, meaning it won't dispatch the
default queue which probably is the queue of the display proxy.

The proper fix is to make the new-proxy setup within a mutex lock, so
that it is created and configured (queue set) without any other thread
dispatching before the queue is set. This needs new API, and the plan is
to add these API; either a proxy wrapper or new thread safe setup
functions and generated helpers.

In the mean time, I recommend implementing your own
wl_display_roundtrip_queue() the way you did in this patch, and after
wayland 1.11 (which will probably include a fix) rely on a properly
working wl_display_roundtrip_queue().


Jonas

> 
> If there are no shortcomings, is it not better to use this approach than introducing many new APIs ?
> 
> Best regards
> 
> Emre Ucan
> Software Group I (ADITG/SW1)
> 
> Tel. +49 5121 49 6937
> 
> > -----Original Message-----
> > From: wayland-devel [mailto:wayland-devel-
> > bounces at lists.freedesktop.org] On Behalf Of Jonas Ådahl
> > Sent: Dienstag, 16. Februar 2016 04:49
> > To: Ucan, Emre (ADITG/SW1)
> > Cc: wayland-devel at lists.freedesktop.org
> > Subject: Re: [PATCH wayland] client: ensure thread safety for
> > wl_display_roundtrip_queue()
> > 
> > On Mon, Feb 15, 2016 at 02:19:43PM +0000, Ucan, Emre (ADITG/SW1) wrote:
> > > We have to block other threads from reading events. Otherwise other
> > > threads may assign done event to the default queue, if they call
> > > wl_display_read_events before setting the queue for the callback proxy.
> > > This would cause a deadlock in wl_display_dispatch_queue, because the
> > > default queue is not dispatched.
> > >
> > > We can call wl_display_prepare_read_queue API which blocks on success
> > > other threads from reading events and call wl_display_cancel read
> > > after the queue is set to the proxy. This new implementation ensures
> > > thread safety for the API.
> > 
> > This is the work-around available to avoid the issue, but it is not the correct
> > solution for libwayland-client. Right now we have two potential
> > solutions: one is to add a "proxy wrapper" where one configures the proxy
> > wrapper object (setting the queue) before using it to issue requests, the
> > other is to add thread safe functions and patch the scanner to add thread
> > safe helpers which would be used instead of for example
> > "wl_display_sync()".
> > 
> > The proxy wrapper approach can be tested out by appling this patch:
> > https://lists.freedesktop.org/archives/wayland-devel/2015-
> > June/023054.html
> > 
> > For more information, check the corresponding bug in bugzilla:
> > https://bugs.freedesktop.org/show_bug.cgi?id=91273
> > 
> > 
> > Jonas
> > 
> > >
> > > Signed-off-by: Emre Ucan <eucan at de.adit-jv.com>
> > > ---
> > >  src/wayland-client.c |   16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/src/wayland-client.c b/src/wayland-client.c index
> > > ef12bfc..00d8cda 100644
> > > --- a/src/wayland-client.c
> > > +++ b/src/wayland-client.c
> > > @@ -1075,12 +1075,26 @@ wl_display_roundtrip_queue(struct wl_display
> > *display, struct wl_event_queue *qu
> > >  	struct wl_callback *callback;
> > >  	int done, ret = 0;
> > >
> > > +	/*We have to block other threads from reading events. Otherwise
> > other threads
> > > +	 * may assign done event to the default queue, if they call
> > wl_display_read_events
> > > +	 * before setting the queue for the callback proxy. This would cause a
> > deadlock
> > > +	 * in wl_display_dispatch_queue, because the default queue is not
> > dispatched.*/
> > > +	while (ret >= 0 && wl_display_prepare_read_queue(display,
> > queue))
> > > +		ret = wl_display_dispatch_queue_pending(display, queue);
> > > +
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > >  	done = 0;
> > >  	callback = wl_display_sync(display);
> > > -	if (callback == NULL)
> > > +	if (callback == NULL) {
> > > +		wl_display_cancel_read(display);
> > >  		return -1;
> > > +	}
> > > +
> > >  	wl_proxy_set_queue((struct wl_proxy *) callback, queue);
> > >  	wl_callback_add_listener(callback, &sync_listener, &done);
> > > +	wl_display_cancel_read(display);
> > >  	while (!done && ret >= 0)
> > >  		ret = wl_display_dispatch_queue(display, queue);
> > >
> > > --
> > > 1.7.9.5
> > >
> > > _______________________________________________
> > > wayland-devel mailing list
> > > wayland-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/wayland-devel