Summary: | wl_display_dispatch(_queue) documentation about thread safeness is probably wrong | ||
---|---|---|---|
Product: | Wayland | Reporter: | Jonas Ådahl <jadahl> |
Component: | wayland | Assignee: | Wayland bug list <wayland-bugs> |
Status: | RESOLVED FIXED | QA Contact: | |
Severity: | normal | ||
Priority: | medium | CC: | daniel, mchqwerty, ppaalanen |
Version: | unspecified | ||
Hardware: | Other | ||
OS: | All | ||
Whiteboard: | |||
i915 platform: | i915 features: | ||
Bug Depends on: | |||
Bug Blocks: | 83430, 91769 |
Description
Jonas Ådahl
2015-08-27 02:32:36 UTC
Adding Marek to CC, since IIRC we worked on the docs, but I have happily forgot all the details by now. (In reply to Jonas Ådahl from comment #0) > There is documentation about wl_display_dispatch() states one may not mix > wl_display_dispatch(_queue) with wl_display_prepare_read() and friends, but > this is probably a common misconception, because internally, > wl_display_dispatch_queue() is implemented in the same way as one would use > wl_display_prepare_read() and friends, just without using those functions. > > I have patches in progress that changes wl_display_dispatch_queue() to > simply use wl_display_prepare_read() and related API internally to make this > more obvious. I forgot not only the details :) But looking into the code... Clearly you cannot do something like: wl_display_prepare_read() wl_display_dispatch() in one thread, that would leak to dead-lock. You're right that the wl_display_dispatch() works on the same basis as the prepare+read API. I think that they should not be mixed for the same reason you cannot call wl_display_dispatch_queue() from more threads. That is, there is (probably) no dead-lock, but thread can get sleeping with events queued. http://cgit.freedesktop.org/wayland/wayland/tree/src/wayland-client.c#n1454 If you call prepare + poll + read in thread A and wl_display_dispatch_queue() in another thread B (dispatching two different queues), the fd could become readable before wl_display_dispatch_queue() calls poll (http://cgit.freedesktop.org/wayland/wayland/tree/src/wayland-client.c#n1574) and whole prepare+read sequence could run before it, queing events into both queues. After prepare+read queued events, thread B goes to sleep in poll with events hanging in the queue. Avoiding this behaviour was why wl_display_prepare_read() and wl_display_read_events() API was added and why programs dispatching events from more threads should use it, while programs with single dispatcher thread can use the usual wl_display_dispatch_...() Implementing wl_display_dispatch(_queue) using prepare+read should fix this IMHO.
> If you call prepare + poll + read in thread A and
> wl_display_dispatch_queue() in another thread B (dispatching two different
> queues), the fd could become readable before wl_display_dispatch_queue()
> calls poll
> (http://cgit.freedesktop.org/wayland/wayland/tree/src/wayland-client.
> c#n1574) and whole prepare+read sequence could run before it, queing events
> into both queues. After prepare+read queued events, thread B goes to sleep
> in poll with events hanging in the queue.
Oh, wait, dispatching queue and increasing reader_count is atomical in wl_display_dispatch_queue(), so the thread A should not events, so it should probably work
(In reply to Marek Chalupa from comment #3) > > If you call prepare + poll + read in thread A and > > wl_display_dispatch_queue() in another thread B (dispatching two different > > queues), the fd could become readable before wl_display_dispatch_queue() > > calls poll > > (http://cgit.freedesktop.org/wayland/wayland/tree/src/wayland-client. > > c#n1574) and whole prepare+read sequence could run before it, queing events > > into both queues. After prepare+read queued events, thread B goes to sleep > > in poll with events hanging in the queue. > > Oh, wait, dispatching queue and increasing reader_count is atomical in > wl_display_dispatch_queue(), so the thread A should not events, so it should > probably work Exactly. So mixing them is fine, as long as you don't call dispatch() between prepare() and cancel()/read() calls. Marking this as fixed now, since the following patch is now merged: commit 0edeeb9cd5806959660af54d63fe9d402d50e6e7 Author: Jonas Ådahl <jadahl@gmail.com> Date: Fri Oct 2 11:12:02 2015 +0800 client: Correct documentation regarding thread safeness The current documentation about wl_display_dispatch() states one may not mix wl_display_dispatch(_queue)() with wl_display_prepare_read() and friends, but this is a misconception about how wl_display_dispatch(_queue)() works. The fact is that the dispatch functions does the equivalent of what the preparation API does internally, and it is safe to use together. What is not safe is to dispatch using the wl_display_dispatch(_queue)() functions while being prepared to read using wl_display_read_events(). This patch rewrites the documentation to correctly state when the various API's are thread safe and how they may not be used. https://bugs.freedesktop.org/show_bug.cgi?id=91767 Signed-off-by: Jonas Ådahl <jadahl@gmail.com> Reviewed-by: Daniel Stone <daniels@collabora.com> |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.