fix(runtime): AbortSignal parity — accept in events helpers + dynamic method dispatch#6046
Conversation
… method dispatch Node treats `AbortSignal` as an `EventTarget`; Perry represents it as its own native object (url/abort.rs) that two dispatch layers didn't know: 1. The module-level `node:events` helpers. `setMaxListeners(n, controller.signal)` — how SDKs raise the MaxListenersExceededWarning threshold on a shared signal — threw `ERR_INVALID_ARG_TYPE: The "eventTargets" argument must be an instance of EventEmitter or EventTarget`, rejecting the caller's whole request path. The four helpers (`setMaxListeners`, `getMaxListeners`, `listenerCount`, `getEventListeners`) now recognize signals in both perry-stdlib and perry-ext-events: set is a faithful no-op (the warning threshold is the call's only Node-observable effect and Perry never emits that warning for signals), get reports the EventTarget default, and the listener queries report the signal's registered "abort" listeners through two new runtime helpers (`js_abort_signal_listener_count`, `js_abort_signal_listeners_copy`). 2. Dynamic receivers. The statically-typed form (`c.signal.addEventListener(...)`) lowers to the native call, but the same method through an untyped local — the shape minified SDK code takes — resolved to `undefined` and threw `addEventListener is not a function` (the #5964 URLSearchParams dynamic-dispatch class). Both dynamic paths are covered: property GET returns a bound-method closure (`abort_signal_method_bind` in get_field_by_name_tail), and the fused dynamic method CALL pre-dispatches to the natives in js_native_call_method (the #5961 precedent) for `addEventListener` / `removeEventListener` / `throwIfAborted`. `options` is accepted and ignored — a signal only ever fires "abort" once, so `{ once: true }` is behaviorally implied. Two e2e tests cover the helpers surface and the dynamic-receiver forms (including listeners actually firing on abort and plain objects still being rejected).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds AbortSignal support to Node-compatible ChangesAbortSignal Events Integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant JSCode
participant Runtime
participant EventsHelpers
participant abort.rs as abort.rs
JSCode->>Runtime: signal.addEventListener(...)
Runtime->>abort.rs: abort_signal_method_bind / listener thunk
abort.rs-->>Runtime: bound method or dispatch result
JSCode->>EventsHelpers: listenerCount(signal, "abort")
EventsHelpers->>abort.rs: js_abort_signal_listener_count(signal)
abort.rs-->>EventsHelpers: count
EventsHelpers-->>JSCode: count
JSCode->>EventsHelpers: getEventListeners(signal, "abort")
EventsHelpers->>abort.rs: js_abort_signal_listeners_copy(signal)
abort.rs-->>EventsHelpers: copied array
EventsHelpers-->>JSCode: listeners array
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/perry-ext-events/src/lib.rs (1)
1855-1864: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffAbortSignal special-casing logic is duplicated verbatim in
perry-stdlib/src/events/module_helpers.rs.Same detection-and-dispatch logic (resolve ptr, check
"abort"event name, return listeners/count, no-op on setMaxListeners) appears in both crates. This mirrors an existing pre-PR pattern for other abort helpers in this file, so it's consistent with the codebase, but it means any future change to signal semantics (e.g., honoring per-signal max-listener overrides) needs to be kept in sync in two places.Consider extracting the shared "resolve + dispatch" logic into a single helper in
perry-runtimethat both crates can call, to avoid drift.Also applies to: 1895-1902, 1925-1930, 1956-1967
🤖 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-ext-events/src/lib.rs` around lines 1855 - 1864, The AbortSignal handling is duplicated between this module and the matching helpers in perry-stdlib, so update the shared “resolve ptr + dispatch on abort” logic to live in a single helper in perry-runtime and have the existing abort-related entry points call it. Keep the behavior centralized for the functions around js_abort_signal_resolve_ptr, js_abort_signal_listeners_copy, and the abort-only branches so future signal-semantic changes only need one implementation.
🤖 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/object/native_call_method.rs`:
- Around line 741-775: Guard the AbortSignal fast path in native_call_method
before reading recv_ptr.class_id, since pointer-tagged handle-band values can
enter this branch and dereferencing them may hit unmapped memory. Update the
addEventListener/removeEventListener/throwIfAborted path to use the same
handle-band vs object-pointer validation already used elsewhere in
native_call_method.rs, so only real object pointers reach the
ABORT_SIGNAL_CLASS_ID check and other values fall through to the normal
TypeError path.
---
Nitpick comments:
In `@crates/perry-ext-events/src/lib.rs`:
- Around line 1855-1864: The AbortSignal handling is duplicated between this
module and the matching helpers in perry-stdlib, so update the shared “resolve
ptr + dispatch on abort” logic to live in a single helper in perry-runtime and
have the existing abort-related entry points call it. Keep the behavior
centralized for the functions around js_abort_signal_resolve_ptr,
js_abort_signal_listeners_copy, and the abort-only branches so future
signal-semantic changes only need one implementation.
🪄 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: 612e32d4-0234-424f-907d-f11064464aee
📒 Files selected for processing (6)
crates/perry-ext-events/src/lib.rscrates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/url/abort.rscrates/perry-stdlib/src/events/module_helpers.rscrates/perry/tests/issue_events_helpers_abort_signal.rs
… receivers A native handle (Headers/fetch/timer/...) is nanbox-pointer-tagged but its "pointer" is a small integer id, not a heap ObjectHeader. Guard the class_id read in the fused dynamic-call AbortSignal arm with is_small_handle so a handle receiver never gets dereferenced as an object.
Problem
Node treats
AbortSignalas anEventTarget. Perry represents it as its own native object (url/abort.rs) that two dispatch layers didn't recognize, which breaks real minified applications in two ways:1. Module-level
node:eventshelpers reject signals.This is the standard way SDKs raise the MaxListenersExceededWarning threshold on a shared signal; the throw rejects the caller's whole request path.
2. Signal methods vanish on dynamically-typed receivers.
Same class as the #5964 URLSearchParams dynamic-dispatch wall.
Fix
perry-stdlib/events/module_helpers.rs+perry-ext-events/lib.rs, kept in sync): recognize signals ahead ofevent_helper_targetinsetMaxListeners/getMaxListeners/listenerCount/getEventListeners. Set is a faithful no-op (the warning threshold is the call's only Node-observable effect and Perry never emits that warning for signals — documented inline), get reports the EventTarget default (10), and the listener queries report the signal's registered "abort" listeners via two new runtime helpers (js_abort_signal_listener_count,js_abort_signal_listeners_copy— a copy, so callers can't mutate the internal list).get_field_by_name_tail.rs+native_call_method.rs): property GET returns a bound-method closure (abort_signal_method_bind, signal captured as its NaN-boxed bits so the GC scan keeps/relocates it), and the fused dynamic method CALL pre-dispatchesaddEventListener/removeEventListener/throwIfAbortedto the natives (the URLSearchParams methods unreachable through dynamically-typed access (append is not a function) #5961 precedent block).optionsis accepted and ignored — a signal only ever fires"abort"once, so{ once: true }is behaviorally implied.Tests
crates/perry/tests/issue_events_helpers_abort_signal.rs(2 e2e compile+run tests):set=ok max=10 count0=0 count1=1 list1=1 other=0and a plain object still throws.typeof s.addEventListener === "function", 2-arg and 3-arg registration,removeEventListenerworks, listeners actually fire onabort(postAbortFired=11),throwIfAbortedno-ops while pending and throws after abort.Both pass;
cargo buildclean on the touched crates.Summary by CodeRabbit
New Features
AbortSignalsupport tonode:eventshelper APIs (getEventListeners,listenerCount,getMaxListeners,setMaxListeners) for"abort"listeners.AbortSignalmethod calls now correctly dispatchaddEventListener,removeEventListener, andthrowIfAborted.Bug Fixes
setMaxListeners(signal)no longer throws forAbortSignaland is treated as a safe no-op for signals.getMaxListeners(signal)returns the default cap (10), and non-"abort"events return empty/zero results.Tests