fix(runtime): route AsyncLocalStorage super() through any bound-export heritage shape - #10634
proggeramlug wants to merge 2 commits into
Conversation
proggeramlug
added
the
package-audit
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughChangesAsyncLocalStorage heritage dispatch
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Subclass
participant js_fetch_or_value_super
participant RuntimeHook
participant PerryStdlib
Subclass->>js_fetch_or_value_super: call super()
js_fetch_or_value_super->>RuntimeHook: resolve AsyncLocalStorage hook
RuntimeHook->>PerryStdlib: invoke js_async_local_storage_subclass_init
PerryStdlib-->>Subclass: return initialized subclass instance
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 |
|
Held out of merge train 222 — this needs a rebase onto current main, and so does its sibling #10649. Both conflict on the same file for the same reason.
I'd rather you took this than resolve it here: you have the series context, and hunk 2 is exactly the shape where a mechanical resolution produces something that builds and is wrong. Rebase Worth knowing for the rebase: your |
…t heritage shape
class X extends AsyncLocalStorage threw "Class constructor
AsyncLocalStorage cannot be invoked without 'new'" at super() for every
heritage shape except a bare import { AsyncLocalStorage } from
"node:async_hooks" binding -- the same defect #10621 fixed for
AsyncResource (#10453). A local alias, a namespace member, a default
import, and a CJS destructured require() all reach the identical bound
native export value the bare import does, but only that shape is
recognized statically at HIR-lowering time
(crates/perry-hir/src/lower_decl/class_decl.rs), which routes to
perry-stdlib's js_async_local_storage_subclass_init via a
codegen-declared extern symbol. Every other shape fell through
js_fetch_or_value_super to a plain CALL of the bound export.
Unlike AsyncResource, whose implementation lives entirely in
perry-runtime, AsyncLocalStorage's subclass-init helper lives in
perry-stdlib (it needs the stdlib Handle registry), and perry-runtime
cannot depend on perry-stdlib. Route through a registration hook
perry-stdlib installs at startup (JS_NATIVE_ASYNC_LOCAL_STORAGE_SUBCLASS_INIT),
matching the existing JS_NATIVE_ASYNC_HOOKS_CONSTRUCT /
JS_NATIVE_EVENTS_CONSTRUCT pattern already used for this exact kind of
cross-crate reach.
Adds test_gap_10625_asynclocalstorage_heritage.ts covering the canonical
import (control), local alias, namespace member, default import, and
two CJS require() shapes, asserting a real run()/getStore() round-trip
through the subclass -- not just that construction doesn't throw.
7dc50de to
926b820
Compare
…eritage shape
Generalizes js_fetch_or_value_super (crates/perry-runtime/src/object/global_this/fetch_globals.rs)
to recognize Readable/Writable/Duplex/Transform reached through a local alias, namespace member,
indirect subclass, or CJS destructured require('stream') -- the same pattern #10621/#10634 already
fixed for AsyncResource/AsyncLocalStorage. PassThrough is deliberately left unhandled (separate,
deeper HIR-level gap; see code comment).
Fixes #10448
1 participant
Summary
class X extends AsyncLocalStoragethrewClass constructor AsyncLocalStorage cannot be invoked without 'new'atsuper()for every heritage shapeexcept a bare
import { AsyncLocalStorage } from "node:async_hooks"binding — the same defect #10621 fixed for
AsyncResource(#10453). A localalias, a namespace member, a default import, and a CJS destructured
require()all reach the identical bound native export value the bareimport does, but only that shape was recognized statically.
Root cause
Only the canonical bare-import heritage name is recognized statically at
HIR-lowering time (
crates/perry-hir/src/lower_decl/class_decl.rs:404-406),which routes to perry-stdlib's
js_async_local_storage_subclass_initvia acodegen-declared extern symbol
(
crates/perry-codegen/src/expr/this_super_call.rs:337-342,:1002-1006).Every other heritage shape (local alias, namespace member, default import,
CJS
require()) can't be resolved by name at lowering time, sosuper()falls through at RUNTIME to
js_fetch_or_value_super(
crates/perry-runtime/src/object/global_this/fetch_globals.rs), whichpreviously only special-cased WASI and did a plain CALL of the bound native
export for everything else — and
AsyncLocalStorage's constructor throws bydesign when called without
new.The design decision (per the issue)
Two shapes were available to make the fix reachable from
js_fetch_or_value_super(which lives in perry-runtime):js_async_local_storage_subclass_initdown into perry-runtime.startup.
Chose (2).
js_async_local_storage_subclass_init(
crates/perry-stdlib/src/async_local_storage.rs:129) depends onperry-stdlib's own
Handleregistry (get_handle_mut/register_handle)and
crate::common::dispatch::unbound_async_local_storage_methodto wire upthe
run/getStore/enterWith/exit/disablemethods on the subclassinstance — moving it would mean either relocating that registry too or
duplicating it, which is exactly the kind of architectural change the fix
guidance says to avoid for a bug this narrow. Meanwhile the registration-hook
pattern already exists in this exact spot for this exact reason:
JS_NATIVE_ASYNC_HOOKS_CONSTRUCT/js_set_native_async_hooks_construct(
crates/perry-runtime/src/value/{tags,handle}.rs) is the identicalmechanism, registered by perry-stdlib at startup so perry-runtime's
newdispatcher (
crates/perry-runtime/src/object/class_registry/construct.rs)can reach a stdlib-side
AsyncLocalStorage/AsyncResourceconstructorwithout depending on perry-stdlib.
JS_NATIVE_EVENTS_CONSTRUCTis the samepattern for
events.EventEmitter. I addedJS_NATIVE_ASYNC_LOCAL_STORAGE_SUBCLASS_INIT/js_set_native_async_local_storage_subclass_init, matching that precedentexactly (same
AtomicPtr<()>+ setter + null-check-before-dispatch shape),and
js_fetch_or_value_supernow routes("async_hooks", "AsyncLocalStorage")through it, mirroring the existing WASI arm.bound_native_callable_module_and_method(the function that resolves aruntime value back to its
(module, method)bound-native-export identity)needed no changes — it already generically resolves any bound native
export, independent of how the heritage expression reached it (bare import,
alias, namespace member, CJS destructure); only the per-consumer match arm
in
js_fetch_or_value_superwas missing forAsyncLocalStorage. I alsoswitched the existing WASI-arm's
wasi_parent.is_some_and(...)(whichconsumed the
Option) tobound_native_parent.as_ref().is_some_and(...),since the resolved value now has two consumers.
Tests
test-files/test_gap_10625_asynclocalstorage_heritage.ts(+gap_10625_asynclocalstorage_heritage_helper.cjs) covers, for each shape, areal
run()/getStore()round-trip through the subclass instance (not justthat construction doesn't throw) — including a nested
run()call andinstanceof/method-typeofchecks:import { AsyncLocalStorage }(control — already worked)const Alias = AsyncLocalStorage)import * as ah,ah.AsyncLocalStorage)import ahDefault from "node:async_hooks",ahDefault.AsyncLocalStorage)require()require()resultProof it fails without the fix: built a pristine clone at
main(
0058babd83, no fix), ran the new gap test —TS extends AsyncLocalStorage (import)passed, all five other shapes threwClass constructor AsyncLocalStorage cannot be invoked without 'new'.Confirmed the fix makes all six pass (100% parity), and re-confirmed the
--releaseissue_806integration failure below is unaffected by stashingthe fix and rebuilding — it reproduces identically on unmodified
main.Validation
RUST_TEST_THREADS=1for perry-runtime):perry-runtime: 3998 passed / 2 failed / 4 ignored. Both failures(
gc::tests::copy_slot_decode::sabotaged_remembering_arm_is_refused_by_the_coverage_cross_check,gc::tests::heap_generation::a_free_or_move_outside_every_scope_is_caught_in_debug_builds)are
debug_assert!-dependent and compiled out under--release(documented in
CLAUDE.md); both pass undercargo test --profile gcaudit(release codegen + debug assertions). Unrelated to thischange — neither test touches
value/,object/global_this/, orasync_hooks/async_local_storage.perry-stdlib: 139 passed / 0 failed (lib), all integration suites 0tests (no stdlib-crate integration tests exist for this area).
./scripts/run_lint_gates.sh, full,SKIP_COMPILE_GATESnotset): 82/83 gates passed; pre-existing red:
[Public benchmark evidence freshness] python3 benchmarks/ci_public_baseline_check.py— redon every PR in this repo, not mine to fix.
PERRY_SKIP_BUILD=1 ./run_parity_tests.sh --filter <name>): new test 100% parity; re-rantest_async_local_storage_context,test_gap_als_run_spread_args,test_gap_asynchooks_3089_3090_3091_3093,test_harness_async_context,test_parity_async_context,test_parity_async_hooks,test_parity_async_local_storage— all 100% parity, no regressions.Also ran
crates/perry/tests/issue_5587_temporal_subclass.rs(6/6passed, same
js_fetch_or_value_supercodepath) andcrates/perry/tests/issue_806_default_derived_ctor_forwarding.rs(5/6passed — the 1 failure,
walk_into_rest_param_capturing_ctor, reproducesidentically with my changes stashed against unmodified
main; unrelatedrest-param/mixin-ctor capture bug, pre-existing).
perf stat -e instructions,task-clock, 3 runs each,pristine-
mainbinary vs fixed binary, both linked withPERRY_NO_AUTO_OPTIMIZE=1against their own prebuilt archives):newon aclass extends GlobalResponsealias — exercises the exact samejs_fetch_or_value_supercodepath my new arm adds a check to, butisn't itself
AsyncLocalStorage): baseline avg 6,440.68Minstructions vs fixed avg 6,434.41M instructions — no
regression (fixed is ~0.10% lower, within noise).
new ViaAlias()+run()/getStore(), the actual bug's hot path, previously threw):fixed avg 10,489.10M instructions, ~804ms task-clock. Node
(
node --experimental-strip-types, scaled) took 15.8s for 20kiterations of the equivalent workload — Perry is roughly 200x
faster here, nowhere near the 20%-slower floor.
Not verified
plausibly-affected tests since this change is narrowly scoped to one
runtime dispatch arm + one registration hook); leaving the 6 CI gap-suite
shards as the broader gate.
issue_806rest-param/mixin-ctorfailure or the two
debug_assert!-onlyperry-runtimetest failures —both confirmed pre-existing and unrelated, out of scope for this PR.
Fixes #10625
Summary by CodeRabbit
Bug Fixes
AsyncLocalStoragewhen accessed through aliases, namespace or default imports, and CommonJS destructuring.instanceof, and async context propagation.Tests
AsyncLocalStoragesubclassing across supported import and require patterns.