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

[Ucan, Emre (ADITG/SW1)]

Hi Jonas,

What are the shortcomings of my approach ? Because we want to use this approach internally for thread safety.

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