smol-rs / async-channel

Async multi-producer multi-consumer channel
Apache License 2.0
796 stars 42 forks source link

Allow blocking channels in wasm #77

Open elias098 opened 11 months ago

elias098 commented 11 months ago

The blocking methods on channels got restricted to non wasm and while its a bad idea to use it on the main thread, it can be quite useful when you want to run blocking code on a web worker.

my current use is with https://crates.io/crates/wasm_thread

notgull commented 11 months ago

My impression is that blocking in a web worker is undefined and will immediately lead to a panic. So it doesn't make much sense to have blocking channels if it would immediately kill the thread, unless you have some other synchronization mechanism. But if you have that other synchronization mechanism, why use channels in the first place?

@daxpedda I know there was discussion of using these channels in winit, would it make sense to enable blocking on the web, even if it panics? I'm not familiar enough with web platforms to make the call

elias098 commented 11 months ago

in my experience it hasnt panicked yet, and web workers are allowed to run blocking code, it even got synchronous file readers.

the reason i need it to be blocking is because a library uses the read trait which afaik must be blocking.

im not sure if it changes anything with it but i do have atomics enabled to allow workers to share the same memory as the main thread.

daxpedda commented 11 months ago

Blocking in Web workers is fine, it's just not allowed in the main thread (the Window context).

If blocking in the main thread is considered UB is a complicated question. Firstly, it's not defined in the Wasm spec yet what should happen if blocking is not allowed (see https://github.com/WebAssembly/threads/issues/174), so right now it is actually some kind of UB in the literal sense.

But even if it were well defined, e.g. a trap is triggered when blocking is not allowed, it might still be UB from Rusts perspective. I will assume that Rust makes some assumptions on blocking that there won't simply be reentrancy in the same thread, so that sounds pretty UB to me unless Rust can detect that blocking failed, which it currently definitely can't. But I don't know the details on what kind of assumptions Rust makes and what it does with these assumptions.

In any case, without digging more into this and finding a general solution, the easiest way for async-channel is probably to detect if you are in the main thread (the Window context) and panic there instead. But I believe the general request of allowing blocking for use in Web workers is valid. I don't think we need it in Winit though, it's still a question if we will use async-channel at all there.

im not sure if it changes anything with it but i do have atomics enabled to allow workers to share the same memory as the main thread.

I'm not aware of any Wasm instruction that allows you to block without the atomics proposal. I assume any other implementation out there just uses a loop or does it through the Atomics JS API. Rust std just panics when blocking isn't available (locks API selection -> non-atomic locks API implementation), but has no detection for the main thread, because Rust has to support pure Wasm and not just the browser.

daxpedda commented 11 months ago

In any case, without digging more into this and finding a general solution, the easiest way for async-channel is probably to detect if you are in the main thread (the Window context) and panic there instead.

On another thought, this will only work in browsers. If async-channel wants to offer general Wasm support, you will have to guard the check behind a crate feature.

Additionally, for non-browser support, it can be guarded behind target_feature = "atomics" and for browser support as well unless you want to go through JS Atomics.

notgull commented 11 months ago

I can see this being used as a solution. However, it would require implementing support for it first in parking and then in event-listener. See https://github.com/smol-rs/parking/issues/20 for that issue.