[PATCH wayland 1/4 v2] client: Introduce proxy wrappers
Pekka Paalanen
ppaalanen at gmail.com
Wed Apr 27 09:49:07 UTC 2016
On Wed, 27 Apr 2016 15:37:38 +0800
Jonas Ådahl <jadahl at gmail.com> 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 dispatch events after
Hi Jonas,
this should say: "The problem is that one thread can read and *queue*
events ..."
But yes, queueing to the wrong queue may then cause all of the below. I
think it would be good to explain more explicitly that queueing to the
wrong queue is the root of problems here.
> 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 wl_proxy struct that will
> never itself emit 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 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.
>
Bugzilla link?
> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> ---
>
> Changes since v1:
>
> - Function type signatures changed to use more void * since
> in all cases the wrapper needs to be type casted anyway. Using
> void * avoids several casts.
> - Require destroying wrappers using wl_proxy_wrapper_destroy()
>
>
> Jonas
>
> src/wayland-client-core.h | 6 +++
> src/wayland-client.c | 135 +++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 140 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 33033e7..f21b5f7 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 {
> @@ -428,6 +429,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
> @@ -435,6 +438,12 @@ wl_proxy_destroy(struct wl_proxy *proxy)
> {
> struct wl_display *display = proxy->display;
>
> + if (proxy->flags == WL_PROXY_FLAG_WRAPPER) {
Should this not be testing (proxy->flags & WL_PROXY_FLAG_WRAPPER)?
It would make more sense to the casual reader, even if no other flag
was possible with WRAPPER. Also more future-proof.
This comment applies to all such checks below.
> + wl_log("warning: tried to destroy wrapper with "
> + "wl_proxy_destroy()\n");
This is always an API user error. How about we abort() here to make it
painfully obvious?
At least for starters. Removing the abort() later is possible, while
adding it later is not.
I would propose this for all error cases where a function must never be
called with a wrapper.
> + return;
> + }
> +
> pthread_mutex_lock(&display->mutex);
> proxy_destroy(proxy);
> pthread_mutex_unlock(&display->mutex);
> @@ -455,12 +464,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);
abort()?
> + return -1;
> + }
> +
> if (proxy->object.implementation || proxy->dispatcher) {
> wl_log("proxy %p already has listener\n", proxy);
> return -1;
> @@ -507,6 +523,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
> @@ -514,6 +532,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);
abort()?
> + return -1;
> + }
> +
> if (proxy->object.implementation || proxy->dispatcher) {
> wl_log("proxy %p already has listener\n", proxy);
> return -1;
> @@ -1955,6 +1978,116 @@ 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 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
"are still emitted"
> + * implementation or dispatcher will have no effect but result in a warning
> + * being logged.
As said above, I'd prefer abort() for starters.
> + *
> + * 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 using before the proxy it was created from.
-using
> + * 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.
This is the most important message. Could we summarize it in the short
description, maybe as: "Create a proxy wrapper for making queue
assignments thread-safe"? It's a bit long, but it needs to get
attention.
> + *
> + * 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
Yess.
> + */
> +WL_EXPORT void *
> +wl_proxy_create_wrapper(void *proxy)
> +{
> + struct wl_proxy *wrapped_proxy = proxy;
> + struct wl_proxy *wrapper;
> +
> + if (wrapped_proxy->flags == WL_PROXY_FLAG_WRAPPER) {
> + wl_log("tried to wrap proxy wrapper\n");
> + return NULL;
I think this case was not mentioned in the doc.
> + }
> +
> + 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);
> + 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;
> + wrapper->flags = WL_PROXY_FLAG_WRAPPER;
> + wrapper->refcount = 1;
> +
> + pthread_mutex_unlock(&wrapped_proxy->display->mutex);
> +
> + return wrapper;
> +}
Looking good.
> +
> +/** 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_log("warning: tried to destroy non-wrapper proxy with "
> + "wl_proxy_wrapper_destroy\n");
abort()?
> + return;
> + }
> +
> + assert(wrapper->refcount == 1);
> +
> + free(wrapper);
> +}
> +
> WL_EXPORT void
> wl_log_set_handler_client(wl_log_func_t handler)
> {
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160427/8408b045/attachment.sig>
More information about the wayland-devel
mailing list