[PATCH wayland v3] client: Introduce proxy wrappers

[Pekka Paalanen]

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.

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.


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(-)  
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160429/66579dd4/attachment.sig>