2015-06-29 14:30 GMT+03:00 Jonas Ådahl <jad...@gmail.com>:
> 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 <jad...@gmail.com>
> ---
>
> 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.
--
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->object.id);
> - else if (proxy->object.id < WL_SERVER_ID_START)
> - wl_map_insert_at(&proxy->display->objects, 0,
> - proxy->object.id, WL_ZOMBIE_OBJECT);
> - else
> - wl_map_insert_at(&proxy->display->objects, 0,
> - proxy->object.id, NULL);
> -
> + if (proxy->flags != WL_PROXY_FLAG_WRAPPER) {
> + if (proxy->flags & WL_PROXY_FLAG_ID_DELETED)
> + wl_map_remove(&proxy->display->objects,
> + proxy->object.id);
> + else if (proxy->object.id < WL_SERVER_ID_START)
> + wl_map_insert_at(&proxy->display->objects, 0,
> + proxy->object.id, WL_ZOMBIE_OBJECT);
> + else
> + wl_map_insert_at(&proxy->display->objects, 0,
> + proxy->object.id, 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->object.id = proxy->object.id;
> + 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
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
