On 28/04/16 02:31 AM, Jonas Ådahl wrote: > Using the libwayland-client client API with multiple threads with > thread local queues are prone to race conditions. > > The problem is that one thread can read and queue events after another > thread creates a proxy but before it sets the queue. > > This may result in the event to the proxy being silently dropped, or > potentially dispatched on the wrong thread had the creating thread set > the implementation before setting the queue. > > This patch introduces API to solve this case by introducing "proxy > wrappers". In short, a proxy wrapper is a wl_proxy struct that will > never itself proxy any events, but may be used by the client to set a > queue, and use it instead of the original proxy when sending requests > that creates new proxies. 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 inherit to the > same proxy queue as the wrapper. > > https://bugs.freedesktop.org/show_bug.cgi?id=91273
Ugh, that bug ticket is ugly and doesn't seem to have actually started life as this bug at all... :) > > Signed-off-by: Jonas Ådahl <[email protected]> > --- > > Changes since v2: > > - Reworded the commit message and added bugzilla link > - Changed some wl_log("warning: ..."); return; to wl_abort(...); > - Made wl_proxy_create_wrapper() set errno on error (including EINVAL) > - Made it allowed to create a wrapper from a wrapper - this was an arbitrary > restriction, and I believe it makes sense to allow it Is there a reasonable use case for wrapping a wrapper? > - Changed flags == ... to flags & ... > - Documentation fixes (The documentation is excellent.) > > Jonas > > > > src/wayland-client-core.h | 6 +++ > src/wayland-client.c | 127 > +++++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 132 insertions(+), 1 deletion(-) > > diff --git a/src/wayland-client-core.h b/src/wayland-client-core.h > index 91f7e7b..b1d6515 100644 > --- a/src/wayland-client-core.h > +++ b/src/wayland-client-core.h > @@ -132,6 +132,12 @@ struct wl_proxy * > wl_proxy_create(struct wl_proxy *factory, > const struct wl_interface *interface); > > +void * > +wl_proxy_create_wrapper(void *proxy); > + > +void > +wl_proxy_wrapper_destroy(void *proxy_wrapper); > + > struct wl_proxy * > wl_proxy_marshal_constructor(struct wl_proxy *proxy, > uint32_t opcode, > diff --git a/src/wayland-client.c b/src/wayland-client.c > index 0c2ab32..3aad9d0 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 { > @@ -425,6 +426,8 @@ proxy_destroy(struct wl_proxy *proxy) > * > * \param proxy The proxy to be destroyed > * > + * \c proxy must not be a proxy wrapper. > + * > * \memberof wl_proxy > */ > WL_EXPORT void > @@ -432,6 +435,9 @@ wl_proxy_destroy(struct wl_proxy *proxy) > { > struct wl_display *display = proxy->display; > > + if (proxy->flags & WL_PROXY_FLAG_WRAPPER) > + wl_abort("Tried to destroy wrapper with wl_proxy_destroy()\n"); > + > pthread_mutex_lock(&display->mutex); > proxy_destroy(proxy); > pthread_mutex_unlock(&display->mutex); > @@ -452,12 +458,19 @@ wl_proxy_destroy(struct wl_proxy *proxy) > * \c n, \c implementation[n] should point to the handler of \c n for > * the given object. > * > + * \c proxy must not be a proxy wrapper. > + * > * \memberof wl_proxy > */ > 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; > + } I wouldn't mind seeing this be an abort() as well, honestly... > + > if (proxy->object.implementation || proxy->dispatcher) { > wl_log("proxy %p already has listener\n", proxy); > return -1; > @@ -504,6 +517,8 @@ wl_proxy_get_listener(struct wl_proxy *proxy) > * The exact details of dispatcher_data depend on the dispatcher used. This > * function is intended to be used by language bindings, not user code. > * > + * \c proxy must not be a proxy wrapper. > + * > * \memberof wl_proxy > */ > WL_EXPORT int > @@ -511,6 +526,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; > + } Maybe even here too... :) > + > if (proxy->object.implementation || proxy->dispatcher) { > wl_log("proxy %p already has listener\n", proxy); > return -1; > @@ -1952,6 +1972,111 @@ wl_proxy_set_queue(struct wl_proxy *proxy, struct > wl_event_queue *queue) > proxy->queue = &proxy->display->default_queue; > } > > +/** Create a proxy wrapper for making queue assignments thread-safe > + * > + * \param proxy The proxy object to be wrapped > + * \return A proxy wrapper for the given proxy or NULL on failure > + * > + * A proxy wrapper is type of 'struct wl_proxy' instance that can be used > when > + * 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. > + * > + * Setting the proxy queue of the proxy wrapper will make new objects created > + * using the proxy wrapper use the set proxy queue. > + * 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 can only be destroyed using wl_proxy_wrapper_destroy(). > + * > + * A proxy wrapper must be destroyed before the proxy it was created from. > + * > + * Creating a proxy wrapper from a proxy that has either been destroyed or > + * removed will fail, errno will be set to EINVAL 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: > + * > + * \code > + * > + * struct wl_event_queue *queue = ...; > + * struct wl_display *wrapped_display; > + * struct wl_callback *callback; > + * > + * wrapped_display = wl_proxy_create_wrapper(display); > + * wl_proxy_set_queue((struct wl_proxy *) wrapped_display, queue); > + * callback = wl_display_sync(wrapped_display); > + * wl_proxy_wrapper_destroy(wrapped_display); > + * wl_callback_add_listener(callback, ...); > + * > + * \endcode > + * > + * \memberof wl_proxy > + */ > +WL_EXPORT void * > +wl_proxy_create_wrapper(void *proxy) > +{ > + struct wl_proxy *wrapped_proxy = proxy; > + struct wl_proxy *wrapper; > + > + wrapper = zalloc(sizeof *wrapper); > + if (!wrapper) > + return NULL; > + > + pthread_mutex_lock(&wrapped_proxy->display->mutex); > + > + if ((wrapped_proxy->flags & WL_PROXY_FLAG_ID_DELETED) || > + (wrapped_proxy->flags & WL_PROXY_FLAG_DESTROYED)) { > + pthread_mutex_unlock(&wrapped_proxy->display->mutex); > + wl_log("proxy %p already deleted or destroyed flag: 0x%x\n", > + wrapped_proxy, wrapped_proxy->flags); > + free(wrapper); > + errno = EINVAL; > + return NULL; > + } > + > + wrapper->object.interface = wrapped_proxy->object.interface; > + wrapper->object.id = wrapped_proxy->object.id; > + wrapper->version = wrapped_proxy->version; > + wrapper->display = wrapped_proxy->display; > + wrapper->queue = wrapped_proxy->queue; Ok, this intentionally omits the dispatcher, since a wrapper won't dispatch... > + wrapper->flags = WL_PROXY_FLAG_WRAPPER; > + wrapper->refcount = 1; > + > + pthread_mutex_unlock(&wrapped_proxy->display->mutex); > + > + return wrapper; > +} > + > +/** Destroy a proxy wrapper > + * \param proxy_wrapper The proxy wrapper to be destroyed > + * > + * \memberof wl_proxy > + */ > +WL_EXPORT void > +wl_proxy_wrapper_destroy(void *proxy_wrapper) > +{ > + struct wl_proxy *wrapper = proxy_wrapper; > + > + if (!(wrapper->flags & WL_PROXY_FLAG_WRAPPER)) > + wl_abort("Tried to destroy non-wrapper proxy with " > + "wl_proxy_wrapper_destroy\n"); > + > + assert(wrapper->refcount == 1); > + > + free(wrapper); > +} > + > WL_EXPORT void > wl_log_set_handler_client(wl_log_func_t handler) > { > I really can't think of a better way to solve this problem - and it's less code than I'd have expected for what it solves. I'd like to see it be a little meaner (the suggested aborts), and if we don't have a reason to wrap wrappers at all we can just short circuit any discussion of the correctness of that implementation. ;) Otherwise, this gets my Reviewed-by: Derek Foreman <[email protected]> _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
