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

Use once_cell instead of async-lock #73

Closed
wants to merge 2 commits into from

Conversation

Bluefinger
Copy link

@Bluefinger Bluefinger commented Nov 6, 2023

Basic fix for getting more recent versions of Executor to compile on WASM is to avoid async-lock as that no longer will work for the OnceCell usage within wasm32 contexts. Unfortunately, adding the test suites for WASM requires async-io to support web timers, so perhaps that needs to be fixed in async-io first before adding additional CI checks here for async-executor.

Fixes #72

@notgull
Copy link
Member

notgull commented Nov 6, 2023

I don't want to take once_cell as a dependency. They've caused problems in the past by bumping their MSRV past our defined limit. Not to mention, we can't really block in web environments. A better solution would be to conditionally replace the get_or_init_blocking() with poll_once(get_or_init()).unwrap() on WASM.

But I don't find this to be a compelling solution either. In all honestly, I thought that this crate didn't compile on WASM so I didn't really put any effort into WASM support. I thought that crates that did async on the web just used the executor from wasm-bindgen-futures.

To clarify, the reference executor contained in this crate is not designed for the web. The local queue structure translates very poorly to web targets; as most contemporary web browsers already include an executor, so it's emulating a queue on top of an existing queue. I'm shocked that it works at all, and flabbergasted that this apparently has worked for Bevy up until this point. I guess it goes to show how well async code can compose with just about anything, even for use cases that it doesn't really support.

A better solution would be similar to what wasm-bindgen-futures does. We should create Promises based on the Tasks and let the browser's event loop take care of it for us. I'll make a PR for this soon-ish.

@Bluefinger
Copy link
Author

The cases in bevy are that it still pulls in LocalExecutor for the web case, and exposes some methods that end up touching that executor, but the main spawn/spawn_local paths go via wasm-bindgn-futures. What probably needs to happen is better dep gating for wasm for bevy, but there's still some features/methods that touch on LocalExecutor that probably need to be rethought for the wasm use-case.

In any case, I'll probably close this PR and continue the conversation on the relevant issue/following PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

v1.7 no longer compiles for wasm32
2 participants