[PATCH wayland v3] client: Introduce proxy wrappers

[Jonas Ådahl]

On Fri, Apr 29, 2016 at 02:08:06PM +0300, Pekka Paalanen wrote:
> On Fri, 29 Apr 2016 14:24:32 +0800
> Jonas Ådahl <jadahl at gmail.com> wrote:
> 
> > 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?
> 
> Hi,
> 
> I'm more interested in why should wrapping a dead proxy fail. If
> someone is using a proxy that might be dead, it's already a race. If it
> doesn't matter whether the race is won or lost, failing to create a
> wrapper on a "dead" proxy is just yet another thing to check. If you
> can wrap a dead object, it won't create any problems that were not
> there already.
> 
> I don't see a reason why wrapping a dead object should fail. It would
> be much worse if you wrap first and then the object becomes dead - the
> wrapper has no idea it's dead, and you can send a request with a bad
> object id anyway.
> 
> I am ambivalent on wrapping a wrapper, either way is fine by me.

Then lets just skip testing for both the type of proxy and whether the
proxy was dead or not.

> 
> Btw. those "this should be a wl_abort" cases that Derek pointed out I
> simply missed in my review, and I agree with Derek. I did flag them the
> first time.

Sorry, missed to fix those.


Jonas

> 
> 
> Thanks,
> pq
> 
> > > 
> > > 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(-)  
> > 
> > 
>