fix(wasm): don't kill task on drop #4572
Conversation
|
cc @jplatte |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4572 +/- ##
==========================================
- Coverage 85.39% 85.37% -0.02%
==========================================
Files 286 286
Lines 32229 32235 +6
==========================================
Hits 27522 27522
- Misses 4707 4713 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: Manmeet Singh <[email protected]>
maan2003
force-pushed
the
ma/jj-yxupsopwloqu
branch
from
3ebe4dc to
3818d1c
Compare
| #[cfg(target_arch = "wasm32")] | ||
| impl<T> Drop for JoinHandle<T> { | ||
| fn drop(&mut self) { | ||
| // don't abort the spawned future |
There was a problem hiding this comment.
This comment must be elaborated to explain why we don't abort the future when the join handle is dropped:
- Why?
- Does it match the Tokio's behaviour?
Thanks.
There was a problem hiding this comment.
Yes, it matches tokio's behavior and I'm pretty sure that's exactly the reason for changing the behavior here. I think we discussed it on Matrix previously that a cancel-on-drop JoinHandle might make more sense for the SDK, but then you would have to
- wrap the tokio
JoinHandlefor non-wasm - update all the existing cases where
JoinHandles are dropped prematurely to store the handle somewhere, or call somedetachmethod on it
That's a lot harder. I think you should accept this PR that fixes the existing inconsistency (which leads to buggy behavior), and evaluate the different JoinHandle semantics afterwards.
|
Thanks for your contribution :-). It would be nice if you could address my comment, and if tests could be added. This is an important change. |
|
what do you think about using https://docs.rs/n0-future/latest/n0_future/ instead of custom stuff |
|
Let's go with the current proposal. I'm not in favor of adding more deps, we already have too much of them. |
…kio::spawn` to make it wasm compatible. (#4959) This PR is a revived version of: #4707 since it is now possible to easily add wasm support thanks to: #4572 Using the `executer::spawn` will select `tokio::spawn` or `wasm_bindgen_futures::spawn_local` based on what platform we are on. ~~They behave differently in that `tokio` actually starts the async block inside the spawn but the wasm `spawn` wrapper will only start the part that is not inside the `async` block and the join handle needs to be awaited for it to work.~~ This has now changed with: #4572. Now they behave the same and this PR becomes a very simple change.
makes behavior similar to tokio::spawn otherwise spawned tasks immediately die if join handles are not saved somewhere