<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 1, 2015 at 9:16 AM, Jonas Ådahl <span dir="ltr"><<a href="mailto:jadahl@gmail.com" target="_blank">jadahl@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Jul 01, 2015 at 08:57:05AM +0200, Marek Chalupa wrote:<br>
> On Mon, Jun 29, 2015 at 3:37 PM, Giulio Camuffo <<a href="mailto:giuliocamuffo@gmail.com">giuliocamuffo@gmail.com</a>><br>
> wrote:<br>
><br>
> > 2015-06-29 14:30 GMT+03:00 Jonas Ådahl <<a href="mailto:jadahl@gmail.com">jadahl@gmail.com</a>>:<br>
> > > Arnaud Vrac discovered an issue in the libwayland client API causing<br>
> > > race conditions when doing round trips using the wl_display object using<br>
> > > non-default proxy queues.<br>
> > ><br>
> > > The problem is that one thread can read and dispatch events after<br>
> > > another thread creates the wl_callback object but before it sets the<br>
> > > queue. This could potentially cause the events to be dropped completely<br>
> > > if the event is dispatched before the creating thread sets the<br>
> > > implementation, or that the event is dispatched on the wrong queue which<br>
> > > might run on another thread.<br>
> > ><br>
> > > This patch introduces API to solve this case by introducing "proxy<br>
> > > wrappers". In short, a proxy wrapper is a struct wl_proxy * that will<br>
> > > never itself emit any events, but the client can set a queue, and use it<br>
> > > when sending requests that creates new proxy objects. When sending<br>
> > > requests, the wrapper will work in the same way as the normal proxy<br>
> > > object, but the proxy created by sending a request (for example<br>
> > > wl_display.sync) will default to the same proxy queue as the wrapper.<br>
> > ><br>
> > > Signed-off-by: Jonas Ådahl <<a href="mailto:jadahl@gmail.com">jadahl@gmail.com</a>><br>
> > > ---<br>
> > ><br>
> > > Hi,<br>
> > ><br>
> > > This is one of the two alternative solutions that was discussed on<br>
> > #wayland.<br>
> > ><br>
> > > The other one being introducing API alternatives to<br>
> > > wl_proxy_marshal_constructor and wl_proxy_marshal_array_constructor that<br>
> > > also take a queue and also changing scanner.c to produce functions that<br>
> > > take a queue for every request that creates a proxy.<br>
> > ><br>
> > > In short, instead of<br>
> > ><br>
> > >         bar = wl_foo_get_bar(foo);<br>
> > >         wl_proxy_set_queue((struct wl_proxy *) bar, queue);<br>
> > >         wl_bar_add_listener(bar, ...);<br>
> > ><br>
> > > with this RFC a client does<br>
> > ><br>
> > >         foo_wrapper = wl_proxy_create_wrapper((struct wl_proxy *) foo);<br>
> > >         wl_proxy_set_queue((struct wl_proxy *) foo_wrapper, queue);<br>
> > ><br>
> > >         bar = wl_foo_get(foo_wrapper);<br>
> > >         wl_bar_add_listener(bar, ...);<br>
> > ><br>
> > > and the with other idea that is implemented anywhere yet AFAIK<br>
> > ><br>
> > >         bar = wl_foo_get_bar_with_queue(foo, queue)<br>
> > >         wl_bar_add_listener(bar, ...);<br>
> ><br>
> > Hi, i think this second option is better. The proxy approach looks to<br>
> > me like a clunky and unnecessary workaround, while i don't see<br>
> > downsides to new api taking a queue.<br>
> ><br>
<br>
</div></div>I agree that the second approach seems less like a work around, both<br>
implementation wise and API wise.<br>
<br>
However I see one benefit with the first approach that the second lacks<br>
and that is that one will be able to have code that is unaware of queues<br>
and dispatching, only doing requests and listening on new objects on<br>
whatever the default queue happens to be, while the "parent" code is the<br>
one running the main loop being aware of what thread it is running,<br>
dispatching on the right queue etc. Not sure it's a use case worth the<br>
clunkyness how the API would look though.<br>
<span class=""><br>
> ><br>
> With this second approach there's still a race before setting listener.<br>
> We should be able to set queue and listener atomically to get rid of this<br>
> race.<br>
><br>
> So something like:<br>
><br>
> wl_proxy_create_full(..., queue, listener)<br>
><br>
> + the scanner thing?<br>
<br>
</span>Since the listener implementation won't be invoked until the events are<br>
dispatched, and if one sets a separate queue, they won't be dispatched<br>
until the user dispatches the queue it set. So in that case, there is no<br>
race as far as I can tell.<br>
<span class="HOEnZb"><font color="#888888"><br></font></span></blockquote><div><br></div><div>Yes, it works if the queue gets dispatched after setting the listener.<br>But what if, for example, the client has one thread that is dispatching the queue<br>and another that would create proxy with this queue? In this case the dispatching could<br></div><div>occur before setting the listener, right?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="HOEnZb"><font color="#888888">
<br>
Jonas<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> ><br>
> > --<br>
> > Giulio<br>
> ><br>
> > ><br>
> > > Opinions?<br>
> > ><br>
> > ><br>
> > > Jonas<br>
> > ><br>
> > ><br>
> > ><br>
> > >  src/wayland-client-core.h |   1 +<br>
> > >  src/wayland-client.c      | 106<br>
> > +++++++++++++++++++++++++++++++++++++++++-----<br>
> > >  src/wayland-private.h     |   7 +++<br>
> > >  3 files changed, 103 insertions(+), 11 deletions(-)<br>
> > ><br>
> > > diff --git a/src/wayland-client-core.h b/src/wayland-client-core.h<br>
> > > index dea70d9..0db8b9c 100644<br>
> > > --- a/src/wayland-client-core.h<br>
> > > +++ b/src/wayland-client-core.h<br>
> > > @@ -125,6 +125,7 @@ void wl_proxy_marshal_array(struct wl_proxy *p,<br>
> > uint32_t opcode,<br>
> > >                             union wl_argument *args);<br>
> > >  struct wl_proxy *wl_proxy_create(struct wl_proxy *factory,<br>
> > >                                  const struct wl_interface *interface);<br>
> > > +struct wl_proxy *wl_proxy_create_wrapper(struct wl_proxy *proxy);<br>
> > >  struct wl_proxy *wl_proxy_marshal_constructor(struct wl_proxy *proxy,<br>
> > >                                               uint32_t opcode,<br>
> > >                                               const struct wl_interface<br>
> > *interface,<br>
> > > diff --git a/src/wayland-client.c b/src/wayland-client.c<br>
> > > index 6450b67..f8c64c4 100644<br>
> > > --- a/src/wayland-client.c<br>
> > > +++ b/src/wayland-client.c<br>
> > > @@ -51,7 +51,8 @@<br>
> > ><br>
> > >  enum wl_proxy_flag {<br>
> > >         WL_PROXY_FLAG_ID_DELETED = (1 << 0),<br>
> > > -       WL_PROXY_FLAG_DESTROYED = (1 << 1)<br>
> > > +       WL_PROXY_FLAG_DESTROYED = (1 << 1),<br>
> > > +       WL_PROXY_FLAG_WRAPPER = (1 << 2),<br>
> > >  };<br>
> > ><br>
> > >  struct wl_proxy {<br>
> > > @@ -405,17 +406,19 @@ wl_proxy_create_for_id(struct wl_proxy *factory,<br>
> > >  void<br>
> > >  proxy_destroy(struct wl_proxy *proxy)<br>
> > >  {<br>
> > > -       if (proxy->flags & WL_PROXY_FLAG_ID_DELETED)<br>
> > > -               wl_map_remove(&proxy->display->objects, proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a><br>
> > );<br>
> > > -       else if (proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a> < WL_SERVER_ID_START)<br>
> > > -               wl_map_insert_at(&proxy->display->objects, 0,<br>
> > > -                                proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a>, WL_ZOMBIE_OBJECT);<br>
> > > -       else<br>
> > > -               wl_map_insert_at(&proxy->display->objects, 0,<br>
> > > -                                proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a>, NULL);<br>
> > > -<br>
> > > +       if (proxy->flags != WL_PROXY_FLAG_WRAPPER) {<br>
> > > +               if (proxy->flags & WL_PROXY_FLAG_ID_DELETED)<br>
> > > +                       wl_map_remove(&proxy->display->objects,<br>
> > > +                                     proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a>);<br>
> > > +               else if (proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a> < WL_SERVER_ID_START)<br>
> > > +                       wl_map_insert_at(&proxy->display->objects, 0,<br>
> > > +                                        proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a>,<br>
> > WL_ZOMBIE_OBJECT);<br>
> > > +               else<br>
> > > +                       wl_map_insert_at(&proxy->display->objects, 0,<br>
> > > +                                        proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a>, NULL);<br>
> > ><br>
> > > -       proxy->flags |= WL_PROXY_FLAG_DESTROYED;<br>
> > > +               proxy->flags |= WL_PROXY_FLAG_DESTROYED;<br>
> > > +       }<br>
> > ><br>
> > >         proxy->refcount--;<br>
> > >         if (!proxy->refcount)<br>
> > > @@ -459,6 +462,11 @@ WL_EXPORT int<br>
> > >  wl_proxy_add_listener(struct wl_proxy *proxy,<br>
> > >                       void (**implementation)(void), void *data)<br>
> > >  {<br>
> > > +       if (proxy->flags == WL_PROXY_FLAG_WRAPPER) {<br>
> > > +               wl_log("proxy %p is a wrapper\n", proxy);<br>
> > > +               return -1;<br>
> > > +       }<br>
> > > +<br>
> > >         if (proxy->object.implementation || proxy->dispatcher) {<br>
> > >                 wl_log("proxy %p already has listener\n", proxy);<br>
> > >                 return -1;<br>
> > > @@ -512,6 +520,11 @@ wl_proxy_add_dispatcher(struct wl_proxy *proxy,<br>
> > >                         wl_dispatcher_func_t dispatcher,<br>
> > >                         const void *implementation, void *data)<br>
> > >  {<br>
> > > +       if (proxy->flags == WL_PROXY_FLAG_WRAPPER) {<br>
> > > +               wl_log("proxy %p is a wrapper\n", proxy);<br>
> > > +               return -1;<br>
> > > +       }<br>
> > > +<br>
> > >         if (proxy->object.implementation || proxy->dispatcher) {<br>
> > >                 wl_log("proxy %p already has listener\n");<br>
> > >                 return -1;<br>
> > > @@ -1885,6 +1898,77 @@ wl_proxy_set_queue(struct wl_proxy *proxy, struct<br>
> > wl_event_queue *queue)<br>
> > >                 proxy->queue = &proxy->display->default_queue;<br>
> > >  }<br>
> > ><br>
> > > +/** Create a proxy wrapper<br>
> > > + *<br>
> > > + * \param proxy The proxy object to be wrapped<br>
> > > + * \return A proxy wrapper for the given proxy<br>
> > > + *<br>
> > > + * A proxy wrapper is type of 'struct wl_proxy' instance that can be<br>
> > used for<br>
> > > + * sending requests instead of using the original proxy. A proxy<br>
> > wrapper does<br>
> > > + * not have an implementation or dispatcher, and events received on the<br>
> > > + * object is still emitted on the original proxy. Trying to set an<br>
> > > + * implementation or dispatcher will have no effect but result in a<br>
> > warning<br>
> > > + * being logged.<br>
> > > + *<br>
> > > + * Even though there is no implementation nor dispatcher, the proxy<br>
> > queue can<br>
> > > + * be changed. This will affect the default queue of new objects<br>
> > created by<br>
> > > + * requests sent via the proxy wrapper.<br>
> > > + *<br>
> > > + * A proxy wrapper must be destroyed before the proxy it was created<br>
> > from.<br>
> > > + * Creating a proxy wrapper on a proxy that has either been destroyed or<br>
> > > + * removed will fail, and NULL will be returned.<br>
> > > + *<br>
> > > + * If a user reads and dispatches events on more than one thread, it is<br>
> > > + * necessary to use a proxy wrapper when sending requests on objects<br>
> > when the<br>
> > > + * intention is that a newly created proxy is to use a proxy queue<br>
> > different<br>
> > > + * from the proxy the request was sent on, as creating the new proxy<br>
> > and then<br>
> > > + * setting the queue is not thread safe.<br>
> > > + *<br>
> > > + * For example, a module that runs using its own proxy queue that needs<br>
> > to<br>
> > > + * do display roundtrip must wrap the wl_display proxy object before<br>
> > sending<br>
> > > + * the wl_display.sync request. For example:<br>
> > > + *<br>
> > > + * struct wl_proxy *wrapped_display;<br>
> > > + * struct wl_callback *callback;<br>
> > > + *<br>
> > > + * wrapped_display = wl_proxy_create_wrapper((struct wl_proxy *)<br>
> > display);<br>
> > > + * callback = wl_display_sync((struct wl_display *) wrapped_display);<br>
> > > + * wl_proxy_destroy(wrapped_display);<br>
> > > + * wl_callback_add_listener(callback, ...);<br>
> > > + *<br>
> > > + * \memberof wl_proxy<br>
> > > + */<br>
> > > +WL_EXPORT struct wl_proxy *<br>
> > > +wl_proxy_create_wrapper(struct wl_proxy *proxy)<br>
> > > +{<br>
> > > +       struct wl_proxy *wrapper;<br>
> > > +<br>
> > > +       wrapper = zalloc(sizeof *wrapper);<br>
> > > +       if (!wrapper)<br>
> > > +               return NULL;<br>
> > > +<br>
> > > +       pthread_mutex_lock(&proxy->display->mutex);<br>
> > > +<br>
> > > +       if ((proxy->flags & WL_PROXY_FLAG_ID_DELETED) ||<br>
> > > +           (proxy->flags & WL_PROXY_FLAG_DESTROYED)) {<br>
> > > +               pthread_mutex_unlock(&proxy->display->mutex);<br>
> > > +               wl_log("proxy %p already deleted or destroyed flag:<br>
> > 0x%x\n",<br>
> > > +                      proxy, proxy->flags);<br>
> > > +               return NULL;<br>
> > > +       }<br>
> > > +<br>
> > > +       wrapper->object.interface = proxy->object.interface;<br>
> > > +       wrapper-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a> = proxy-><a href="http://object.id" rel="noreferrer" target="_blank">object.id</a>;<br>
> > > +       wrapper->display = proxy->display;<br>
> > > +       wrapper->queue = proxy->queue;<br>
> > > +       wrapper->flags = WL_PROXY_FLAG_WRAPPER;<br>
> > > +       wrapper->refcount = 1;<br>
> > > +<br>
> > > +       pthread_mutex_unlock(&proxy->display->mutex);<br>
> > > +<br>
> > > +       return wrapper;<br>
> > > +}<br>
> > > +<br>
> > >  WL_EXPORT void<br>
> > >  wl_log_set_handler_client(wl_log_func_t handler)<br>
> > >  {<br>
> > > diff --git a/src/wayland-private.h b/src/wayland-private.h<br>
> > > index 17c507c..f072811 100644<br>
> > > --- a/src/wayland-private.h<br>
> > > +++ b/src/wayland-private.h<br>
> > > @@ -29,6 +29,7 @@<br>
> > >  #define WAYLAND_PRIVATE_H<br>
> > ><br>
> > >  #include <stdarg.h><br>
> > > +#include <stdlib.h><br>
> > ><br>
> > >  #define WL_HIDE_DEPRECATED 1<br>
> > ><br>
> > > @@ -71,6 +72,12 @@ struct wl_map {<br>
> > ><br>
> > >  typedef void (*wl_iterator_func_t)(void *element, void *data);<br>
> > ><br>
> > > +static inline void *<br>
> > > +zalloc(size_t size)<br>
> > > +{<br>
> > > +       return calloc(1, size);<br>
> > > +}<br>
> > > +<br>
> > >  void wl_map_init(struct wl_map *map, uint32_t side);<br>
> > >  void wl_map_release(struct wl_map *map);<br>
> > >  uint32_t wl_map_insert_new(struct wl_map *map, uint32_t flags, void<br>
> > *data);<br>
> > > --<br>
> > > 2.1.4<br>
> > ><br>
> > > _______________________________________________<br>
> > > wayland-devel mailing list<br>
> > > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a><br>
> > > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a><br>
> > _______________________________________________<br>
> > wayland-devel mailing list<br>
> > <a href="mailto:wayland-devel@lists.freedesktop.org">wayland-devel@lists.freedesktop.org</a><br>
> > <a href="http://lists.freedesktop.org/mailman/listinfo/wayland-devel" rel="noreferrer" target="_blank">http://lists.freedesktop.org/mailman/listinfo/wayland-devel</a><br>
> ><br>
><br>
> Regards,<br>
> Marek<br>
</div></div></blockquote></div><br></div><div class="gmail_extra">Thanks,<br></div><div class="gmail_extra">Marek<br></div></div>