Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,43 @@ jobs:
wasm-pack test --node crates/$x --no-default-features
done

panic_unwind_build:
name: Build with -Cpanic=unwind
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v6

# Recent Rust nightlies with LLVM 22 are temporarily broken with
# `panic=unwind` (see wasm-bindgen/wasm-bindgen#4929). Pin to a known-good
# nightly to avoid flapping.
- uses: dtolnay/rust-toolchain@master
with:
toolchain: nightly-2026-01-28
target: wasm32-unknown-unknown
components: rust-src
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there an issue for this? An unblocking condition for future maintainers would be good


- uses: actions/cache@v4
with:
path: |
~/.cargo/registry
~/.cargo/git
target
key: cargo-${{ runner.os }}-panic-unwind-${{ hashFiles('**/Cargo.toml') }}
restore-keys: |
cargo-${{ runner.os }}-panic-unwind-
cargo-${{ runner.os }}-

# Verify gloo-timers (and any other crate that hosts user closures via
# `Closure::wrap` / `Closure::once`) still builds cleanly under
# `panic = "unwind"` against the new `MaybeUnwindSafe` bounds added in
# wasm-bindgen 0.2.117+. Catches downstream regressions like
# https://github.com/MattiasBuelens/wasm-streams/pull/35 from creeping in.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and any other crate

doesn't this step only test the gloo-timers crate?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I've gone and expanded this to all the crates.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated: the CI step now builds every crate that hosts wasm-bindgen closures (gloo-timers default + futures, gloo-events, gloo-render, gloo-net, gloo-worker) under panic=unwind, and the misleading parenthetical is gone. Also fixed all four sibling crates as part of this PR — see the new commit fix(events,render,net,worker).

- name: Build crates with panic=unwind
env:
RUSTFLAGS: '-Cpanic=unwind'
run: |
cargo build -p gloo-timers --target wasm32-unknown-unknown -Zbuild-std=std,panic_unwind

test-history-wasi:
name: Test gloo-history WASI
runs-on: ubuntu-latest
Expand Down
2 changes: 1 addition & 1 deletion crates/timers/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ rust-version = { workspace = true }
features = ["futures"]

[dependencies]
wasm-bindgen = "0.2"
wasm-bindgen = "0.2.117"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasm-bindgen/src/closure.rs: wrap_assert_unwind_safe landed in 0.2.109, once_assert_unwind_safe in 0.2.110. The actual floor needed is 0.2.110

But I don't see the problem of being a bit more forward compatible.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have guarded all unwind checks with the panic=unwind cfg, so that we don't need to bump the version constraint for non-panic-unwind consumers. Panic unwind consumers then only need to use > 0.2.110, which fits the usage model.

js-sys = { workspace = true }
futures-core = { version = "0.3", optional = true }
futures-channel = { version = "0.3", optional = true }
Expand Down
49 changes: 47 additions & 2 deletions crates/timers/src/callback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,32 @@ use js_sys::Function;
use wasm_bindgen::prelude::*;
use wasm_bindgen::{JsCast, JsValue};

/// Bound carrying the unwind-safety requirement on [`Timeout::new`] /
/// [`Interval::new`] callbacks.
///
/// Under `panic = "unwind"` on wasm the callback is invoked across a
/// `catch_unwind` boundary inside `wasm_bindgen`, so this resolves to
/// [`std::panic::UnwindSafe`]. Under any other panic strategy it is a no-op
/// blanket. Wrap non-`UnwindSafe` captures in [`std::panic::AssertUnwindSafe`]
/// at the call site.
#[cfg(all(target_arch = "wasm32", panic = "unwind"))]
pub trait CallbackUnwindSafe: std::panic::UnwindSafe {}
#[cfg(all(target_arch = "wasm32", panic = "unwind"))]
impl<T: std::panic::UnwindSafe> CallbackUnwindSafe for T {}

/// Bound carrying the unwind-safety requirement on [`Timeout::new`] /
/// [`Interval::new`] callbacks.
///
/// Under `panic = "unwind"` on wasm the callback is invoked across a
/// `catch_unwind` boundary inside `wasm_bindgen`, so this resolves to
/// [`std::panic::UnwindSafe`]. Under any other panic strategy it is a no-op
/// blanket. Wrap non-`UnwindSafe` captures in [`std::panic::AssertUnwindSafe`]
/// at the call site.
Comment thread
guybedford marked this conversation as resolved.
Outdated
#[cfg(not(all(target_arch = "wasm32", panic = "unwind")))]
pub trait CallbackUnwindSafe {}
#[cfg(not(all(target_arch = "wasm32", panic = "unwind")))]
impl<T> CallbackUnwindSafe for T {}

#[wasm_bindgen]
unsafe extern "C" {
#[wasm_bindgen(js_name = "setTimeout", catch)]
Expand Down Expand Up @@ -57,8 +83,18 @@ impl Timeout {
/// ```
pub fn new<F>(millis: u32, callback: F) -> Timeout
where
F: 'static + FnOnce(),
F: 'static + FnOnce() + CallbackUnwindSafe,
{
// Under `panic = "unwind"` we use the `_assert_unwind_safe` variant
// because `WasmClosureFnOnce` selection erases `F` into a trait-object
// dispatch that no longer carries the static `UnwindSafe` bound — the
// same dyn-erasure problem wasm-bindgen handles internally. The
// `CallbackUnwindSafe` bound on the public API has already enforced
// the requirement at the call site. On any other panic strategy
// `Closure::once` is unchanged and works on any 0.2.x wasm-bindgen.
#[cfg(all(target_arch = "wasm32", panic = "unwind"))]
let closure = Closure::once_assert_unwind_safe(callback);
#[cfg(not(all(target_arch = "wasm32", panic = "unwind")))]
let closure = Closure::once(callback);

let id = set_timeout(
Expand Down Expand Up @@ -158,8 +194,17 @@ impl Interval {
/// ```
pub fn new<F>(millis: u32, callback: F) -> Interval
where
F: 'static + FnMut(),
F: 'static + FnMut() + CallbackUnwindSafe,
{
// Same rationale as `Timeout::new`: the `Box<F> as Box<dyn FnMut()>`
// coercion erases the `UnwindSafe` bound, so under `panic = "unwind"`
// we use `_assert_unwind_safe` to acknowledge the erasure (the public
// `CallbackUnwindSafe` bound has already enforced unwind safety at
// the call site). Otherwise the original `Closure::wrap` path is
// preserved for older wasm-bindgen compatibility.
#[cfg(all(target_arch = "wasm32", panic = "unwind"))]
let closure = Closure::wrap_assert_unwind_safe(Box::new(callback) as Box<dyn FnMut()>);
#[cfg(not(all(target_arch = "wasm32", panic = "unwind")))]
let closure = Closure::wrap(Box::new(callback) as Box<dyn FnMut()>);

let id = set_interval(
Expand Down
Loading