On Fri, 29 Apr 2016 14:24:32 +0800 Jonas Ådahl <jad...@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 <jad...@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 <jad...@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(-) > >
pgpNunFTtXBYy.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel