feat(fetch): honor AbortSignal — cancel/timeout in-flight requests#5690
Conversation
📝 WalkthroughWalkthroughFetchWithOptions now carries an optional ChangesAbortSignal fetch plumbing
Sequence Diagram(s)sequenceDiagram
participant global_this_fetch_thunk
participant js_fetch_set_pending_signal
participant js_fetch_with_options
participant take_pending_signal_watch
participant js_fetch_notify_signal_aborted
participant FetchAbortWatch
participant queue_deferred_resolution
global_this_fetch_thunk->>js_fetch_set_pending_signal: store init.signal
js_fetch_with_options->>take_pending_signal_watch: consume pending signal
take_pending_signal_watch-->>js_fetch_with_options: SignalState::Watch or SignalState::AlreadyAborted
alt signal already aborted
js_fetch_with_options->>queue_deferred_resolution: reject with AbortError
else live watch
js_fetch_notify_signal_aborted->>FetchAbortWatch: notify abort listeners
FetchAbortWatch-->>js_fetch_with_options: wake aborted()
js_fetch_with_options->>queue_deferred_resolution: reject with AbortError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-codegen-wasm/src/emit/js_fallback.rs (1)
351-385: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winForward
signalinto the emitted JS fetch options.Line 357 binds the new field but this fallback still drops it, so
fetch(url, { signal })compiled through the JS fallback can no longer be aborted or timed out even though this path emits a native JSfetch(...).Suggested fix
Expr::FetchWithOptions { url, method, body, headers, headers_dynamic, - signal: _, + signal, } => { let url_js = self.emit_js_expr(url, locals); let method_js = self.emit_js_expr(method, locals); let body_js = self.emit_js_expr(body, locals); // In async JS context, we can do a real fetch let mut opts = format!("{{ method: getString({}) || 'GET'", method_js); if !matches!(body.as_ref(), Expr::Undefined) { opts.push_str(&format!(", body: getString({})", body_js)); } if let Some(hexpr) = headers_dynamic { // Dynamically-built headers: pass the JS object through so // fetch enumerates every property (`#4932`). let h = self.emit_js_expr(hexpr, locals); opts.push_str(&format!(", headers: toJsValue({})", h)); } else if !headers.is_empty() { opts.push_str(", headers: {"); for (i, (key, val)) in headers.iter().enumerate() { if i > 0 { opts.push_str(", "); } let v = self.emit_js_expr(val, locals); opts.push_str(&format!("'{}': getString({})", key, v)); } opts.push('}'); } + if let Some(sigexpr) = signal { + let sig = self.emit_js_expr(sigexpr, locals); + opts.push_str(&format!(", signal: toJsValue({})", sig)); + } opts.push('}'); format!("fromJsValue(await fetch(getString({}), {}))", url_js, opts) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-codegen-wasm/src/emit/js_fallback.rs` around lines 351 - 385, The JS fallback for Expr::FetchWithOptions is dropping the signal field, so aborted or timed-out requests cannot be canceled. Update the FetchWithOptions branch in js_fallback.rs to emit the signal option alongside method, body, and headers when present, using the existing signal binding in the same way as the other forwarded fetch options. Keep the change localized to the emit path that builds the fetch options string so native JS fetch behavior is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-stdlib/src/fetch/abort_bridge.rs`:
- Around line 106-109: The FetchAbortWatch cleanup currently only unregisters
the Rust Notify via Drop/unwatch, but it leaves the JS abort listener attached
to the signal. Update the abort bridge flow around FetchAbortWatch and the abort
listener registration path so that the native listener is also removed when the
watch ends, either by scheduling removal on the main thread or by switching to
one shared listener per signal. Make sure both the Drop cleanup and the
corresponding listener setup/teardown code paths are updated consistently in the
abort_bridge logic.
In `@crates/perry-stdlib/src/fetch/mod.rs`:
- Around line 614-625: The pending AbortSignal is still being consumed after
js_promise_new() in the fetch path, which can leave it vulnerable to GC before
js_abort_signal_resolve_ptr() uses it. Update the fetch flow around
take_pending_signal_watch() so the TLS-stashed signal is consumed first, then
allocate the promise, and only after that branch on SignalState::AlreadyAborted
/ Watch / None in the fetch thunk logic.
---
Outside diff comments:
In `@crates/perry-codegen-wasm/src/emit/js_fallback.rs`:
- Around line 351-385: The JS fallback for Expr::FetchWithOptions is dropping
the signal field, so aborted or timed-out requests cannot be canceled. Update
the FetchWithOptions branch in js_fallback.rs to emit the signal option
alongside method, body, and headers when present, using the existing signal
binding in the same way as the other forwarded fetch options. Keep the change
localized to the emit path that builds the fetch options string so native JS
fetch behavior is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 75af683a-174b-45a3-8e67-c0679a8d59cd
📒 Files selected for processing (20)
crates/perry-codegen-js/src/emit/exprs_more.rscrates/perry-codegen-wasm/src/emit/expr/net_fetch_crypto.rscrates/perry-codegen-wasm/src/emit/js_fallback.rscrates/perry-codegen-wasm/src/emit/string_collection.rscrates/perry-codegen/src/collectors/escape_check.rscrates/perry-codegen/src/collectors/escape_news.rscrates/perry-codegen/src/expr/logical_collections.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi/language_core.rscrates/perry-hir/src/capability.rscrates/perry-hir/src/egress.rscrates/perry-hir/src/ir/expr.rscrates/perry-hir/src/lower/expr_call/globals.rscrates/perry-hir/src/stable_hash/expr.rscrates/perry-hir/src/walker/expr_mut.rscrates/perry-hir/src/walker/expr_ref.rscrates/perry-runtime/src/object/global_fetch.rscrates/perry-runtime/src/url/abort.rscrates/perry-runtime/src/url/mod.rscrates/perry-stdlib/src/fetch/abort_bridge.rscrates/perry-stdlib/src/fetch/mod.rs
`fetch(url, { signal })` ignored the signal: `js_fetch_with_options` took only
url/method/body/headers and ran to completion, so neither `controller.abort()`
nor `AbortSignal.timeout(ms)` could cancel it. And `AbortSignal.timeout(ms)` was
a no-op stub returning a never-aborting signal. Together,
`fetch(url, { signal: AbortSignal.timeout(ms) })` on a slow/held response hung
forever instead of rejecting at the deadline.
This wires AbortSignal through the fetch path:
- `AbortSignal.timeout(ms)` (`url/abort.rs`) now schedules a real (unref'd)
callback timer that aborts the signal with a `TimeoutError` and fires its
`abort` listeners when the deadline elapses on the main thread.
- The signal reaches the stdlib fetch via a thread-local stash
(`js_fetch_set_pending_signal`), keeping the 4-arg ABI unchanged. The runtime
fetch thunk sets it for the dynamic/aliased path; codegen emits it before the
call for the `fetch(url, { … })` fast path (a new `signal` field on the
`FetchWithOptions` HIR node).
- `js_fetch_with_options` registers a per-request `tokio::sync::Notify` keyed to
the signal and `select!`s the request against it; on abort it drops the
request future (cancelling the in-flight reqwest request) and rejects with an
`AbortError`. An already-aborted signal rejects up front. The abort reaches
the request through the runtime: `fire_abort_listeners` calls
`js_fetch_notify_signal_aborted` on every abort — so there is no per-fetch JS
listener to accumulate on reused signals.
The pending signal is consumed before the promise is allocated (so a GC during
allocation can't move the TLS-stashed signal). No behavior change for
signal-less fetches; the WASM fetch backend keeps its existing behavior.
proggeramlug
force-pushed
the
fetch-abortsignal
branch
from
4714a92 to
b9fba08
Compare
|
Thanks for the review — addressed both findings plus the file-size lint: 🔴 Consume the pending signal before allocating the promise — done; 🟠 Per-fetch abort listener never removed — reworked to avoid per-fetch listeners entirely. Instead of appending a JS Lint (2000-line file gate) — moved the request dispatch into Re-validated the reworked design with a native repro: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/url/abort.rs`:
- Around line 128-133: The fetch-abort watcher is keyed off the raw AbortSignal
object address, which can change after GC and cause notify_fetch_abort to miss
the registered watcher. Update the AbortSignal abort path in abort.rs so the
watch/notify flow uses a GC-stable signal id or handle stored on the signal
itself, and make the related watcher registry share that stable identifier
instead of casting the signal pointer to i64. Apply the same fix anywhere else
in this flow that registers or notifies abort watchers, including the
timeout/controller.abort path referenced in the same code.
In `@crates/perry-stdlib/src/fetch/abort_bridge.rs`:
- Around line 237-242: The Err(e) branch in abort_bridge’s fetch response path
is creating a JS Error too early by calling fetch_error_bits inside the spawned
request future. Change this path to mirror the abort rejection flow: have the
future return the raw err_msg via spawn_for_promise_deferred(), then perform the
JSValue/Error conversion on the main thread before queue_promise_resolution is
called. Keep the fix localized to the fetch error handling in abort_bridge and
preserve the existing deferred promise resolution structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1e6fdc58-7264-426d-b7e1-97c1a1f3d673
📒 Files selected for processing (20)
crates/perry-codegen-js/src/emit/exprs_more.rscrates/perry-codegen-wasm/src/emit/expr/net_fetch_crypto.rscrates/perry-codegen-wasm/src/emit/js_fallback.rscrates/perry-codegen-wasm/src/emit/string_collection.rscrates/perry-codegen/src/collectors/escape_check.rscrates/perry-codegen/src/collectors/escape_news.rscrates/perry-codegen/src/expr/logical_collections.rscrates/perry-codegen/src/runtime_decls/stdlib_ffi/language_core.rscrates/perry-hir/src/capability.rscrates/perry-hir/src/egress.rscrates/perry-hir/src/ir/expr.rscrates/perry-hir/src/lower/expr_call/globals.rscrates/perry-hir/src/stable_hash/expr.rscrates/perry-hir/src/walker/expr_mut.rscrates/perry-hir/src/walker/expr_ref.rscrates/perry-runtime/src/object/global_fetch.rscrates/perry-runtime/src/url/abort.rscrates/perry-runtime/src/url/mod.rscrates/perry-stdlib/src/fetch/abort_bridge.rscrates/perry-stdlib/src/fetch/mod.rs
✅ Files skipped from review due to trivial changes (3)
- crates/perry-codegen-wasm/src/emit/js_fallback.rs
- crates/perry-codegen-wasm/src/emit/expr/net_fetch_crypto.rs
- crates/perry-runtime/src/url/mod.rs
🚧 Files skipped from review as they are similar to previous changes (14)
- crates/perry-codegen/src/collectors/escape_news.rs
- crates/perry-codegen-js/src/emit/exprs_more.rs
- crates/perry-hir/src/ir/expr.rs
- crates/perry-hir/src/walker/expr_mut.rs
- crates/perry-codegen/src/runtime_decls/stdlib_ffi/language_core.rs
- crates/perry-hir/src/stable_hash/expr.rs
- crates/perry-hir/src/egress.rs
- crates/perry-hir/src/walker/expr_ref.rs
- crates/perry-hir/src/lower/expr_call/globals.rs
- crates/perry-codegen/src/collectors/escape_check.rs
- crates/perry-hir/src/capability.rs
- crates/perry-codegen/src/expr/logical_collections.rs
- crates/perry-codegen-wasm/src/emit/string_collection.rs
- crates/perry-runtime/src/object/global_fetch.rs
| // Wake any in-flight `fetch` bound to this signal so it rejects with an | ||
| // AbortError. Done here — before the JS-listener early return below — so the | ||
| // fetch is cancelled even when the signal carries no JS `abort` listener | ||
| // (the common `fetch(url, { signal })` case registers none). This replaces a | ||
| // per-fetch JS listener, so reused signals don't accumulate stale closures. | ||
| notify_fetch_abort(signal as i64); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Don’t key fetch abort watches by the raw AbortSignal address.
notify_fetch_abort(signal as i64) passes the current ObjectHeader address into the stdlib watcher registry. That registry stores the address after this stack frame is gone; if GC moves the signal before controller.abort() or the timeout fires, the later notification uses the new address and misses the stale watcher, leaving the fetch uncancelled. Use a GC-stable signal id/handle stored on the signal and shared by watch/notify instead of the object address. Based on learnings, raw Rust pointer locals are conservative roots only while directly reachable on the stack; pointers reachable only through heap/global slots need explicit rooting or a stable handle.
Also applies to: 216-222
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/url/abort.rs` around lines 128 - 133, The
fetch-abort watcher is keyed off the raw AbortSignal object address, which can
change after GC and cause notify_fetch_abort to miss the registered watcher.
Update the AbortSignal abort path in abort.rs so the watch/notify flow uses a
GC-stable signal id or handle stored on the signal itself, and make the related
watcher registry share that stable identifier instead of casting the signal
pointer to i64. Apply the same fix anywhere else in this flow that registers or
notifies abort watchers, including the timeout/controller.abort path referenced
in the same code.
Source: Learnings
| Err(e) => { | ||
| let err_msg = format!("Fetch error: {}", e); | ||
| // SAFETY: `fetch_error_bits` allocates an Error JSValue; this is | ||
| // the worker-side error path it replaces verbatim. | ||
| let err_bits = unsafe { super::fetch_error_bits(&err_msg) }; | ||
| queue_promise_resolution(promise_ptr, false, err_bits); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Defer fetch error JSValue creation back to the main thread.
This error branch runs inside the spawned request future, but fetch_error_bits allocates a JS Error. Route the raw err_msg through the deferred/main-thread conversion path instead, matching the abort rejection path and avoiding worker-thread JS heap access. As per coding guidelines, use spawn_for_promise_deferred() to return raw Rust data from async operations, then convert to JSValue on the main thread to avoid thread-local arena issues.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-stdlib/src/fetch/abort_bridge.rs` around lines 237 - 242, The
Err(e) branch in abort_bridge’s fetch response path is creating a JS Error too
early by calling fetch_error_bits inside the spawned request future. Change this
path to mirror the abort rejection flow: have the future return the raw err_msg
via spawn_for_promise_deferred(), then perform the JSValue/Error conversion on
the main thread before queue_promise_resolution is called. Keep the fix
localized to the fetch error handling in abort_bridge and preserve the existing
deferred promise resolution structure.
Source: Coding guidelines
|
Pushed one more fix found while verifying against the real bundle: reject the aborted fetch with the signal's
Confirmed with a repro mimicking that retry pattern: before, the loop spun (130k+ retries); after, it gives up after 1 try with |
1 participant
Problem
fetch(url, { signal })ignored the AbortSignal.js_fetch_with_optionstook onlyurl/method/body/headersand ran the request to completion, so neithercontroller.abort()norAbortSignal.timeout(ms)could cancel an in-flight request. Separately,AbortSignal.timeout(ms)was a no-op stub that returned a signal which never aborts.Together, the common pattern
would hang forever on a slow or unresponsive endpoint instead of rejecting at the deadline. A minimal repro before this change:
AbortSignal.timeout(2000)on a 10s-delay request resolved after the full 10s (the timeout was ignored).Fix
Wire AbortSignal through the whole fetch path:
AbortSignal.timeout(ms)(url/abort.rs) now schedules a real,unref'd callback timer. When the deadline elapses on the main thread (viajs_callback_timer_tick), it aborts the signal with aTimeoutErrorDOMException and fires itsabortlisteners.js_fetch_set_pending_signal/js_fetch_take_pending_signal), which keeps the existing 4-argjs_fetch_with_optionsABI unchanged. The runtimefetchthunk sets it for the dynamic/aliased path; codegen emits it just before the call for thefetch(url, { … })fast path (a newsignalfield threaded through theFetchWithOptionsHIR node).js_fetch_with_optionsconsumes the signal on the main thread, registers a nativeabortlistener that wakes a per-requesttokio::sync::Notify, andselect!s the request against the abort. On abort it drops the request future (cancelling the in-flightreqwestrequest) and rejects the promise with anAbortError. An already-aborted signal rejects up front. Bothcontroller.abort()and theAbortSignal.timeoutdeadline flow through the same listener.Threading is safe: registration runs on the main thread; the worker only awaits the
Notifyand rejects throughqueue_deferred_resolution, whose converter builds the error on the main thread (never touching the JS heap from a worker).Testing
A repro covering both call paths (compiled native):
AbortSignal.timeout(300)standalone.aborted === trueafter the deadline ✓fetch(url, { signal: AbortSignal.timeout(1000) })(direct + aliased)AbortErrorat ~1000ms ✓fetch(url, { signal: controller.signal })+controller.abort()after 1sAbortErrorat ~1000ms ✓fetch(url)(no signal)cargo test -p perry-runtime -p perry-codegen— green (the only failures are pre-existing test-parallelism flakiness in unrelatedclosure::dynamic_props/object::tests, which pass single-threaded).Summary by CodeRabbit
signaloption, includingAbortSignal.timeout()auto-abort and abort-listener behavior.fetchoperations can now be cancelled usingAbortController/AbortSignal.signalfield across string collection, escape checking, walkers, and stable hashing.