race between (say) wl_display_sync and wl_callback_add_listener?

[John Cox]

Hi

Many thanks for your comprehensive answer

>On Mon, Sep 25, 2023 at 05:10:30PM +0100, John Cox wrote:
>> Hi
>
>Hi,
>
>> Assuming I have a separate poll/read_events/dispatch_queue thread to my
>> main thread. If I go wl_display_sync/wl_callback_add_listener on the
>> main thread is there a race condition between those two calls where the
>> other thread can read & dispatch the callback before the listener is
>> added or is there some magic s.t. adding the listener will always work
>> as expected?
>
>This is indeed a racy interaction, in more ways than one:
>
>1. There is an inherent data race setting and using the
>   listener/user_data concurrently from different threads.
>   Neither adding a listener/user_data (see wl_proxy_add_listener()),
>   nor the actual dispatching operation in libwayland (which uses the
>   listener, see wayland-client.c:dispatch_event()) is protected by
>   the internal "display lock".

So are all interactions from another thread than the dispatch thread
unsafe e.g. buffer creation, setup & assignment to a surface or is it
just the obviously racy subset?

>2. Even ignoring the above, if the callback done event is received and
>   dispatched before the listener is actually set, the callback will
>   not get called.

That was the race I could see
   
>> I assume it must be safe if dispatch & create/add_listener are in the
>> same thread.
>
>Yes, a single thread guarantees that the listener is properly set before
>any relevant event is dispatched (assuming you don't dispatch events
>before setting the listener!).
>
>> The same question applies to adding listeners after binding (say) the
>> shm extension where the bind provokes a set of events giving formats.
>
>Yes, any object creation that may cause the compositor to send events as
>an immediate response has this problem.
> 
>> Is there a safe way of doing this? (mutex around the dispatch and
>> create/add_listener pairs would seem plausible even if really not what I
>> want to do)
>
>Here are some ideas:
>
>Use a separate queue in the main thread for this event. Special care is
>needed to ensure the event is actually dispatched on that queue. See
>wl_proxy_create_wrapper(), and wl_display_roundtrip_queue() for a
>function that uses a wrapper. Note that using a separate queue may
>cause the event(s) to be handled out of order relative to the events
>in the main queue. This may or may not be a problem depending on the
>specific scenario.

Yup - understood

>Using a mutex as you describe is also a possible solution. The typical
>caveats apply: holding a mutex when calling out to potentially arbitrary
>code (i.e., dispatch handlers) may increase deadlock risk etc.

Thats why I don't want to do that

>Another alternative would be to introduce a mechanism to pause and
>resume the dispatch thread (e.g., wake up the poll with a pipe/eventfd
>and then wait on some condition variable to resume), which would be used
>like:
>
>pause_dispatch_thread();
>... sync and add listener ...
>resume_dispatch_thread();
>
>You could also introduce a mechanism to schedule your own function
>calls in the dispatch thread:
>
>schedule_in_dispatch_thread(sync_and_add_listener, data);

Yup - thats what I'm now doing for the obvious cases

>Finally, you might also want to consider a different design more in line
>with the libwayland API expectations, if possible.

Not really possible - there are some things I need done (buffer reclaim
mostly) async as soon as possible and I don't have control over the main
loop.

There may be a document that sets out everything you've said above and
gives the exact expectations but I failed to find it. In general the
individual call documentation is great but how interactions between
calls are managed is harder to find. I started from an (incorrect)
assumption that everything was fully async and could be called from any
thread (my natural progamming style) and so there must be magic to
enable that and have only slowly been corrected by reality.

Again many thanks

John Cox

>HTH,
>Alexandros