[PATCH wayland v3] client: Introduce proxy wrappers

[Jonas Ådahl]

On Thu, Apr 28, 2016 at 02:28:21PM +0300, Pekka Paalanen wrote:
> On Thu, 28 Apr 2016 15:31:31 +0800
> Jonas Ådahl <jadahl at gmail.com> 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
> > 
> > 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
> >  - Changed flags == ... to flags & ...
> >  - Documentation fixes
> 
> Hi Jonas,
> 
> hmm, but if you allow creating a wrapper 'B' from a wrapper 'A', you
> also allow creating a wrapper for a dead underlying real proxy 'P'.
> Like this:
> 
> 1. create 'A' from 'P'
> 2. destroy 'P'
> 3. create 'B' from 'A'
> 
> That happens because a wrapper can never get the "dead" flags.
> 
> Will that be a problem?
> 
> I'm not sure, so I'd like to err on the safe side: forbid creating a
> wrapper from a wrapper for now. If someone needs it, we can allow it
> later. We cannot deny it later that easily. How's that?
> 
> Or, do not error out on creating a wrapper from a dead object. Creating
> a wrapper from a dead object and sending request on that is no
> different to sending a request on the original proxy, right?
> Maybe that would make it even a bit easier to use, as the only case of
> wrapper creation failing would be OOM.

The thought behind making wrapping a wrapper allowed is if we have the
following scenario:

- main thread has a wl_display

- main thread starts a worker pool thread with queue Q_a, that does
  *stuff*, it gives it a wrapper of wl_display so that it can round trip
  etc.

 - worker pool thread stars workers that does *sub-stuff*, and each have
   their own queue. worker pool gives them one wrapped wl_display each.

In this scenario, it'd help to be able to wrap a wrapper, because we
wouldn't need to pass around the real wl_display alongside the wrapper.

I don't know whether this is important enough. It might just as well
make just as much sense to pass around the real wl_display and let
everyone else do their own wrapping.

As this is just imaginare use cases, so I really don't have a strong
opinion whether to allow wrapping wrappers, wrapping removed proxies, or
just disallow everything that our particular thought out use case.

Are you (and you as well Derek) still leaning towards the more
restrictive path?


Jonas

> 
> Otherwise all the changes look good to me, and the above question is
> the only issue barring my R-b.
> 
> 
> Thanks,
> pq
> 
> >  src/wayland-client-core.h |   6 +++
> >  src/wayland-client.c      | 127 +++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 132 insertions(+), 1 deletion(-)