[RFC wayland 1/2] client: Introduce 'proxy wrapper' concept

Jonas Ådahl jadahl at gmail.com
Wed Jul 1 01:10:26 PDT 2015


On Wed, Jul 01, 2015 at 09:51:41AM +0200, Marek Chalupa wrote:
> On Wed, Jul 1, 2015 at 9:16 AM, Jonas Ådahl <jadahl at gmail.com> wrote:
> 
> > On Wed, Jul 01, 2015 at 08:57:05AM +0200, Marek Chalupa wrote:
> > > On Mon, Jun 29, 2015 at 3:37 PM, Giulio Camuffo <giuliocamuffo at gmail.com
> > >
> > > wrote:
> > >
> > > > 2015-06-29 14:30 GMT+03:00 Jonas Ådahl <jadahl at 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 <jadahl at 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.
> > > >
> >
> > I agree that the second approach seems less like a work around, both
> > implementation wise and API wise.
> >
> > However I see one benefit with the first approach that the second lacks
> > and that is that one will be able to have code that is unaware of queues
> > and dispatching, only doing requests and listening on new objects on
> > whatever the default queue happens to be, while the "parent" code is the
> > one running the main loop being aware of what thread it is running,
> > dispatching on the right queue etc. Not sure it's a use case worth the
> > clunkyness how the API would look though.
> >
> > > >
> > > With this second approach there's still a race before setting listener.
> > > We should be able to set queue and listener atomically to get rid of this
> > > race.
> > >
> > > So something like:
> > >
> > > wl_proxy_create_full(..., queue, listener)
> > >
> > > + the scanner thing?
> >
> > Since the listener implementation won't be invoked until the events are
> > dispatched, and if one sets a separate queue, they won't be dispatched
> > until the user dispatches the queue it set. So in that case, there is no
> > race as far as I can tell.
> >
> >
> Yes, it works if the queue gets dispatched after setting the listener.
> But what if, for example, the client has one thread that is dispatching the
> queue
> and another that would create proxy with this queue? In this case the
> dispatching could
> occur before setting the listener, right?

That is true, but I don't think sharing a queue over multiple threads
has been considered a valid use case, i.e. one should have one queue per
thread. Or is there something EGL:y that requires it to work?


Jonas

> 
> 
> >
> > Jonas
> >
> > >
> > >
> > > >
> > > > --
> > > > 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 at lists.freedesktop.org
> > > > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > > > _______________________________________________
> > > > wayland-devel mailing list
> > > > wayland-devel at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> > > >
> > >
> > > Regards,
> > > Marek
> >
> 
> Thanks,
> Marek


More information about the wayland-devel mailing list