event_loop: remove generic user event #3694

kchibisov · 2024-05-10T17:41:49Z

Let the users wake up the event loop and then they could poll their
user sources.

--

I haven't tested these changes outside of Wayland. Please carefully review your backend changes.

I know that Windows and Web are likely done not correctly.

On macOS/ios the atomic ordering is likely not correct either, but I'm not familiar enough with the CFRunLoop behavior to ensure that.

There's also #3424 , but the proxy should be platform specific and in the end just a trait. Maybe we'll use Waker, but for now I went with just removing generic which prevents me from using dyn more actively.

This PR is build on top of #3683

Let the users wake up the event loop and then they could poll their user sources.

madsmtm

I've reviewed the macOS, iOS and Windows implementations now to the best of my ability.

src/changelog/unreleased.md

madsmtm · 2024-05-20T17:13:15Z

src/application.rs

@@ -82,11 +82,11 @@ pub trait ApplicationHandler<T: 'static = ()> {
    /// [`Suspended`]: Self::suspended
    fn resumed(&mut self, event_loop: &ActiveEventLoop);

-    /// Emitted when an event is sent from [`EventLoopProxy::send_event`].
+    /// Called when user requested wake up via [`wake_up`] occurs.


Continuing from #3424 (comment).

I'd like to discuss the semantics of this. Here's what I think the semantics should be:

It is guaranteed to happen between new_events and about_to_wait (like all the other events).

It will not be emitted the same number of times as the user called wake_up.

This should be documented thoroughly, ideally with an example on the method using a std::sync::mpsc::channel which loops over the received events (instead of doing let event = receiver.try_recv().unwrap();).

The order in which this is emitted is not guaranteed (i.e. it may be emitted right after new_events, or it may be emitted just before about_to_wait, or any time in between those).

The time at which this will be emitted is not guaranteed (only that it will happen "soon", i.e. there may be several executions of the event loop, including multiple redraws to windows, between wake_up being called and the event being delivered).

The event may be emitted spuriously.

The event may be ignored spuriously (see Return an error when Windows event loop is busy #3687).

This seems like it'd make it too unreliable to be of any real use, though unsure if we can do it any other way? Maybe on Windows, we can keep a wake_up_failed: Arc<AtomicBool> around, and then do it in the next iteration if the boolean is set?

In any case, we can make no guarantee in this PR, and then in a follow-up, make the change and add the guarantee!

I've deliberately chosen the most conservative semantics here, if we find that users need stronger guarantees, we can always add them. Please raise your concerns with any of these!

Also, we should document all of this in detail once we agree.

That's a lot of words for something that should behave more or less like RedrawRequested.

I'd say that the:

Order is not defined, but generally should be before RedrawRequsted.

Events are squashed, as request_redraw.

Events being lost or busy can not really happen due to semantics of how it's written or should be written, if it does it's a bug.

The event may be ignored spuriously (see Return an error when Windows event loop is busy #3687).

This seems like it'd make it too unreliable to be of any real use, though unsure if we can do it any other way? Maybe on Windows, we can keep a wake_up_failed: Arc<AtomicBool> around, and then do it in the next iteration if the boolean is set?

In any case, we can make no guarantee in this PR, and then in a follow-up, make the change and add the guarantee!

I think without a guarantee that it will actually wake up it is indeed not very valuable, so I would prefer if we get everything in order right here.

I very much agree with all the other semantics.

I'll outline that the semantics must be:

It's delivered only once between NewEvents and AboutToWait, no matter how much you ask for it. That's how e.g. eventfd works, for example(and that's what we use on linux here).

It can not fail (when the loop is running at least), winit must ensure that you get a wakeup.

Now I also think that we should communicate errors just in case for forward compat/backward, since it's a good way to detect whether the loop is still alive from auxiliary threads.

At least error should have the option of LoopLost or something like that.

So as for the current implementation you already can see that everything is following 1, but as for the 2, there's no guarantee for that, so maybe adding an error for now is a good way forward with that.

src/changelog/unreleased.md

examples/child_window.rs

src/lib.rs

src/changelog/unreleased.md

madsmtm · 2024-05-20T17:39:24Z

src/platform_impl/windows/event_loop.rs

@@ -2454,7 +2392,7 @@ unsafe extern "system" fn thread_event_target_callback(
            // user event is still in the mpsc channel and will be pulled
            // once the placeholder event is delivered to the wrapper
            // `event_handler`


Nit: Update comment.

I think it's relatively accurate given that we still do roundtrips with channel, because I'm afraid to change that to use something else, because I can not really test this one.

src/platform_impl/macos/event_handler.rs

madsmtm · 2024-05-20T18:10:54Z

src/platform_impl/macos/event_loop.rs

-    pub fn send_event(&self, event: T) -> Result<(), EventLoopClosed<T>> {
-        self.sender.send(event).map_err(|mpsc::SendError(x)| EventLoopClosed(x))?;
+    pub fn wake_up(&self) {
+        self.user_wake_up.store(true, AtomicOrdering::Relaxed);


I don't think there is a correct ordering to use here, technically the CPU could choose to execute all the code below, and wake up the main thread to process the event, before actually setting this flag (SeqCst is not strong enough to do this either, as it'd have to have been used in CFRunLoopWakeUp as well, which I'm not sure it has).

The proper way to do this would be to emit the wake-up event (or just set a flag) inside the event_loop_proxy_handler function above (which runs on the main thread). That's a bit of a larger rework though, the current impl will work fine for now (and the old code had the same problem), I'll change it myself at a later point.

The only thing I care about is that the event is emitted only once and the wake up is not lost. Though, I think more intense sync should be on the user here...

src/event_loop.rs

src/lib.rs

tests/sync_object.rs

daxpedda · 2024-05-20T19:37:42Z

Web should be good now.

Apart from us agreeing on the details and adding some documentation I'm very much in favor of moving forward with this.

notgull

ACK to X11

event_loop: remove generic user event

d5fdf2b

Let the users wake up the event loop and then they could poll their user sources.

kchibisov force-pushed the kchibisov/remove-user-event branch from 5244f86 to d5fdf2b Compare May 20, 2024 16:37

madsmtm reviewed May 20, 2024

View reviewed changes

madsmtm mentioned this pull request May 20, 2024

Return an error when Windows event loop is busy #3687

Open

5 tasks

daxpedda reviewed May 20, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

tests/sync_object.rs Outdated Show resolved Hide resolved

daxpedda force-pushed the kchibisov/remove-user-event branch from b3386f4 to 11bf122 Compare May 20, 2024 19:36

daxpedda force-pushed the kchibisov/remove-user-event branch from 11bf122 to 175e0ba Compare May 20, 2024 19:38

web: squash UserWakeUp events

c3a9c58

daxpedda force-pushed the kchibisov/remove-user-event branch from 175e0ba to 4e8bbc1 Compare May 20, 2024 19:44

ci: fix remaining issues

4ac84e5

daxpedda force-pushed the kchibisov/remove-user-event branch from 4e8bbc1 to 4ac84e5 Compare May 20, 2024 19:48

madsmtm added S - api Design and usability S - maintenance Repaying technical debt I - BREAKING CHANGE labels May 20, 2024

notgull approved these changes May 21, 2024

View reviewed changes

kchibisov force-pushed the kchibisov/remove-user-event branch from ac357ed to aa5b67b Compare May 21, 2024 11:42

kchibisov marked this pull request as ready for review May 21, 2024 11:43

kchibisov requested review from jackpot51, msiglreith and MarijnS95 as code owners May 21, 2024 11:43

docs: apply suggestions

b142035

kchibisov force-pushed the kchibisov/remove-user-event branch from aa5b67b to b142035 Compare May 21, 2024 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

event_loop: remove generic user event #3694

event_loop: remove generic user event #3694

kchibisov commented May 10, 2024

madsmtm left a comment

madsmtm May 20, 2024

kchibisov May 20, 2024

daxpedda May 20, 2024 •

edited

kchibisov May 21, 2024 •

edited

madsmtm May 20, 2024

kchibisov May 21, 2024

madsmtm May 20, 2024

kchibisov May 21, 2024

daxpedda commented May 20, 2024

notgull left a comment

event_loop: remove generic user event #3694

Are you sure you want to change the base?

event_loop: remove generic user event #3694

Conversation

kchibisov commented May 10, 2024

madsmtm left a comment

Choose a reason for hiding this comment

madsmtm May 20, 2024

Choose a reason for hiding this comment

kchibisov May 20, 2024

Choose a reason for hiding this comment

daxpedda May 20, 2024 • edited

Choose a reason for hiding this comment

kchibisov May 21, 2024 • edited

Choose a reason for hiding this comment

madsmtm May 20, 2024

Choose a reason for hiding this comment

kchibisov May 21, 2024

Choose a reason for hiding this comment

madsmtm May 20, 2024

Choose a reason for hiding this comment

kchibisov May 21, 2024

Choose a reason for hiding this comment

daxpedda commented May 20, 2024

notgull left a comment

Choose a reason for hiding this comment

daxpedda May 20, 2024 •

edited

kchibisov May 21, 2024 •

edited