On Wed, 1 Jul 2015 09:51:41 +0200
Marek Chalupa <mchqwerty at gmail.com> 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.
I wonder if this is a real benefit. You can pass a proxy with a
different queue set for all objects to be created from it, but without
the callee knowing it is actually in a new queue. By your definition
the callee does not care about threads or queues, so it assumes any
callbacks it registers are called in the same thread it was originally
called in.
The benefit is that the caller who knows about threads and queues, can
choose to dispatch the parent object events separately from the
callee's events, and so ensure the callee's events do get called in the
right context.
That actually sounds pretty useful to me.
> > > >
> > > 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?
I'm really not sure if that is a valid use case or not.
There is no reason to deny sending requests on an object from a
different thread that the thread that dispatches its queue. That can
happen with EGL, if one thread is handling the wl_surface events, while
another thread bangs eglSwapBuffers, which internally uses
wl_surface.frame.
But that's not the question nor the use case, because there the
wl_callback needs to be in EGL's event queue, that is, the thread that
creates the new object.
Dispatching a queue from multiple threads is forbidden, or at least
won't lead to anything sensible.
The question is, can a thread assign an object to another thread's
queue?
I have very hard time even inventing a use case, but I'm likely not
imaginative enough.
However, we already have a user requirement: you must not destroy a
wl_event_queue if there are wl_proxies assigned to it. Sure, apps can
ensure that holds, but it seems like an additional trap, if we allowed
assigning my created object to another thread's queue.
I'm torn. I'd want to deny it, but then I'm sure someone will come in
two years with a valid use case who needs exactly this.
So, let's look at it from a different perspective: is there any harm in
allowing the assignment to another thread's queue?
Obviously it places the requirement of being able to set also the
listener, not just the queue, atomically when creating the new object.
Does a wl_proxy have any other user settable state at all? Yes. Here
are all the user settable state:
int wl_proxy_add_listener(struct wl_proxy *proxy,
void (**implementation)(void), void *data);
int wl_proxy_add_dispatcher(struct wl_proxy *proxy,
wl_dispatcher_func_t dispatcher_func,
const void * dispatcher_data, void *data);
void wl_proxy_set_user_data(struct wl_proxy *proxy, void *user_data);
void wl_proxy_set_queue(struct wl_proxy *proxy, struct wl_event_queue *queue);
So that makes implementation, user_data, dispatcher_func,
dispatcher_data, and queue. That is literally everything in our API,
and we should be prepared to be able to set all that atomically when
creating a new object. Quite many parameters, and some of them are
mutually exclusive.
I have hard time seeing the latter proposal (a single new function to
set everything) managing that, while the proxy-wrapper approach is even
extendable, if we ever need to add more fields to wl_proxy.
Hmm...
We also must not forget about language bindings. The real
libwayland-client ABI is the wl_proxy_*() API, not the generated C
functions. Whatever we come up with here, must also translate to other
language bindings.
Also, if we generate new function, we still need to keep the old
functions too. However, deprecating generated functions could be
achieved by wayland-scanner options... in cases where we do not already
ship generated headers.
Maybe we should have the proxy-wrapper as the libwayland-client ABI,
and use the single-function-sets-all style in the generated API?
Thanks,
pq