fix: implement UnwindSafe/RefUnwindSafe for wasm-bindgen exports#35
Open
guybedford wants to merge 4 commits into
Open
fix: implement UnwindSafe/RefUnwindSafe for wasm-bindgen exports#35guybedford wants to merge 4 commits into
guybedford wants to merge 4 commits into
Conversation
Newer versions of wasm-bindgen emit type-level `RefUnwindSafe` assertions on `#[wasm_bindgen]` exports under `panic = "unwind"`, because methods on exported types are invoked across a `catch_unwind` boundary. `IntoUnderlyingSource`, `IntoUnderlyingByteSource` and `IntoUnderlyingSink` all hold `Rc<RefCell<Inner>>`, so the assertion fails and downstream crates that depend on wasm-streams fail to build with panic=unwind. The take-and-replace pattern already in place around every fallible await means the inner `Option` is `None` whenever a panic propagates, so no torn intermediate state can leak through the `RefCell` to a later call. That is exactly what the logical `RefUnwindSafe` contract advertises, so add explicit `UnwindSafe` / `RefUnwindSafe` impls for the three exported types.
The previous host `drager.github.io/wasm-pack` started returning 404 after the project moved its docs/installer to `wasm-bindgen.github.io/wasm-pack`, breaking every CI run since 2026-02-08. Update both the wasm-pack installer step and the `fetch_as_stream` integration test (which fetches a PNG asset from the same host) to the new URL.
guybedford
force-pushed
the
fix-unwind-safety-impls
branch
from
8f941d5 to
b47af71
Compare
guybedford
added a commit
to guybedford/gloo
that referenced
this pull request
Apr 28, 2026
wasm-bindgen 0.2.117+ added `MaybeUnwindSafe` bounds on `Closure::wrap` and `Closure::once`, because closures handed to JS are invoked across a `catch_unwind` boundary under `panic = "unwind"`. `gloo-timers` accepts arbitrary user `FnOnce` / `FnMut` callbacks, but its `Closure::wrap(...)` / `Closure::once(...)` calls fail to compile under panic=unwind because the `Box::new(callback) as Box<dyn FnMut()>` coercion (and the `FnOnce` trait selection) erase the static `UnwindSafe` bound that wasm-bindgen requires. This breaks every downstream crate that pulls in `gloo-timers` as a `dev-dependency` and tests under `-Cpanic=unwind` \u2014 see the failure on MattiasBuelens/wasm-streams#35. Fix: - Surface the requirement at the public API: `Timeout::new` / `Interval::new` now require `F: CallbackUnwindSafe`, a marker that resolves to `std::panic::UnwindSafe` under `panic = "unwind"` on wasm and to a no-op blanket otherwise. Callers with non-`UnwindSafe` captures must wrap them in `std::panic::AssertUnwindSafe` at the call site, which is where the invariants can actually be reasoned about. - Internally use `Closure::wrap_assert_unwind_safe` / `Closure::once_assert_unwind_safe` under panic=unwind to acknowledge the dyn-erasure explicitly. The public bound has already enforced the requirement at the call site, so the internal assertion is sound. - Bump the minimum `wasm-bindgen` requirement to `0.2.117` (where the `_assert_unwind_safe` helpers were added). - Add a `panic_unwind_build` CI job that builds `gloo-timers` with `-Cpanic=unwind` so this regression cannot recur silently.
Contributor
Author
|
This still depends on ranile/gloo#562 to pass. |
The unwind-safety bound added in wasm-bindgen makes the published gloo-timers 0.3.0 fail to compile against newer wasm-bindgen. Patch to the gloo fork branch which propagates UnwindSafe through the timer callbacks until ranile/gloo#562 lands and a new gloo-timers is published.
Extract the JsValue before taking the RefCell borrow so a panic during extraction cannot tear the borrow, and wrap the captured Rc in AssertUnwindSafe to satisfy the new wasm-bindgen closure bound.
Madoshakalaka
pushed a commit
to ranile/gloo
that referenced
this pull request
May 16, 2026
…#562) wasm-bindgen 0.2.109+ added a `MaybeUnwindSafe` bound on `Closure::wrap` and `Closure::once` to make panics in JS-invoked closures sound under `panic = "unwind"`. The bound landed quietly because under the default `panic = "abort"` it resolves to a no-op blanket impl, but downstream crates that test under `-Cpanic=unwind` (notably wasm-streams) hit a hard compile error: the `Box<F> as Box<dyn Fn*>` coercion used to construct the closure type erases the static `UnwindSafe` bound the constructors now require. See MattiasBuelens/wasm-streams#35 for the originating report. This change applies the same pattern to every gloo crate that hosts a `Closure::wrap` / `Closure::once` call site: a `CallbackUnwindSafe` marker on the public boundary (where one exists), and an internal switch to the `_assert_unwind_safe` variant under `panic = "unwind"` to acknowledge the dyn-erasure at the call site that has already enforced the bound. gloo-timers * `Timeout::new` and `Interval::new` add a `CallbackUnwindSafe` bound, a marker that resolves to `std::panic::UnwindSafe` under `panic = "unwind"` on wasm and to a no-op blanket otherwise. Internally both route through `Closure::once_assert_unwind_safe` and `Closure::wrap_assert_unwind_safe` under panic=unwind. * The `futures` feature wraps its internal `oneshot::Sender` and `mpsc::UnboundedSender` captures in `std::panic::AssertUnwindSafe`. `TimeoutFuture` consumes the wrapper through a by-value helper so that RFC 2229 disjoint capture sees the closure capturing the wrapper rather than projecting to the inner sender (writing `tx.0` or `let AssertUnwindSafe(x) = tx` inside the closure would silently defeat the assertion). gloo-events * All four `EventListener` constructors (`new`, `once`, `new_with_options`, `once_with_options`) surface `CallbackUnwindSafe` on the user callback and route through the `_assert_unwind_safe` variants under panic=unwind. gloo-render * `request_animation_frame` surfaces the same bound. The internal trampoline that fans the user `FnOnce` out through `Rc<RefCell<Option<CallbackWrapper>>>` is audited unwind-safe: the slot is transitioned to `None` via `borrow_mut().take()` before the user callback runs, so a panic leaves a legitimately reachable post-fire state and `AnimationFrame::drop` correctly skips cancellation. gloo-net * Internal closures only; no public bound. The four WebSocket callbacks and the two EventSource callbacks route through a crate-local `wrap_internal!` macro that resolves to `Closure::wrap_assert_unwind_safe` under panic=unwind. The captures (`Rc<RefCell<Option<Waker>>>` and `mpsc::UnboundedSender<...>`) are exclusively owned by the crate; `unbounded_send` is lock-free and the waker slot is taken in one shot, so a caught panic leaves no observable invariant violated. gloo-worker * The `set_on_packed_message` trampoline is `pub(crate)` and called only from the spawner and registrar. Both call sites either construct a value and forward via `scope.send`, or hold a `RefMut<callbacks>` that is correctly released during unwind via `RefMut::drop`, so asserting unwind safety here is sound. Public-API semver impact is none under the default panic strategy: `CallbackUnwindSafe` blanket-impls every type, so the new bound is invisible to existing callers. Under `panic = "unwind"` the affected crates previously did not compile against wasm-bindgen 0.2.109+, so there are no existing callers to break. The `wasm-bindgen` floor is left at `"0.2"` in every crate; the `_assert_unwind_safe` call sites are cfg-gated on `panic = "unwind"`, and any user opting into that strategy will have already resolved a wasm-bindgen new enough to expose the helpers. CI gains a `panic_unwind_build` job that builds every affected crate with `-Cpanic=unwind -Zbuild-std=std,panic_unwind --all-features` so the same regression cannot reappear silently. The `--all-features` flag covers gloo-timers' `futures` feature, gloo-net's transport features (json, websocket, http, eventsource), and gloo-worker's `futures` feature in a single invocation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Upcoming versions of wasm-bindgen will emit a type-level
RefUnwindSafeassertion on every#[wasm_bindgen]exported method underpanic = "unwind", because the method body is invoked inside acatch_unwindboundary.IntoUnderlyingSource,IntoUnderlyingByteSourceandIntoUnderlyingSinkall holdRc<RefCell<Inner>>, so the assertion fails and downstream crates that depend onwasm-streams(e.g.workers-rs) fail to build withpanic=unwind.The take-and-replace pattern already in place around every fallible await in
Inner::pull/Inner::write/Inner::close/Inner::abortmeans the innerOption<...>is alwaysNonewhenever a panic propagates, so no torn intermediate state can leak through theRefCellto a subsequent call. That is exactly the property theRefUnwindSafecontract advertises, so it is sound to add explicit impls for the three exported types:(and likewise for
IntoUnderlyingByteSourceandIntoUnderlyingSink).The impls are inert on toolchains whose wasm-bindgen does not yet emit the assertion, so this is a purely additive change with no behavioural impact on the existing
panic=unwindtest matrix. The existingtests/tests/panic_unwind.rscontinues to cover the actual unwind behaviour.