<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Jul 1, 2015 at 9:16 AM, Jonas Ådahl <<a href="mailto:jadahl@gmail.com" target="_blank">jadahl@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Jul 01, 2015 at 08:57:05AM +0200, Marek Chalupa wrote: > On Mon, Jun 29, 2015 at 3:37 PM, Giulio Camuffo <<a href="mailto:giuliocamuffo@gmail.com">giuliocamuffo@gmail.com</a>> > wrote: > > > 2015-06-29 14:30 GMT+03:00 Jonas Ådahl <<a href="mailto:jadahl@gmail.com">jadahl@gmail.com</a>>: > > > Arnaud Vrac discovered an issue in the libwayland client API causing > > > race conditions when doing round trips using the wl_display object using > > > non-default proxy queues. > > > > > > The problem is that one thread can read and dispatch events after > > > another thread creates the wl_callback object but before it sets the > > > queue. This could potentially cause the events to be dropped completely > > > if the event is dispatched before the creating thread sets the > > > implementation, or that the event is dispatched on the wrong queue which > > > might run on another thread. > > > > > > This patch introduces API to solve this case by introducing "proxy > > > wrappers". In short, a proxy wrapper is a struct wl_proxy * that will > > > never itself emit any events, but the client can set a queue, and use it > > > when sending requests that creates new proxy objects. When sending > > > requests, the wrapper will work in the same way as the normal proxy > > > object, but the proxy created by sending a request (for example > > > wl_display.sync) will default to the same proxy queue as the wrapper. > > > > > > Signed-off-by: Jonas Ådahl <<a href="mailto:jadahl@gmail.com">jadahl@gmail.com</a>> > > > --- > > > > > > Hi, > > > > > > This is one of the two alternative solutions that was discussed on > > #wayland. > > > > > > The other one being introducing API alternatives to > > > wl_proxy_marshal_constructor and wl_proxy_marshal_array_constructor that > > > also take a queue and also changing scanner.c to produce functions that > > > take a queue for every request that creates a proxy. > > > > > > In short, instead of > > > > > > bar = wl_foo_get_bar(foo); > > > wl_proxy_set_queue((struct wl_proxy *) bar, queue); > > > wl_bar_add_listener(bar, ...); > > > > > > with this RFC a client does > > > > > > foo_wrapper = wl_proxy_create_wrapper((struct wl_proxy *) foo); > > > wl_proxy_set_queue((struct wl_proxy *) foo_wrapper, queue); > > > > > > bar = wl_foo_get(foo_wrapper); > > > wl_bar_add_listener(bar, ...); > > > > > > and the with other idea that is implemented anywhere yet AFAIK > > > > > > bar = wl_foo_get_bar_with_queue(foo, queue) > > > wl_bar_add_listener(bar, ...); > > > > Hi, i think this second option is better. The proxy approach looks to > > me like a clunky and unnecessary workaround, while i don't see > > downsides to new api taking a queue. > > </div></div>I agree that the second approach seems less like a work around, both implementation wise and API wise. However I see one benefit with the first approach that the second lacks and that is that one will be able to have code that is unaware of queues and dispatching, only doing requests and listening on new objects on whatever the default queue happens to be, while the "parent" code is the one running the main loop being aware of what thread it is running, dispatching on the right queue etc. Not sure it's a use case worth the clunkyness how the API would look though. > > > With this second approach there's still a race before setting listener. > We should be able to set queue and listener atomically to get rid of this > race. > > So something like: > > wl_proxy_create_full(..., queue, listener) > > + the scanner thing? Since the listener implementation won't be invoked until the events are dispatched, and if one sets a separate queue, they won't be dispatched until the user dispatches the queue it set. So in that case, there is no race as far as I can tell. </blockquote><div> </div><div>Yes, it works if the queue gets dispatched after setting the listener. But what if, for example, the client has one thread that is dispatching the queue and another that would create proxy with this queue? In this case the dispatching could </div><div>occur before setting the listener, right? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Jonas <div class="HOEnZb"><div class="h5"> > > > > > > -- > > Giulio > > > > > > > > Opinions? > > > > > > > > > Jonas > > > > > > > > > > > > src/wayland-client-core.h | 1 + > > > src/wayland-client.c | 106 > > +++++++++++++++++++++++++++++++++++++++++----- > > > src/wayland-private.h | 7 +++ > > > 3 files changed, 103 insertions(+), 11 deletions(-) > > > > > > diff --git a/src/wayland-client-core.h b/src/wayland-client-core.h > > > index dea70d9..0db8b9c 100644 > > > --- a/src/wayland-client-core.h > > > +++ b/src/wayland-client-core.h > > > @@ -125,6 +125,7 @@ void wl_proxy_marshal_array(struct wl_proxy *p, > > uint32_t opcode, > > > union wl_argument *args); > > > struct wl_proxy *wl_proxy_create(struct wl_proxy *factory, > > > const struct wl_interface *interface); > > > +struct wl_proxy *wl_proxy_create_wrapper(struct wl_proxy *proxy); > > > struct wl_proxy *wl_proxy_marshal_constructor(struct wl_proxy *proxy, > > > uint32_t opcode, > > > const struct wl_interface > > *interface, > > > diff --git a/src/wayland-client.c b/src/wayland-client.c > > > index 6450b67..f8c64c4 100644 > > > --- a/src/wayland-client.c > > > +++ b/src/wayland-client.c > > > @@ -51,7 +51,8 @@ > > > > > > enum wl_proxy_flag { > > > WL_PROXY_FLAG_ID_DELETED = (1 << 0), > > > - WL_PROXY_FLAG_DESTROYED = (1 << 1) > > > + WL_PROXY_FLAG_DESTROYED = (1 << 1), > > > + WL_PROXY_FLAG_WRAPPER = (1 << 2), > > > }; > > > > > > struct wl_proxy { > > > @@ -405,17 +406,19 @@ wl_proxy_create_for_id(struct wl_proxy *factory, > > > void > > > proxy_destroy(struct wl_proxy *proxy) > > > { > > > - if (proxy->flags & WL_PROXY_FLAG_ID_DELETED) > > > - wl_map_remove(&proxy->display->objects, proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a> > > ); > > > - else if (proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a> < WL_SERVER_ID_START) > > > - wl_map_insert_at(&proxy->display->objects, 0, > > > - proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a>, WL_ZOMBIE_OBJECT); > > > - else > > > - wl_map_insert_at(&proxy->display->objects, 0, > > > - proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a>, NULL); > > > - > > > + if (proxy->flags != WL_PROXY_FLAG_WRAPPER) { > > > + if (proxy->flags & WL_PROXY_FLAG_ID_DELETED) > > > + wl_map_remove(&proxy->display->objects, > > > + proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a>); > > > + else if (proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a> < WL_SERVER_ID_START) > > > + wl_map_insert_at(&proxy->display->objects, 0, > > > + proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a>, > > WL_ZOMBIE_OBJECT); > > > + else > > > + wl_map_insert_at(&proxy->display->objects, 0, > > > + proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a>, NULL); > > > > > > - proxy->flags |= WL_PROXY_FLAG_DESTROYED; > > > + proxy->flags |= WL_PROXY_FLAG_DESTROYED; > > > + } > > > > > > proxy->refcount--; > > > if (!proxy->refcount) > > > @@ -459,6 +462,11 @@ WL_EXPORT int > > > wl_proxy_add_listener(struct wl_proxy *proxy, > > > void (**implementation)(void), void *data) > > > { > > > + if (proxy->flags == WL_PROXY_FLAG_WRAPPER) { > > > + wl_log("proxy %p is a wrapper\n", proxy); > > > + return -1; > > > + } > > > + > > > if (proxy->object.implementation || proxy->dispatcher) { > > > wl_log("proxy %p already has listener\n", proxy); > > > return -1; > > > @@ -512,6 +520,11 @@ wl_proxy_add_dispatcher(struct wl_proxy *proxy, > > > wl_dispatcher_func_t dispatcher, > > > const void *implementation, void *data) > > > { > > > + if (proxy->flags == WL_PROXY_FLAG_WRAPPER) { > > > + wl_log("proxy %p is a wrapper\n", proxy); > > > + return -1; > > > + } > > > + > > > if (proxy->object.implementation || proxy->dispatcher) { > > > wl_log("proxy %p already has listener\n"); > > > return -1; > > > @@ -1885,6 +1898,77 @@ wl_proxy_set_queue(struct wl_proxy *proxy, struct > > wl_event_queue *queue) > > > proxy->queue = &proxy->display->default_queue; > > > } > > > > > > +/** Create a proxy wrapper > > > + * > > > + * \param proxy The proxy object to be wrapped > > > + * \return A proxy wrapper for the given proxy > > > + * > > > + * A proxy wrapper is type of 'struct wl_proxy' instance that can be > > used for > > > + * sending requests instead of using the original proxy. A proxy > > wrapper does > > > + * not have an implementation or dispatcher, and events received on the > > > + * object is still emitted on the original proxy. Trying to set an > > > + * implementation or dispatcher will have no effect but result in a > > warning > > > + * being logged. > > > + * > > > + * Even though there is no implementation nor dispatcher, the proxy > > queue can > > > + * be changed. This will affect the default queue of new objects > > created by > > > + * requests sent via the proxy wrapper. > > > + * > > > + * A proxy wrapper must be destroyed before the proxy it was created > > from. > > > + * Creating a proxy wrapper on a proxy that has either been destroyed or > > > + * removed will fail, and NULL will be returned. > > > + * > > > + * If a user reads and dispatches events on more than one thread, it is > > > + * necessary to use a proxy wrapper when sending requests on objects > > when the > > > + * intention is that a newly created proxy is to use a proxy queue > > different > > > + * from the proxy the request was sent on, as creating the new proxy > > and then > > > + * setting the queue is not thread safe. > > > + * > > > + * For example, a module that runs using its own proxy queue that needs > > to > > > + * do display roundtrip must wrap the wl_display proxy object before > > sending > > > + * the wl_display.sync request. For example: > > > + * > > > + * struct wl_proxy *wrapped_display; > > > + * struct wl_callback *callback; > > > + * > > > + * wrapped_display = wl_proxy_create_wrapper((struct wl_proxy *) > > display); > > > + * callback = wl_display_sync((struct wl_display *) wrapped_display); > > > + * wl_proxy_destroy(wrapped_display); > > > + * wl_callback_add_listener(callback, ...); > > > + * > > > + * \memberof wl_proxy > > > + */ > > > +WL_EXPORT struct wl_proxy * > > > +wl_proxy_create_wrapper(struct wl_proxy *proxy) > > > +{ > > > + struct wl_proxy *wrapper; > > > + > > > + wrapper = zalloc(sizeof *wrapper); > > > + if (!wrapper) > > > + return NULL; > > > + > > > + pthread_mutex_lock(&proxy->display->mutex); > > > + > > > + if ((proxy->flags & WL_PROXY_FLAG_ID_DELETED) || > > > + (proxy->flags & WL_PROXY_FLAG_DESTROYED)) { > > > + pthread_mutex_unlock(&proxy->display->mutex); > > > + wl_log("proxy %p already deleted or destroyed flag: > > 0x%x\n", > > > + proxy, proxy->flags); > > > + return NULL; > > > + } > > > + > > > + wrapper->object.interface = proxy->object.interface; > > > + wrapper-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a> = proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a>; > > > + wrapper->display = proxy->display; > > > + wrapper->queue = proxy->queue; > > > + wrapper->flags = WL_PROXY_FLAG_WRAPPER; > > > + wrapper->refcount = 1; > > > + > > > + pthread_mutex_unlock(&proxy->display->mutex); > > > + > > > + return wrapper; > > > +} > > > + > > > WL_EXPORT void > > > wl_log_set_handler_client(wl_log_func_t handler) > > > { > > > diff --git a/src/wayland-private.h b/src/wayland-private.h > > > index 17c507c..f072811 100644 > > > --- a/src/wayland-private.h > > > +++ b/src/wayland-private.h > > > @@ -29,6 +29,7 @@ > > > #define WAYLAND_PRIVATE_H > > > > > > #include <stdarg.h> > > > +#include <stdlib.h> > > > > > > #define WL_HIDE_DEPRECATED 1 > > > > > > @@ -71,6 +72,12 @@ struct wl_map { > > > > > > typedef void (*wl_iterator_func_t)(void *element, void *data); > > > > > > +static inline void * > > > +zalloc(size_t size) > > > +{ > > > + return calloc(1, size); > > > +} > > > + > > > void wl_map_init(struct wl_map *map, uint32_t side); > > > void wl_map_release(struct wl_map *map); > > > uint32_t wl_map_insert_new(struct wl_map *map, uint32_t flags, void > > *data); > > > -- > > > 2.1.4 > > > > > > _______________________________________________ > > > wayland-devel mailing list > > > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> > > > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> > > _______________________________________________ > > wayland-devel mailing list > > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a> > > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a> > > > > Regards, > Marek </div></div></blockquote></div> </div><div class="gmail_extra">Thanks, </div><div class="gmail_extra">Marek </div></div>