<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Mon, Jun 29, 2015 at 3:37 PM, Giulio Camuffo <<a href="mailto:giuliocamuffo@gmail.com" target="_blank">giuliocamuffo@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">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, ...); </div></div>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. </blockquote><div> </div><div>With this second approach there's still a race before setting listener. </div><div>We should be able to set queue and listener atomically to get rid of this race. </div><div>So something like: </div><div>wl_proxy_create_full(..., queue, listener) </div><div>+ the scanner thing? </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> -- Giulio <div class="HOEnZb"><div class="h5"> > > 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> </div></div></blockquote></div> </div><div class="gmail_extra">Regards, </div><div class="gmail_extra">Marek </div></div>