[WASM] remove MaybeSend, ensure 'timeout' is Send#362
Conversation
| "rt", | ||
| "time", | ||
| ] } | ||
| wasm-bindgen-futures = "0.4.50" |
There was a problem hiding this comment.
Can we just use tokio_with_wasm? It's got futures under the hood to make it work with wasm as well using promises. The less we've got to bundle in wasm, the better 🙏.
There was a problem hiding this comment.
tokio_with_wasm already uses wasm-bindgen-futures so this doesn't add any dependency, it just exposes it for direct usage by ractor
tokio_with_wasm::spawn_local calls exactly wasm-bindgen-futures::spawn_local, so, I can replace that, but also, it literally doesn't change anything w.r.t. binary size or else
There was a problem hiding this comment.
Correct, that's why was thinking, should we use the tokio with wasm one 🤔? At least one less dependency for cargo to process, and for the ractor to maintain.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #362 +/- ##
==========================================
+ Coverage 82.28% 82.44% +0.15%
==========================================
Files 70 71 +1
Lines 12765 12891 +126
==========================================
+ Hits 10504 10628 +124
- Misses 2261 2263 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
So just to be clear, this is going to require that no one accidently use a timeout implementation that's not strictly I think that's fine for WASM environments, especially if we keep iterating on it, but I don't specifically work much in WASM, so are you sure that timeout is the only limitation on making the bulk of WASM futures Send safe? |
|
Unfortunately it's not just timeout. Anything that directly touches a JS If you have just a few interactions with JS, and the rest is pure rust, then you can wrap a non-Send async into a Send async (as long as the return value itself is Send). Detailsfn wrap_future_as_send<F, T>(f: F) -> impl Future<Output = Result<T, RecvError>> + Send
where
F: Future<Output = T> + 'static,
T: Send + 'static,
{
let (tx, rx) = crate::concurrency::oneshot();
// The spawn_local ensures that the non-Send async stays on exactly this thread, and the return value will be dispatched into the Send async.
tokio_with_wasm::spawn_local(async move {
let result = f.await;
let _ = tx.send(result); // note: failures here are ignored, the most likely reason would be a dropped receiver
});
async { rx.await }
}That's what I did to wrap That's fine for like an egui web app that maybe uses a websocket for communication, but not really reasonable for something that heavily interacts with JS. But I still want to emphasize that WASM is not categorically different from native! Even though non-Send'ness might be a little bit more pervasive here than there, WASM supports multithreading (native wasm threads or JS browser web-workers), making Send relevant, and non-Send-ness is also potentially an issue in Desktop, just less common. The solution to all this might be as simple as porting If you have an actor that interacts with JS, great, just use a That's what I'll be looking at soon (I'm away on the weekend, so likely some time next week), and why this PR is still on Draft. I just wanted to jump into the conversation as early as possible, because I believe it was going the wrong direction with making everything non-Send by default, and which would inhibit code-sharing between WASM and native. |
|
To elaborate on In rust, you can not infer whether an async fn foo(input: TIn) -> TOut {
bar().await
}
This also means that if you change In the case of ractor, In WASM, This is different when using the proc-macro
|
|
This is more or less ready. (I'll check over it once more then rebase it.) I've had to copy and modify the 'tokio_with_wasm::time' implementation to make it properly Send. It's out of scope for this PR, but it would be great to formalize the backend system. I renamed As the name suggests, this only works with wasm in the browser, but would NOT work with wasm in the cloud. Unfortunately this is something that needs to be implemented in ractor directly (at least for now) and can not be implemented by the user. Add to this the possibility of adding a native backend based on smol, to replace the obsolete |
e14ac30 to
710115f
Compare
|
hmm looks like some of the workflows need fixing :/ Can you address when you have some time? |
|
@slawlor can you please restart CI? |
44d0b81 to
5ff3234
Compare
|
i'm trying to figure out how to auto-approve CI for your updates, so you aren't blocked on my seeing the notifications. apologies for the delays! |
|
I can run CI by opening a dummy PR on my own fork: 0x53A#1 with the exception of code coverage, which fails because of a missing token, everything is now green
|
Remove MaybeSend, all functions take and return Send futures just like native. Enable 'ThreadLocalActor' on WASM. The WASM concurrency backend was partially copied from tokio_with_wasm and adapted to ensure Send'ness. Note: ./ractor/src/macros/tests.rs was copied from slawlor#363
9962146 to
4d02ece
Compare
| let (tx, rx) = crate::concurrency::oneshot(); | ||
| tokio_with_wasm::spawn_local(async move { | ||
| let result = f.await; | ||
| let _ = tx.send(result); // note: failures here are ignored, the most likely reason would be a dropped receiver |
There was a problem hiding this comment.
I guess for WASM there's no real risk of panic! as today, all panic's abort the process, correct? Like we don't need to specifically try and catch the panic
There was a problem hiding this comment.
the value ignored by let _ is a Result, so I don't think it would panic at all - and I also believe ignoring the error is more or less correct since dropping a future (which drops the receiver) is allowed.
Regarding panics, today, wasm is panic=abort, but they are hard at work implementing panic=unwind (tracking issue), so "soon" wasm will work just like native.

I wanted to open this PR mainly to push additional fuel into the flames of the discussion in #361.
Specifically, I believe this makes the whole MaybeSync machinery obsolete so that it will be easier to share code between WASM and non-WASM.
I have changed the wasm tests to run twice - once without, and once with the feature
async-trait. This ensures that both configurations are valid in WASM.I have added a unit test that demonstrates something that failed before, that is, invoking
call!from inside an actor (see #361 (comment)).First I added the unit test, which failed in my CI (https://github.com/0x53A/ractor/actions/runs/15676933563/job/44159323493) with
error: future cannot be sent between threads safely.Then I applied the changes, which made
timeoutSend, which fixed the failing test (https://github.com/0x53A/ractor/actions/runs/15679737485).The implementation inside
tokio_with_wasm_primitives.rsis way too complex - I just copy-pasted a bunch of code to make it work. I'll clean that up.Next step would be to also reenable other code that's currently disabled for wasm and align it with non-wasm, for example the
thread_localmodule withThreadLocalActor, I see no reason why this shouldn't be able to run on WASM.