Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EventLoopProxy::waker(self) -> Waker #3424

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add EventLoopProxy::waker(self) -> Waker #3424

wants to merge 1 commit into from

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Jan 25, 2024

As part of #3367, we're considering removing the generic on Event<T>, EventLoop<T>, EventLoopProxy<T> and the callback. By extension, we'd be getting rid of Event::UserEvent(T).

One approach to doing so would be to allow converting EventLoopProxy to the standard library's Waker type, which allows the user to wake a task (in this case the event loop) from any thread; then they can implement what they need themselves on top of that, using the standard library's mpsc::channel, mpsc::sync_channel or likewise, depending on their needs.

This is likely not a perfect solution for wakeups, we still need to properly figure out how to do timers; but this can get us some of the way there.

Blocked on #3449, we need to ensure that Sync is actually sound.

CC @notgull, I think this may be slightly useful for async?

Questions:

  • Do we need a UserWoken event, or is NewEvents is enough?
  • Should we hide the Waker behind a custom type like EventLoopWaker, in case we want to expose further functionality in the future?
    • We keep EventLoopProxy, and allow that to be converted to Waker.
  • Is Waker too heavy-weight for this? Can we just use a function wake(&self) on EventLoopProxy? Or maybe both?
  • Does the user need the ability to handle the case where the event loop has been destroyed? Or is that handled fine enough through other means, like in mpsc?

TODO:

  • Finish implementation on all platforms.
  • Properly test Waker implementations with updated example.
  • Add an entry to CHANGELOG.md.
  • Update documentation.

@kchibisov

This comment was marked as resolved.

@madsmtm madsmtm changed the title Add EventLoop::waker(&self) -> Waker Add EventLoopProxy::waker(self) -> Waker Jan 25, 2024
@madsmtm madsmtm force-pushed the waker branch 2 times, most recently from 78cd1d5 to 28f339f Compare January 25, 2024 14:46
src/platform_impl/windows/event_loop.rs Show resolved Hide resolved
}
});

event_loop.run(move |event, elwt| match event {
Event::UserEvent(event) => println!("user event: {event:?}"),
Event::NewEvents(_) => {
for event in receiver.iter() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocking inside NewEvents feels not great, I would suggest try_iter here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to have separate event for all of that tbf, so you kind of know that you woke up due to your event.

Copy link
Member Author

@madsmtm madsmtm Jan 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest try_iter here

That was an oversight, I've changed it, thanks!

I'd prefer to have separate event for all of that tbf, so you kind of know that you woke up due to your event.

I think a UserWoken event is a bad idea, for that to be useful we'd have to dispatch it multiple times, once per invocation of wake.

This would mean that a valid chain of events is:

NewEvents(WaitCancelled { .. })
UserWoken
UserWoken
UserWoken
AboutToWait

Which seems counter-intuitive.

I'd be more inclined to adding it as something like StartCause::UserWoken, but that has its own set of problems.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, you invoke it only once, so you don't always poll your events if you don't have anything. Doing in init forces to do the checking every time, doing in separate event is the better way to put it.

The wakeup must be single per event loop. The WakeUp itself must be squashed. Basically how it's done in winit-next.

The point of being useful is that you could have complex sources of events and some may require locking to poll, so having a dedicated event sounds better, it also separates the logic and makes it more clear.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing in init forces to do the checking every time

Fair point.

My primary fear is that I don't want people to do:

Event::UserWoken => {
    let event = receiver.try_recv().unwrap();
    // Handle event
}

As they may then miss events, and I think that is more easily avoided if the wakeup is bundled into NewEvents.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what events are they going to miss. The order is NewEvents, and then Wakeup event, they are explicitly aware of such thing. The thing is that it also doesn't really matter, since they can poll in NewEvents and Wakeup event, because NewEvents and Wakeup are coming together, it's just a way to optimize/clean things a bit, given that winit already handled the part that you have new event.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want three calls to waker.wake_by_ref() to be guaranteed to result in three Wakeup/UserWoken events, but I fear the user might assume that's how it works, and design their code around that assumption.

One solution to this would be to fold the event into NewEvents, as I'm proposing. Another is better documentation.

it also doesn't really matter

No this is definitely nitpicking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, wakeup must be squashed, no matter how much you call wake_by_ref you must always get exactly one event. We already have the same behavior with the RedrawRequested, since no matter how much you call it, you'll get exactly one.

Copy link
Contributor

@dhardy dhardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I support this change: it makes winit simpler (at least at the API level).

Also, even though I currently use UserEvent, half the reason for that is just to create a Waker over it.

Comment on lines +494 to 500
// Note: This does not have the generic from `EventLoopProxy<T>`, since this
// is the new API that shouldn't need it.
impl From<EventLoopProxy> for Waker {
fn from(proxy: EventLoopProxy) -> Self {
proxy.waker()
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is wanted — eventually EventLoopProxy will (presumably) just be replaced with a Waker.

In fact, maybe just deprecate EventLoopProxy and create_proxy now, replacing with EventLoop::create_waker?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to reuse proxy in the future for some internal stuff in the future around Send/Sync and also, not be limited on the Waker API, since it's rather limited. But yeah, in the current state they are mostly the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - needs discussion Direction must be ironed out S - api Design and usability
Development

Successfully merging this pull request may close these issues.

4 participants