Skip to content

fix: keep ResponseBody's Send impl sound with wasm threads - #120

Open
QuantumExplorer wants to merge 1 commit into
devashishdxt:mainfrom
QuantumExplorer:fix/response-body-send-atomics
Open

QuantumExplorer wants to merge 1 commit into
devashishdxt:mainfrom
QuantumExplorer:fix/response-body-send-atomics

Conversation

@QuantumExplorer

Copy link
Copy Markdown

Fixes #106.

Problem

BodyStream holds JS objects (the fetch body stream, the AbortController and the timeout closure), which belong to the thread that created them. Its unsafe impl Send applies to every target. That is fine on single-threaded wasm, but with wasm threads (target_feature = "atomics") a ResponseBody can be moved to another worker, where polling or dropping it touches JS handles that belong to another thread.

Fix

With target_feature = "atomics", BodyStream records the thread that created it, panics if it is polled on another thread, and leaks its JS objects if it is dropped on another thread. Without atomics nothing changes: the new code is all behind cfg.

The Send and Sync impls stay, with a comment on why they hold. tonic needs Send, and Sync was already sound, because &BodyStream gives no access to the JS objects. Removing Send under atomics would also be sound, but builds with wasm threads would then stop compiling, since tonic would reject ResponseBody.

Before and after

Build and use Before After
wasm32 without threads Send, sound (one thread) unchanged
wasm32 with atomics, body polled on the thread that created it works works
wasm32 with atomics, body polled on another thread uses another thread's JS handles panics: "a ResponseBody must be polled on the thread that created it"
wasm32 with atomics, body dropped on another thread uses another thread's JS handles the JS objects are leaked

Tests

There is no automated test, since exercising this needs a multi-threaded wasm app. I checked that the crate builds and passes clippy both ways: as usual, and with RUSTFLAGS="-C target-feature=+atomics,+bulk-memory" cargo +nightly clippy --target wasm32-unknown-unknown -Z build-std=panic_abort,std.

cargo fmt --check, cargo clippy --all-targets -- -D warnings, cargo clippy --target wasm32-unknown-unknown -- -D warnings and cargo test are clean, and the browser suite in test-suite/simple passes in headless Chrome 153.

This is one of 14 PRs from the review asked for in #97. They are independent, and all 14 also merge together cleanly; with all of them merged, both browser suites (test-suite/simple and test-suite/gzip) pass.

🤖 Generated with Claude Code

BodyStream holds JS objects (the fetch body stream, the AbortController
and a timeout closure), which belong to the thread that created them.
Its unconditional unsafe impl Send is fine on single-threaded wasm, but
with wasm threads (target_feature = "atomics") a ResponseBody could be
moved to another worker, where polling or dropping it touches JS handles
from the wrong thread.

With atomics, BodyStream now records its thread, panics if it is polled
on another one, and leaks the JS objects if it is dropped on another
one. Without atomics nothing changes. Send stays, since tonic requires
it; Sync is sound as is, because &BodyStream gives no access to the JS
objects.

Fixes devashishdxt#106

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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

Successfully merging this pull request may close these issues.

ResponseBody's unsafe impl Send is unsound with wasm threads (target_feature = "atomics")

1 participant