Bug 91767 - wl_display_dispatch(_queue) documentation about thread safeness is probably wrong
Summary: wl_display_dispatch(_queue) documentation about thread safeness is probably w...
Status: RESOLVED FIXED
Alias: None
Product: Wayland
Classification: Unclassified
Component: wayland (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Wayland bug list
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 83430 91769
  Show dependency treegraph
 
Reported: 2015-08-27 02:32 UTC by Jonas Ådahl
Modified: 2016-02-02 02:12 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments

Description Jonas Ådahl 2015-08-27 02:32:36 UTC
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.
Comment 1 Pekka Paalanen 2015-09-01 08:40:24 UTC
Adding Marek to CC, since IIRC we worked on the docs, but I have happily forgot all the details by now.
Comment 2 Marek Chalupa 2015-09-03 08:22:01 UTC
(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.
Comment 3 Marek Chalupa 2015-09-03 08:28:46 UTC
> 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
Comment 4 Jonas Ådahl 2015-09-03 08:42:26 UTC
(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.
Comment 5 Jonas Ådahl 2016-02-02 02:12:07 UTC
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.