race between (say) wl_display_sync and wl_callback_add_listener?

[Alexandros Frantzis]

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".

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.
   
> 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.

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.

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);

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

HTH,
Alexandros