Re: race between (say) wl_display_sync and wl_callback_add_listener?
Hi >On Tue, Sep 26, 2023 at 10:08:55AM +0100, John Cox wrote: >> 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? > >From a low-level libwayland API perspective, creating objects or making >requests from another thread is generally fine. It's this particular >case of requests that create object and cause events to be emitted >immediately for them that's problematic. > >Many requests that fall in this category are in fact global binds (e.g., >wl_shm, wl_seat, wl_output), which are typically handled in the context of the >wl_registry.global handler, thus in the dispatch thread, in which case no >special consideration is required. > >There are also some requests that at first glance seem problematic, >but in fact they are not (if things are done properly, and assuming >correct compositor behavior). For example: > > struct wl_callback *wl_surface_frame(struct wl_surface *surface); > >seems suspiciously similar to wl_display_sync(). However, this request >takes effect (i.e., the callback will receive events) only after the next >wl_surface_commit(), so the following is safe: > >cb = wl_surface_frame(surface); >/* As long as the listener is set before the commit we are good. */ >wl_callback_add_listener(cb); >wl_surface_commit(surface); > >Of course, on top of all these there are also the typical higher-level >multithreading synchronization considerations. > > Good - I think my current expectations align with what you and pq have said. So thank you once again for taking the time to help me John Cox
Re: race between (say) wl_display_sync and wl_callback_add_listener?
On Tue, Sep 26, 2023 at 10:08:55AM +0100, John Cox wrote: > 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? >From a low-level libwayland API perspective, creating objects or making requests from another thread is generally fine. It's this particular case of requests that create object and cause events to be emitted immediately for them that's problematic. Many requests that fall in this category are in fact global binds (e.g., wl_shm, wl_seat, wl_output), which are typically handled in the context of the wl_registry.global handler, thus in the dispatch thread, in which case no special consideration is required. There are also some requests that at first glance seem problematic, but in fact they are not (if things are done properly, and assuming correct compositor behavior). For example: struct wl_callback *wl_surface_frame(struct wl_surface *surface); seems suspiciously similar to wl_display_sync(). However, this request takes effect (i.e., the callback will receive events) only after the next wl_surface_commit(), so the following is safe: cb = wl_surface_frame(surface); /* As long as the listener is set before the commit we are good. */ wl_callback_add_listener(cb); wl_surface_commit(surface); Of course, on top of all these there are also the typical higher-level multithreading synchronization considerations. > >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. Buffer reclaiming (if by that you mean handling wl_buffer.release and making the buffer available for drawing again) is a good case for a separate queue, since wl_buffers are "independent" objects, as long as you can dispatch on demand: when you need a new buffer and don't have any you can dispatch the queue to check if the compositor has released one. > 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. I wouldn't mind an official Wayland API/technical FAQ myself :) Thanks, Alexandros
Re: race between (say) wl_display_sync and wl_callback_add_listener?
On Mon, 25 Sep 2023 17:10:30 +0100 John Cox wrote: > 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? > > I assume it must be safe if dispatch & create/add_listener are in the > same thread. > > The same question applies to adding listeners after binding (say) the > shm extension where the bind provokes a set of events giving formats. > > 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) Hi, yeah, this requires some careful handling, because the race is real otherwise. The following applies to the libwayland implementation. The first thing is that you should have a separate wl_event_queue for each "execution context". A thread is definitely an execution context, but not the only possible case. Then, you dispatch that event queue only from the correct execution context. This will serialize your own actions with the dispatch, so when you send a wl_display.sync, even if the compositor already responds with wl_callback.done, it won't get dispatched until you explicitly call dispatch in that thread. wl_display itself counts as one wl_event_queue, the default event queue. wl_event_queues are also a way to filter events without dispatching everything up to the event you wait for. This is the other case of execution context. The disadvantage is that you lose the event ordering between two queues. The second thing is more subtle. wl_display is always on the default event queue. A new protocol object inherits the event queue of the object it is created from. That is, wl_display.sync creates a wl_callback, and the wl_callback starts in the default event queue. This is a problem if you send wl_display.sync from another thread. Your main thread might dispatch the 'done' reply before you can set up the listener or change the queue assignment. This problem is fixed by using wl_proxy_create_wrapper(). See its documentation. Proxy wrappers are used whenever you need to have the new protocol object in a different event queue than the object creating it. You can run the poll/read_events/dispatch_queue loop in any and all execution contexts you like as long as at least one thread is always quick to read the Wayland socket. The reader will direct events to all queues. Thanks, pq pgpMhYVsnH4gl.pgp Description: OpenPGP digital signature
Re: race between (say) wl_display_sync and wl_callback_add_listener?
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
Re: race between (say) wl_display_sync and wl_callback_add_listener?
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
race between (say) wl_display_sync and wl_callback_add_listener?
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? I assume it must be safe if dispatch & create/add_listener are in the same thread. The same question applies to adding listeners after binding (say) the shm extension where the bind provokes a set of events giving formats. 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) Many thanks John Cox
