[PATCH wayland v3] client: Introduce proxy wrappers

[Derek Foreman]

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 <jadahl at gmail.com>
> ---
> 
> 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 <derekf at osg.samsung.com>