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

On web, allow recreating event loop when using spawn #2720

Closed
wants to merge 3 commits into from

Conversation

grovesNL
Copy link
Contributor

@grovesNL grovesNL commented Mar 6, 2023

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

As described in #2344 (comment), it's useful to be able to recreate event loops on the web when using spawn, since the wasm module remains loaded. e.g., switching between tabs on a single page app and recreating a canvas doesn't currently work without some kind of workaround like this.

This won't change the behavior of run, so that will still panic when attempting to create multiple event loops.

I know there are probably still closures leaked when using this approach but don't expect this to be an issue in practice. We can also deal with any leaked closures later. At least anecdotally I've been using this approach for a while now and haven't run into any problems.

@grovesNL
Copy link
Contributor Author

grovesNL commented Mar 6, 2023

cc @kchibisov this is the approach you mentioned on #2344 (comment)

Copy link
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

I think that's not exactly what I've suggested.

What I meant was more like .with_allow_multiple_loops(true) in the new EventLoopBuilderExtWeb.

Can't you create multiple event loops at once on the web and then use spawn? If you can't and you must use spawn before creating the second loop, then it should be static in a comment.

You should also update the docs for the spawn that it allows multiple event loop creation.

Though, I guess you can't have 2 loops before spawn, since you need to consume the loop?

@grovesNL
Copy link
Contributor Author

I'd be happy to change the approach here but I'm not sure I follow the approach you described.

In case the use case isn't clear, my use case works like this:

  • the wasm module is loaded
  • spawn/the event loop is created once and sets the static flag
  • some action in the application causes the event loop and canvas are destroyed – basically all state is destroyed except for statics because they're shared at the wasm module level
  • some action in the application triggers a new spawn/event loop creation, which fails because of the static flag

The flow works the same way as run/spawn normally would, except I need to clear the static flag that's set at the wasm module level because it prevents run/spawn from being called again. I'm not trying to run multiple event loops simultaneously or similar.

@kchibisov
Copy link
Member

No, what I mean is could you do something like that on wasm

let evl1 = EventLoop::new();
let evl2 = EventLoop::new();
// And spawn later on.

Right now you won't be able to do so, because you clear only on spawn.

What I suggested is that once you build you pass inside the builder a flag that you must not restrict additional creation, so instead of clearing on spawn you won't set the static global at all.

@grovesNL
Copy link
Contributor Author

Sure, we could do that instead. I don't have a strong preference though it feels like we should just handle it internally instead of having it be opt-in on the builder. Is it valid to recreate the event loop multiple times without consuming it in run/spawn?

@kchibisov
Copy link
Member

Is it valid to recreate the event loop multiple times without consuming it in run/spawn?

Not on any other platform, buy maybe you can do that on Wasm? That's what I'm asking you, if you can create multiple events loop at the same time on wasm, we should allow that with the extension to the builder, on any other platform it's impossible.

@daxpedda
Copy link
Member

That's what I'm asking you, if you can create multiple events loop at the same time on wasm, we should allow that with the extension to the builder

I don't think this is possible in Winit, there are event handlers registered on the window itself, like the scale change observer, which would be overwritten by a new event loop.

It's possible to augment these event handlers to handle multiple event loops, but I don't think that's the goal here.

src/event_loop.rs Outdated Show resolved Hide resolved
src/event_loop.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/platform_impl/web/event_loop/mod.rs Show resolved Hide resolved
@kchibisov
Copy link
Member

I don't think this is possible in Winit, there are event handlers registered on the window itself, like the scale change observer, which would be overwritten by a new event loop.

Thanks, that's what I wanted to know.

@notgull
Copy link
Member

notgull commented Mar 13, 2023

Wouldn't it be possible to avoid resetting the event handlers by using addEventListener instead of setting them?

@daxpedda
Copy link
Member

I didn't know this, I thought addEventListener would overwrite, just tried it to confirm. I guess it's possible then.

Just checked our code, we aren't using any setters as far as I can see. Also noticed that we are using the deprecated MediaQueryList.addListener(), but that should still work like addEventListener.

@grovesNL
Copy link
Contributor Author

Thank you for the review! I think everything should be addressed now.

I don't think it's worth trying to support multiple event loops right now (at least to solve this immediate issue) since it opens up a lot of other questions about how we might want to extend support for the web in general. For example:

  • Should we allow multiple windows (canvases) each having their own event loop or should they share one event loop?
  • Are we confident that all of the event handlers/CSS queries are set up correctly to support multiple event loops instead of setting a single event loop as the listener?
  • How do multiple event loops interact with web workers and offscreen canvases?

I'd be happy to consider ways we could extend this later on, but the current change feels like a simple enough that we could land initially.

Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

LGTM.

I think we shouldn't try to support multiple event loops until we know what a use-case for this even looks like, in any case I agree with @grovesNL this is out-of-scope for this PR.

@kchibisov kchibisov added this to the Version 0.29.0 milestone Mar 16, 2023
@daxpedda daxpedda added DS - web C - waiting on author Waiting for a response or another PR labels May 28, 2023
@daxpedda daxpedda self-assigned this May 28, 2023
@daxpedda
Copy link
Member

@grovesNL I'm happy to merge this after a rebase.

@daxpedda
Copy link
Member

I'm gonna close this as it's rather old now and needs a rebase.
Feel free to reopen after a rebase.

In general this issue is tracked in #2885.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - waiting on author Waiting for a response or another PR DS - web
Development

Successfully merging this pull request may close these issues.

4 participants