fix(runtime): AsyncResource super() via any bound-export heritage shape, not just bare import - #10621
proggeramlug wants to merge 2 commits into
Conversation
…lue, not just the bare-import name class X extends AsyncResource threw "Class constructor AsyncResource cannot be invoked without 'new'" at super() for every heritage shape except a bare import binding. A local alias, a namespace member, and a CJS destructured require() all resolve to the identical bound native export value the canonical import does, but only the bare import shape was recognized statically at HIR-lowering time, so super() fell through to a plain call of the export -- which AsyncResource throws on by design. Recognize the bound export VALUE in js_fetch_or_value_super, exactly as the existing WASI arm does, and run the same native-backing init the canonical path already uses.
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 (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe runtime now recognizes bound ChangesAsyncResource heritage resolution
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change has no remaining concrete merge-blocking risk in the reviewed AsyncResource inheritance paths. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
1 participant
Summary
class X extends AsyncResourcethrewClass constructor AsyncResource cannot be invoked without 'new'atsuper()for every heritage shapeexcept a bare
import { AsyncResource } from "node:async_hooks"binding. Alocal alias (
const Alias = AsyncResource), a namespace member(
ah.AsyncResource), and a CJS destructuredrequire('node:async_hooks')all threw the same way.
undiciuses exactly the CJS destructured shape inevery API handler (
lib/api/api-request.jsetc.:const { AsyncResource } = require('node:async_hooks'); class RequestHandler extends AsyncResource),so
undici.request()and theAgent/Pool/Clientrequest methods allfailed.
Root cause
canonical_native_parent_name(crates/perry-hir/src/lower_decl/class_decl.rs:10-24)recognizes a native-module parent (including
async_hooks'sAsyncResource/AsyncLocalStorage) only when the heritageIdentis abare import binding. That is the only shape that reaches the dedicated
static codegen (
crates/perry-codegen/src/expr/this_super_call.rs:305-355),which calls
js_async_resource_subclass_initdirectly. Every other shape —a local alias, a namespace member, a CJS destructured
require()— iseither
locally_shadowedor a member expression, so it takes the dynamicextends_exprpath instead:super()resolves throughjs_fetch_or_value_super(
crates/perry-runtime/src/object/global_this/fetch_globals.rs:642), whichhad no recognition arm for
async_hooksclasses and fell through to aplain CALL of the bound native export — and
AsyncResourcethrows bydesign when called without
new(
crates/perry-runtime/src/object/native_module_dispatch/dispatch_a_c.rs:155-167).The fix doesn't need to catch every syntactic aliasing shape at
HIR-lowering time (which
canonical_native_parent_namealready tries, andinherently can't cover every case — a runtime alias like
obj[key] = AsyncResource; class X extends obj[key]has no static name atall).
js_fetch_or_value_superalready has a general mechanism for thiskind of problem:
bound_native_callable_module_and_methodinspects thevalue reaching
super()— a bound-native-export closure carrying itsoriginating module/method name as captures — which is identical regardless
of how the heritage expression reached it. The existing WASI arm already
uses this to recognize
class X extends anyAliasOf(WASI)generally. ThisPR adds the same recognition for
async_hooks'sAsyncResource.The fix
crates/perry-runtime/src/object/global_this/fetch_globals.rs:generalized the existing
wasi_parentlocal (renamedbound_native_parent,.as_ref()instead of consuming) and added a new armright after the WASI one: when the resolved
(module, method)is("async_hooks", "AsyncResource"), pulltype/optionsfrom the raw argsand call
js_async_resource_subclass_init— the exact same runtimefunction the canonical bare-import path already uses — instead of falling
through to the plain-call dispatch that throws.
AsyncLocalStorageis very likely affected the same way for the samereason (untested — see "Not verified" below): its subclass-init helper
(
js_async_local_storage_subclass_init) lives inperry-stdlib, notperry-runtime, andperry-runtimecannot depend onperry-stdlib(thedependency only goes the other way —
perry-stdlibis sometimes aseparate dylib image in app-only deployments, per the comment at the top of
crates/perry-stdlib/src/async_local_storage.rs). Fixing it the same waywould need either a cross-crate FFI declaration (risky under a
dylib-provider deployment) or moving/duplicating the init helper — that's
more than this PR's surgical fix, so it's left out. Filing a follow-up if
the maintainer agrees it's worth a dedicated issue.
Tests
New gap test
test-files/test_gap_10453_asyncresource_heritage.ts+test-files/gap_10453_asyncresource_heritage_helper.cjs(the exactundici-shaped CJS destructuredrequire()), covering: bare import(already worked, kept as a control), local alias, namespace member, CJS
destructured
require()with a plainsuper(), and the same inside atry/catch. Node oracle (26.5.1) prints "ok" for all six; on thepristine
mainbaseline (0058babd83), five of six throw:This branch's binary matches Node byte-for-byte on all six lines, and
PERRY_SKIP_BUILD=1 ./run_parity_tests.sh --filter test_gap_10453_asyncresource_heritagereports PASS.I found one more, unrelated pre-existing gap while writing the repro and
removed it from the committed test rather than folding it in:
class NoCtor extends AsyncResource {}withnew NoCtor("X")(the implicit defaultderived constructor forwarding
super(...args)) throwsThe "type" argument must be of type string. Received undefinedon both thebaseline and this fix — the implicit derived constructor for a native base
doesn't forward its arguments to
super(). Not touched here.Validation
cargo test --release -p perry-runtime --tests(RUST_TEST_THREADS=1):3998 passed, 2 failed. Both failures are pre-existing and unrelated —
confirmed by running the identical two tests against the pristine
baseline (
0058babd83), where they fail identically:gc::tests::copy_slot_decode::sabotaged_remembering_arm_is_refused_by_the_coverage_cross_checkand
gc::tests::heap_generation::a_free_or_move_outside_every_scope_is_caught_in_debug_builds.Both assert
debug_assert!-only behavior, which is compiled out under--release(see CLAUDE.md's "Verifying a runtime change" note) — they'dneed
--profile gcauditto actually exercise the assertion. Neithertouches anything near
fetch_globals.rs.SKIP_COMPILE_GATES=1 ./scripts/run_lint_gates.sh): 76 of 77passed (compile tier skipped per the box's standing instructions). The
one red,
[Public benchmark evidence freshness] python3 benchmarks/ci_public_baseline_check.py, is the documented pre-existingred on every PR in this repo (CLAUDE.md, PR step 6) — reproduced
identically on the pristine baseline.
check_test_registration.py(both above); did not run the full local suite (this change is scoped to
one
async_hooks-specific dispatch arm, not a hot lowering/runtime pathused by most programs). Deferring the broader gap-suite gate to CI's
6-shard run per the box's standing instructions.
perf stat -e instructions,task-clock, 3 runs each,200k-iteration loops, baseline = pristine
0058babd83):super()dispatch that doesnot match
wasi/async_hooks(the shared fallthrough path thisfix adds one check ahead of): baseline ≈1.896B instructions avg, this
branch ≈1.897B avg — a ~0.07% difference, noise.
class X extends Alias(Alias = AsyncResource)constructed 200k times: throws immediately on baseline (no instruction
count to compare), ≈47.9B instructions on this branch. For scale, the
already-working canonical bare-import shape doing the identical
200k-construction workload costs ≈37.0B instructions on the pristine
baseline — so the dynamic-dispatch path this PR adds costs roughly
25–30% more instructions than the already-existing static path for the
same work, which is the expected, inherent cost of resolving the bound
export value dynamically rather than statically; it is not a
regression (this exact program did not run at all before). Node wall
time for the same 200k-construction workload: 0.137–0.143s, for
context only (instruction counts are the load-bearing comparison on
this shared host).
undiciitself — it isn't installed inthis clone and there's no existing
perry.compilePackageswiring for it,so a real package-level compile wasn't cheap to set up. The CJS
destructured-
requireshape in the new gap test is copied verbatim fromundici's actual usage pattern (per the issue body), so I'm confidentthis closes undici's specific blocker, but I haven't run undici's own
test suite against this build.
Not verified
AsyncLocalStoragethrough the same non-canonical heritage shapes (see"The fix" above for why it's out of scope here).
undici's own package-level compile/test suite.Fixes #10453
Summary by CodeRabbit
Bug Fixes
AsyncResourcewhen it was referenced through aliases, namespace members, or CommonJS imports.AsyncResourcesubclasses now initialize correctly across supported import and inheritance patterns.Tests
AsyncResourceinheritance scenarios.