[JSC] innerModuleEvaluation: don't skip async-dep wait for siblings in same Evaluate()#215
Conversation
…n same Evaluate() The Bun-specific skip at 11.c.v (avoid self-deadlock when require(esm) / dynamic import re-enters a TLA module from inside its own suspension) previously fired whenever the dep was already EvaluatingAsync with pendingAsyncDependencies == 0. That also matches a sibling static import within the *same* Evaluate() pass when the TLA dep has no async deps of its own: the dep is suspended at its first await and bindings declared after it are still TDZ, so the importer ran too early (oven-sh/bun#30259). Thread an asyncOrderWatermark (the VM's moduleAsyncEvaluationCount at the start of this Evaluate()) through innerModuleEvaluation and only skip when the cycle root's asyncEvaluationOrder predates it, i.e. the dep transitioned to EvaluatingAsync in a *prior* Evaluate(). Siblings that transition during the current DFS keep the spec wait.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR adds async module evaluation watermarking to JavaScriptCore's module evaluation logic when ChangesAsync Module Evaluation Watermark
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Preview Builds
|
…aluate() Regression tests for #30259. Requires oven-sh/WebKit#215.
…0262) Fixes #30259. Bumps WebKit to `88b2f7a2159c913f7dd0d73c0e88d66138cd67ba` (oven-sh/WebKit#215, merged). ## What A static import that re-imports a TLA dep already visited by a sibling earlier in the **same** `Evaluate()` pass was skipping the spec-mandated wait and running with the dep's post-`await` bindings still in TDZ: ```ts // root.ts import { foo } from "./await.ts"; // await.ts → EvaluatingAsync, suspended at `await 0` import "./child.ts"; // child.ts visits await.ts, skips wait, runs too early // await.ts await 0; export const foo = 123; // child.ts import { foo } from "./await.ts"; console.log(foo); // ReferenceError: Cannot access 'foo' before initialization ``` ## Why The Bun-specific skip at `innerModuleEvaluation` 11.c.v (which avoids self-deadlock when `require(esm)` / dynamic import re-enters a TLA module from inside its own suspension) used `pendingAsyncDependencies == 0` as the discriminator for "body already entered". A TLA dep with no async deps of its own also has count 0 after suspending at its first `await` within the same DFS, so the discriminator matched the sibling case. ## How (oven-sh/WebKit#215) Snapshot `vm.moduleAsyncEvaluationCount()` at the start of each `Evaluate()` and thread it through `innerModuleEvaluation` as `asyncOrderWatermark`. Only skip the wait when the cycle root's `asyncEvaluationOrder()` **predates** the watermark (i.e. it became `EvaluatingAsync` in a *prior* `Evaluate()` — the re-entrant deadlock case). Siblings that transition during the current DFS get `order >= watermark` and keep the spec wait. ## Tests Two regression tests in `test/js/bun/resolve/dynamic-import-tla-cycle.test.ts`: - direct sibling re-import (the issue repro) - indirectly-shared TLA dep through different parents (guards against discriminating by "asyncParentModule on stack") Existing re-entrancy / deadlock-avoidance tests in the same file continue to pass.
Fixes oven-sh/bun#30259.
What
The Bun-specific skip at 11.c.v (avoid self-deadlock when
require(esm)/ dynamic import re-enters a TLA module from inside its own suspension) previously fired whenever the dep was alreadyEvaluatingAsyncwithpendingAsyncDependencies == 0.That also matches a sibling static import within the same
Evaluate()pass when the TLA dep has no async deps of its own: the dep is suspended at its firstawaitand bindings declared after it are still TDZ, so the importer ran too early.How
Thread an
asyncOrderWatermark(the VM'smoduleAsyncEvaluationCountsnapshot at the start of thisEvaluate()) throughinnerModuleEvaluationand only skip when the cycle root'sasyncEvaluationOrder()predates it — i.e. the dep transitioned toEvaluatingAsyncin a priorEvaluate()call (the re-entrant deadlock case). Siblings that transition during the current DFS getorder >= watermarkand keep the spec-mandated wait.The existing
pendingAsyncDependencies == 0guard is kept: a prior-Evaluate()dep can still be queued behind its own async deps with its body never entered.Tests
Regression tests added on the Bun side in
test/js/bun/resolve/dynamic-import-tla-cycle.test.ts(direct sibling + indirectly-shared sibling). Existing re-entrancy / deadlock-avoidance tests in the same file continue to pass.