-
Notifications
You must be signed in to change notification settings - Fork 879
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
Return an error when Windows event loop is busy #3687
base: master
Are you sure you want to change the base?
Return an error when Windows event loop is busy #3687
Conversation
This was a regression introduced in rust-windowing#3418 This new error could be used to retry events at later time This also removes `EventLoopClosed` struct and replaces it with `EventLoopProxyError` enum
Hmm, I'm unsure how you are expected to handle this as a user? The event is still in the queue, we can't just try to handle it later? Also, this somewhat conflicts with #3424, I would really like the wakeup mechanism that we use to not be able to fail. |
I initially started this PR with an older commit without noticing, it used to mess up the order of events, but no I am having a hard time reproducing it, will convert this to a draft until I can reproduce. (sorry for then noise)
yeah that would be great but |
I have added an example showcasing the problem, basically even though the events are not lost and are sitting in the queue, we need to send empty wakeup events so they can be dispatched again. Without this PR, we can't know if The example tries to send |
Quickly reading through the changes in #3694, it still exhibits the same problem, but it is a lot better since it won't consume the data being sent.
Not sure what you mean by "do it in the next iteration", do you mean call I don't think that helps much, since more than one wakeup events can fail consecutively. I could probably change my logic to use |
I meant to, if This would work under the assumption that the reason why |
But why not just return an error if it failed, ofc I am talking in the context of #3694 where the proxy just calls |
Hmm, mostly because it's unclear how the user is expected to handle that error? Like, they can't just retry, waking up, as that'll likely just give them the same error. Especially so if there is a solution for us to handle this fully internally in Winit, then I'd rather not push the error case onto users. |
Yeah that is expected, and in that case we have a logic to try which re-tries until succession let mut res = wakeup_eventloop();
while (res.is_err()) {
sleep(15ms);
res = wakeup_eventloop();
} This is a very basic solution and maybe sufficient along with a receiver implementation that uses
I don't see how Winit could handle this case tbh, especially since winit will no longer take ownership of the data, and if wakeup failed, user can't know if wakeup failed or which one failed and can't implement logic to gracefully handle that. |
We haven't finalized this yet, but I think we're going to decide that multiple wakeup calls don't necessarily lead to multiple wakeups; i.e. they may be merged.
I'll try to whip up another PR for it once #3694 is done (especially if reminded), so let's discuss it at that time. |
That should still be fine for our use-case.
Thanks, I will keep an eye for it |
I very much agree with this. |
This was a regression introduced in #3418
This new error could be used to retry events at later time
This also removes
EventLoopClosed
structand replaces it with
EventLoopProxyError
enumchangelog
module if knowledge of this change could be valuable to users