On Fri Jun 2 15:38:27 2023 +0000, Alexandros Frantzis wrote:
TL;DR: Although I would also prefer to avoid any additional complexity, I haven't been able to find a better approach, given the constraints of the libwayland API and the impedance mismatch between the win32/wayland multi-threading dispatch models. Due to the way multi-threading support works in libwayland, there are cases related to object destruction, like this one (see commit message for more details), where we need to ensure that any event handler currently executing will be finished before it's safe to continue 100% race-free (this is trivially true in single-threaded applications). Let me know if you would like me to go into more detail about this issue (we will need to dive into libwayland dispatch internals). The high-level blocking API `wl_display_dispatch_queue` we used before doesn't provide any (direct, but see alternative below) mechanism to enforce this guarantee, so the currently proposed approach is to use the lower-level libwayland APIs that allow us to exercise more control on when events are dispatched (or not dispatched, in this case) in a thread. The proposed mechanism might seem complex and arcane (and, to be fair, it is), but it is a standard libwayland idiom recommended by the documentation (see https://gitlab.freedesktop.org/wayland/wayland/-/blob/main/src/wayland-clien...) to enable a dispatch thread to handle multiple event sources. In fact, the `wl_display_dispatch_queue` function we used before uses this exact prepare/poll/read dance internally (https://gitlab.freedesktop.org/wayland/wayland/-/blob/main/src/wayland-clien...), but only reads from one source, the compositor fd. Our custom dispatch function just extends this mechanism so that we poll for an additional fd (the callback pipe), to allow us to ensure that after calling our sync function, all previous events dispatches have finished. I have experimented with several other approaches and the only one I have found that is somewhat simpler in terms of client code while still being correct/race-free is: https://gitlab.winehq.org/afrantzis/wine/-/commit/9af3e50c856432922dce5c9567.... However, despite the apparent simplicity there is a lot of complexity involved in proving the correctness of this alternative. I have been in contact with libwayland upstream about this, and it was not a trivial exercise for us to be fully convinced that this is correct, involving digging into the libwayland internals and nuances. It's also less performant since it involves a possible roundtrip to the compositor. For these reasons, and, FWIW, the upstream recommendation was against this approach, and rather strongly in favor of the battle-tested prepare/poll/read dance.
and a lot like the event dispatch mechanism that was not well received
in the previous MR Although some parts of the mechanism are similar (e.g., using pipe fds for thread notification/wakeup), this is distinctly different in that it doesn't interact at all with wine event/message dispatch, it's a just a way to interject callbacks into the internal Wayland event dispatch thread.
Okay, I will need some time to look and think about that, I don't really have a strong opinion anyway but at the same time, like for the previous MR, I'm afraid that my eventual approval won't be enough.