On Fri, Apr 29, 2016 at 02:08:06PM +0300, Pekka Paalanen wrote: > On Fri, 29 Apr 2016 14:24:32 +0800 > Jonas Ådahl <[email protected]> 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 <[email protected]> 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 <[email protected]> > > > > --- > > > > > > > > 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(-) > > > > > _______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
