fix(async_hooks): expose constructor prototypes - #7093
Conversation
📝 WalkthroughWalkthroughAdds reflective prototype metadata and method dispatch for ChangesAsync hooks prototype support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AsyncHooksConstructor
participant AsyncHooksPrototype
participant PrototypeMethodThunk
participant DynamicMethodDispatcher
AsyncHooksConstructor->>AsyncHooksPrototype: attach rooted prototype and method closures
AsyncHooksPrototype->>PrototypeMethodThunk: invoke reflective method with receiver and arguments
PrototypeMethodThunk->>DynamicMethodDispatcher: forward method name and arguments
DynamicMethodDispatcher-->>PrototypeMethodThunk: return method result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
![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
🤖 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_module/async_hooks_exports.rs`:
- Around line 59-122: The allocating operations in attach_prototype can move the
constructor and prototype, leaving raw addresses stale. Root both values with
crate::gc::RuntimeHandleScope and reload their current addresses before each
reuse after js_object_alloc, js_string_from_bytes, and js_closure_alloc; in
crates/perry-runtime/src/object/native_module/callable_exports.rs lines
1683-1726, closure_addr requires no independent change because the upstream
rooting fix makes its reuse safe.
- Around line 43-52: Update the receiver guard for the enterWith/disable no-op
path to use the canonical is_plausible_heap_addr predicate instead of
!is_handle_band. Preserve the pointer check, ensuring only plausible generic
heap objects take the no-op return while handle-band values and addresses below
the heap floor fall through.
🪄 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: 202fbf56-7789-45cc-80a0-8e7ef1e73887
📒 Files selected for processing (5)
changelog.d/7093-async-hooks-prototypes.mdcrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/native_module/async_hooks_exports.rscrates/perry-runtime/src/object/native_module/callable_exports.rscrates/perry/tests/issue_6764_async_hooks_prototype_metadata.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-runtime/src/object/native_module/callable_exports.rs (1)
125-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
set_bound_native_closure_nameinstead of duplicating it. Lines 125-134 reproduce the body ofset_bound_native_closure_name(Lines 1464-1497), including the identicalPropertyAttrs::new(false, false, true)descriptor, but drop the documented#6809rationale. Passing the reloaded pointer to the existing helper keeps that invariant in one place.♻️ Proposed refactor
- crate::closure::closure_set_dynamic_prop( - closure_handle.get_raw_mut_ptr::<crate::closure::ClosureHeader>() as usize, - "name", - f64::from_bits(JSValue::string_ptr(name_handle.get_raw_mut_ptr()).bits()), - ); - super::super::set_builtin_property_attrs( - closure_handle.get_raw_mut_ptr::<crate::closure::ClosureHeader>() as usize, - "name".to_string(), - super::super::PropertyAttrs::new(false, false, true), - ); + set_bound_native_closure_name( + closure_handle.get_raw_mut_ptr::<crate::closure::ClosureHeader>(), + name, + );Note: the helper allocates its own name string, so
name_string/name_handlebecome unnecessary here.🤖 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/object/native_module/callable_exports.rs` around lines 125 - 134, Replace the duplicated name-property setup in the current callable export path with the existing set_bound_native_closure_name helper, passing the reloaded closure pointer as required. Remove the now-unnecessary name_string/name_handle allocation and preserve the helper’s documented property attributes and naming behavior.
🤖 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.
Nitpick comments:
In `@crates/perry-runtime/src/object/native_module/callable_exports.rs`:
- Around line 125-134: Replace the duplicated name-property setup in the current
callable export path with the existing set_bound_native_closure_name helper,
passing the reloaded closure pointer as required. Remove the now-unnecessary
name_string/name_handle allocation and preserve the helper’s documented property
attributes and naming behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aacb02c3-bffb-4e29-a456-787edd4a383a
📒 Files selected for processing (2)
crates/perry-runtime/src/object/native_module/async_hooks_exports.rscrates/perry-runtime/src/object/native_module/callable_exports.rs
|
Addressed the late CodeRabbit maintainability note in c3bb2c4: async_hooks static methods now reuse set_bound_native_closure_name. I also made the shared helper root/reload both the closure and allocated name so the refactor preserves relocation safety across its allocations. Validation: cargo fmt/check and the full issue_6764_async_hooks_prototype_metadata integration test (1 passed, 156.08s). |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-runtime/src/object/native_module/callable_exports.rs (1)
1460-1495: 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy liftReturn or preserve the relocated closure handle.
This scope protects
closureduring allocation, but the function returns no updated pointer. Any caller that reuses its original raw pointer after this call may access stale memory if GC relocates the closure. Pass an outerRuntimeHandleScopeand return the reloaded pointer, then update callers to use it.Based on learnings, Rust stack locals are not GC roots; relocated values must be reloaded from their handles before reuse.
🤖 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/object/native_module/callable_exports.rs` around lines 1460 - 1495, Update the closure-name installation function around RuntimeHandleScope and closure_handle so it accepts an outer RuntimeHandleScope and returns the closure’s reloaded pointer after all allocations and property updates. Ensure callers use the returned handle-derived pointer rather than reusing their original raw closure pointer, preserving GC safety when the closure relocates.Source: Learnings
🤖 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.
Outside diff comments:
In `@crates/perry-runtime/src/object/native_module/callable_exports.rs`:
- Around line 1460-1495: Update the closure-name installation function around
RuntimeHandleScope and closure_handle so it accepts an outer RuntimeHandleScope
and returns the closure’s reloaded pointer after all allocations and property
updates. Ensure callers use the returned handle-derived pointer rather than
reusing their original raw closure pointer, preserving GC safety when the
closure relocates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ec5e6011-5bbf-47c9-a575-b76226155584
📒 Files selected for processing (1)
crates/perry-runtime/src/object/native_module/callable_exports.rs
1 participant
Summary
AsyncLocalStorage.prototypeandAsyncResource.prototypeobjects with Node-compatible method names and aritiesAsyncResource.lengthfrom 2 to 1 and cover constructor/prototype descriptors and real callsThis is a focused parity increment for the larger async_hooks umbrella; the remaining lifecycle/provider gaps stay tracked in #6764.
Refs #6764
Validation
cargo check -p perry-runtimecargo test -p perry-runtime arity_table_matches_reference_exhaustively -- --test-threads=1cargo test -p perry --test issue_6764_async_hooks_prototype_metadata -- --nocapture./run_parity_tests.sh --suite node-suite --module async_hooks --filter api-function-metadata(100%)./run_parity_tests.sh --suite node-suite --module async_hooks --filter run-nesting(100%)Summary by CodeRabbit
node:async_hooksconstructor and prototype metadata forAsyncLocalStorageandAsyncResource.run,runInAsyncScope,enterWith, anddisable.async_hookscallable arity forAsyncResourceto match expected behavior.enterWith/disableon unsupported (“foreign”) receivers consistently returnsundefined.