search

Smithay / calloop

A callback-based Event Loop
MIT License
176 stars 34 forks source link

Allow event source to mark itself as being ready from the `pre_run` or similar #127

Closed kchibisov closed 12 months ago

kchibisov commented 1 year ago

Some event sources could be read be read outside of the calloop reading.

An example of such source could be a WaylandSource. The wayland display could be read by other libraries, like mesa, to check that you have events, the wl_display_prepare_read is used. Such issue created a workaround inside the winit library.

One possible solution would be to add a way for event source to set the indicator about instant wake ups, so calloop when trying to poll on them again will do instant wakeup instead and call the event/read part.

cc @rib

rib commented 1 year ago

Yeah, so after I had initially convinced myself that calloop was using "prepare_read" and therefore we don't need that workaround in Winit - I think now that calloop isn't currently following the standard protocol that's required to allow multiple threads to safely access a shared Wayland socket.

E.g. here's a C reference for how the prepare_read protocol can be followed to safely support multi-threaded access without deadlocks: https://wayland.freedesktop.org/docs/html/apb.html#Client-classwl__display_1a40039c1169b153269a3dc0796a54ddb0

Also here's a reference for implementing a similar kind of source for the glib main loop: https://gitlab.gnome.org/GNOME/gtk/-/blob/main/gdk/wayland/gdkeventsource.c#L63

It looks like the logic has changed between smithay-client-toolkit-0.16 and master but in either case it doesn't seem to follow the ordering of the reference C code linked above.

elinorbgr commented 1 year ago

calloop in itself does nothing about Wayland, so if there is a bug in the use of prepare_read in the adapter, this should be discussed in the issue tracker of wayland-client instead.

Furthermore, I'm sorry but I cannot guess what is being done wrong in that logic from your comment, so a more precise explanation would be helpful.

elinorbgr commented 1 year ago

Coming back to this, I believe the proper way to fix this is to replace the pre_run API with something with the following semantics:

This should allow to fix the Wayland issues, which I believe come from the fact that we should not keep the source in a "preparing to read events" state while we are doing something else than sleeping on the socket (for example while we are running user callbacks).

kchibisov commented 1 year ago

A method that says to the event source "I'm going to sleep", and from with the source can answer "actually I'm already ready"

Yeah, I've tried adding something like that with the return to pre_run and then |= based on that, the thing is though, that the user callback must be dispatched consistently, so they know that they have events actually in their queue.

A method that notifies the source that we are done sleeping (be it to process the events, or returning from the event loop), and this method should also tell the source whether it has been woken up or not, so that it can do some preprocessing if necessary (but not invoke user code from it).

I'm not sure what this suppose to do? Is it a post_run basically?

This should allow to fix the Wayland issues, which I believe come from the fact that we should not keep the source in a "preparing to read events" state while we are doing something else than sleeping on the socket (for example while we are running user callbacks).

Are you talking about the fact that we hold a read guard across the different trait method invocations? Because I think we release it before calling back to user (though, I'm likely wrong).

elinorbgr commented 12 months ago

Done in #144