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

Document that Context can't be locked on Wasm #2812

Open
daxpedda opened this issue Mar 15, 2023 · 4 comments
Open

Document that Context can't be locked on Wasm #2812

daxpedda opened this issue Mar 15, 2023 · 4 comments

Comments

@daxpedda
Copy link
Contributor

daxpedda commented Mar 15, 2023

I recently adjusted a cross-platform game engine that uses Egui to support multi-threading on Wasm and noticed that it will panic almost immediately with "already mutably borrowed" which I knew about but was wondering why it worked just fine on native targets.

After some digging I found out that the issue is that on native targets it's using RwLock but on Wasm it's using atomic_refcell, which was kinda fine because multi-threaded Wasm is available on nightly only and barely anybody is using it (I assume). But obviously this means that it can't wait for a non-existent lock to release, it will just panic.

To make Egui usable with Wasm on the main thread (the document), there is no real fix, as waiting is not possible there, see #1401.

I was actually surprised that my implementation was locking in the first place, nothing wrong with Egui there, it's documented, I just didn't notice. So I'm kind of a big proponent of #1399 now.

In the meantime I think it could be useful to mention the difference between native and Wasm somewhere. Happy to make a PR, just really don't know exactly where to put that.

@daxpedda daxpedda changed the title Document that Context can't be shared between threads on Wasm Document that Context can't be locked on Wasm Mar 15, 2023
@Gentoli
Copy link

Gentoli commented Apr 1, 2023

Looks like if you unpin rust toolchain and build the demo with wgpu instead of glow, it will also hit this issue:

    at core::panicking::panic_fmt::h8069c8a5e0a61f4b (wasm://wasm/01bd19c6:wasm-function[6542]:0x3fd0e3)
    at eframe::web::backend::AppRunnerContainer::add_event_listener::{{closure}}::h6c7db9a9fa0e0ae3 (wasm://wasm/01bd19c6:wasm-function[1763]:0x2ae2db)
    at <dyn core::ops::function::FnMut<(A,)>+Output = R as wasm_bindgen::closure::WasmClosure>::describe::invoke::h08179ee5559e3c97 (wasm://wasm/01bd19c6:wasm-function[7317]:0x40574c)

Build using glow works fine using nightly.
versions: egui eb0812a9530f77712bf77aad0956f932a3a8f23d, rust cargo 1.70.0-nightly (145219a9f 2023-03-27)

@daxpedda
Copy link
Contributor Author

daxpedda commented Apr 1, 2023

This doesn't look like the same issue, the blocking error the browser throws doesn't actually go through Rusts panic handler.

@emilk
Copy link
Owner

emilk commented Apr 20, 2023

When I wrote crates/epaint/src/mutex.rs I assumed wasm32 = singlethreaded. It should be pretty easy to either change crates/epaint/src/mutex.rs to always use parking_lot and never AtomicRefcell, or to add a feature flag

@daxpedda
Copy link
Contributor Author

We could indeed just use parking_lot if target_feature = "atomics": https://github.com/Amanieu/parking_lot/blob/746df3ebe4bbcde4f9a9a7ea8e78845b5575adb5/core/src/thread_parker/mod.rs#L69-L75.

But it would still need to be documented that you can only share it between workers on Wasm, because you can't lock in the main thread. I don't think this is solvable unless Context somehow becomes lock-free.

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

No branches or pull requests

3 participants