Module loader: skip the TLA wait when the dynamic import's promise gates the dependency - #543
Conversation
…tes the dependency
innerModuleEvaluation step 12.b.v makes a module wait for a dependency
that is EvaluatingAsync. When the Evaluate() belongs to a dynamic
import() whose result that dependency awaits, the wait is a deadlock:
the dependency resumes only after the import settles, and the import
settles only after the graph being evaluated finishes.
Bun already skips the wait when the module that lexically contains the
import() call is the dependency (referrerAsyncOrder, captured at the
call site). A helper module that issues the import() on behalf of the
suspended module has no async order of its own, so the skip misses and
the process hangs:
entry.ts: export function helper() {}
const { load } = await import("./loader.ts");
await load();
loader.ts: export async function load() { await import("./child.ts"); }
child.ts: import { helper } from "./entry.ts";
The helper's import() promise has, by the time Evaluate() runs in a
microtask, the reactions that describe what the caller does with it:
an await in an async function (holding that function's promise), an
await in a module body (holding the module), .then()/.finally() and
resolve-with-promise plumbing (holding a derived promise), Promise.all
(holding the combined promise), and the loader's own pipeline steps.
Thread the dynamic import's capability promise through
ModuleLoaderPayload into evaluate() and innerModuleEvaluation. At
12.b.v, before waiting on an EvaluatingAsync dependency, follow those
reactions from the import promise. If a chain ends in the body of the
dependency, or of a module the dependency waits on through
[[AsyncParentModules]], the wait is a guaranteed deadlock and is
skipped. Promise.race/any can settle without the import and are not
followed. Resolving functions captured by closures and async generator
queues are opaque and end the walk, so anything the walk cannot see
keeps the spec behavior of waiting. The referrerAsyncOrder check stays
as it is.
JSPromise::forEachPendingReaction exposes the reaction list (inline or
heap) as (task, promiseOrCapability, contextOrHandler) triples so the
walk does not depend on the storage layout.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughBun-specific module evaluation now carries the dynamic-import promise through cyclic evaluation. JavaScriptCore can inspect pending promise reactions and detect async dependency deadlocks before registering dependencies. ChangesDynamic import evaluation
Merge Risk: ⚪ Minimal · up to The PR changes module-evaluation behavior to avoid dynamic-import and top-level-await deadlocks, and no confirmed current-head build or correctness failure is identified; it is merge-ready after normal checks and review with no actionable merge-blocking risk remaining. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the problem, fix, scope, and verification. However, it does not follow the repository template because it omits the Bugzilla bug title and link, the "Reviewed by NOBODY (OOPS!)." line, and the required changed-file and function list.
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| case InternalMicrotask::PromiseAllResolveJob: | ||
| case InternalMicrotask::PromiseAllSettledResolveJob: { | ||
| // The combined promise waits for every element. | ||
| JSCell* contextCell = cellOf(cell); | ||
| if (auto* globalContext = contextCell ? dynamicDowncast<JSPromiseCombinatorsGlobalContext>(contextCell) : nullptr) | ||
| follow(globalContext->promise()); | ||
| break; |
There was a problem hiding this comment.
🔴 Following PromiseAllResolveJob treats the combined promise as gated by the import, but Promise.all short-circuits to rejection when any other element rejects (JSMicrotask.cpp:445-448), so the wait is not a guaranteed deadlock: on base a helper doing await Promise.all([import(plugin), mayReject]) lets plugin wait on the awaiter and see its post-await bindings once mayReject rejects, while after this change plugin runs immediately against TDZ/partial bindings. Fix: only claim gating for combinators whose settlement strictly requires this element — keep PromiseAllSettledResolveJob, but treat PromiseAllResolveJob as terminal (like race/any) or document the deviation.
Extended reasoning...
entry.mjs: import {load} from './loader.mjs'; try { await load(); } catch {} export const x = 1;. loader.mjs: export const load = () => Promise.all([import('./plugin.mjs'), fetch(url)]); where fetch will reject. plugin.mjs: import { x } from './entry.mjs'; use(x);. When plugin's Evaluate() runs, entry is EvaluatingAsync and the walk from capabilityPromise sees PromiseAllResolveJob → follows globalContext->promise() → AsyncModuleExecutionResume for entry → resumesDependency(entry) returns true, so 12.b.v skips the wait and plugin executes with x still in TDZ. On the base branch referrerAsyncOrder is -1 (loader is not EvaluatingAsync) so plugin waits; when fetch rejects, promiseAllResolveJob (JSMicrotask.cpp:445) rejects the combined promise, entry catches, assigns x=1 and completes, and plugin then runs with x=1. Promise.allSettled truly needs every input to settle so following it is sound, but Promise.all's reject path settles without importPromise, breaking the invariant the walk relies on and the PR's "guaranteed deadlock" claim.
Verification: normal — The claim checks out against the code. In importPromiseGatesAsyncDependency (Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp), the PromiseAllResolveJob case unconditionally follows the combinator's aggregate promise: ```cpp case InternalMicrotask::PromiseAllResolveJob: case InternalMicrotask::PromiseAllSettledResolveJob: { // The combined promise waits for every element.…
There was a problem hiding this comment.
(Claude wrote this reply on Jarred's behalf.)
Deliberate, and now called out in the PR body. Promise.all is the one followed edge that is not a strict "settles only after the import" edge, because another element rejecting settles it early. Treating it as terminal would put await Promise.all([import(a), import(b)]) in a TLA module — a common shape, and one of the Bun-side test cases — back to hanging forever. The cost of following it is that in the reject-then-swallow shape you describe the plugin evaluates against the awaiter's pre-await bindings (TDZ) instead of waiting; that is the same trade the previous referrerAsyncOrder check made for fire-and-forget imports, and a thrown TDZ error is a better failure than a silent hang. allSettled stays followed (strict), race/any stay terminal.
…ck, and fail safe on unknown reactions The dynamic import's promise walk added in the previous commit already recognises every case the lexical referrerAsyncOrder check did (an `await import()` directly in the suspended module's body reaches that body through AsyncModuleExecutionResume), and it does not share that check's false positive: a fire-and-forget `import()` in a TLA module no longer skips the step 12.b.v wait when nothing would deadlock. Drop referrerAsyncOrder and its plumbing through importModule, requestImportModule, loadModule, ModuleLoadingContext, ModuleLoaderPayload and JSModuleLoader::asyncEvaluationOrderForKey. Embedders stop passing it. The walk now lists the reaction kinds it follows (user handlers, resolve/without-handler/finally plumbing, the ModuleLoad* and DynamicImport* pipeline steps, ImportModuleNamespace) and stops on anything else, so a new InternalMicrotask keeps the spec behaviour of waiting instead of silently skipping the wait. JSTests/modules/dynamic-import-tla-cycle.js covers the direct, helper, helper-after-await and Promise.all shapes with a watchdog that fails on deadlock; dynamic-import-tla-siblings-wait.js checks that two dynamic imports sharing a genuinely async dependency still wait for it.
main (#543) replaced the referrerAsyncOrder TLA-deadlock hint with the dynamic import()'s promise; the graph-instance evaluate/innerModuleEvaluation overloads now take (JSPromise* dynamicImportPromise, ModuleGraphInstance*), instance-internal evaluation passes no hint as before, and the import-promise walk follows the ModuleGraphInstance*Settled reactions.
| // VM-unique identity, so equality is exact — siblings that | ||
| // happen to be EvaluatingAsync (#30259, #30634) never match. | ||
| if (cyclic->asyncEvaluationOrder().order() != referrerAsyncOrder) { | ||
| if (!dynamicImportPromise || !importPromiseGatesAsyncDependency(dynamicImportPromise, cyclic)) { |
There was a problem hiding this comment.
🔴 Dropping the referrerAsyncOrder equality check (the PR body says it is "unchanged", but the code removed it) regresses the kick-off-then-await shape: an entry that does const p = import('./child'); await ioPromise; await p; now deadlocks in Bun where the base skipped the wait, because when dynamicImportLoadSettled runs the entry's AsyncModuleExecutionResume reaction is still on ioPromise, so the walk from capabilityPromise reaches the user-visible import promise with no reactions and returns false. Fix: keep a fallback that skips when cyclic is the lexical referrer (thread the referrer record or its asyncEvaluationOrder alongside the promise), or re-run the walk when the import promise later gains a module-resume reaction.
Extended reasoning...
entry.mjs: export function helper(){return 1}; const p = import('./child.mjs'); await new Promise(r=>setTimeout(r,0)); export const ns = await p;. child.mjs: import { helper } from './entry.mjs'; export const v = helper();. The import() call is lexically in entry, so on the base branch Bun's moduleLoaderImportModule hook passed referrerAsyncOrder = asyncEvaluationOrderForKey('entry.mjs') (the removed comment at old line 1330-1341 confirms the order is already assigned before the first await), and at 12.b.v cyclic->asyncEvaluationOrder().order() == referrerAsyncOrder matched, child skipped the wait, ran with helper already hoisted, and entry later resumed. After this diff evaluate() receives only capabilityPromise (the internal statePromise). When dynamicImportLoadSettled runs, the microtask queue has drained the loader hops but the setTimeout macrotask has not fired, so entry is suspended at await ioPromise with its AsyncModuleExecutionResume reaction attached to that timer promise, not to p. importPromiseGatesAsyncDependency walks statePromise →…
Verification: normal — The PR body claims "The referrerAsyncOrder check is unchanged", but the diff removes it entirely and replaces it with the promise-graph walk. Base (d71031a) at AbstractModuleRecord.cpp step 12.b.v: cpp if (cyclic->asyncEvaluationOrder().order() != referrerAsyncOrder) { referrerAsyncOrder was captured at the import() call site (via the now-removed… | normal — The PR body…
| setTimeout(() => { | ||
| if (settled != 4) | ||
| throw new Error("dynamic import() inside a TLA cycle deadlocked: " + settled + "/4 settled"); | ||
| }, 1000); |
There was a problem hiding this comment.
🟡 nit (optional): The 1000 ms watchdog setTimeout stays pending after all four imports settle, so the jsc shell run loop waits the full second before exiting even on success — every configuration of this test runs ≥1000 ms, violating the JSTests rule that new tests must complete in under 200 ms. Fix: drop the timer (a real deadlock already hangs the harness and fails), or clearTimeout it once settled == 4, and use a much shorter delay.
Extended reasoning...
JSTests/CLAUDE.md imports JSTests/README.md, whose §Adding Tests rule 1 requires new tests to run in under 200 ms in all configurations. dynamic-import-tla-cycle.js:4-7 arms setTimeout(..., 1000) before the loop and never clears it. On the happy path the four await import(...) calls resolve in a few ms, module TLA completes, but the shell's run loop still has the 1000 ms timer queued and drains it before exit; the callback then sees settled==4 and does nothing. Net effect: the test's wall time floor is 1 s across every stress configuration, which --report-execution-time will flag. If the shell does not wait for timers, the watchdog never fires at all and is dead code — either way it does not do its job cleanly.
Verification: nit — The 1000 ms watchdog does keep the jsc shell alive for the full second on the happy path. JSTests/modules/dynamic-import-tla-cycle.js:4-7 arms setTimeout(..., 1000) and never cancels it. In the jsc shell, setTimeout (Source/JavaScriptCore/jsc.cpp:3293) calls vm.deferredWorkTimer->addPendingWork(...), which inserts a ticket into m_pendingTickets (DeferredWorkTimer.cpp:211). After…
| JSPromise* loadModule(JSGlobalObject*, const Identifier& moduleName, RefPtr<ScriptFetchParameters>, RefPtr<ScriptFetcher>, OptionSet<ModuleLoadFlag>, const String& referrer = { }); | ||
| JSPromise* linkAndEvaluateModule(JSGlobalObject*, const Identifier& moduleKey, RefPtr<ScriptFetchParameters>, RefPtr<ScriptFetcher>); |
There was a problem hiding this comment.
🟡 nit (optional): Removing asyncEvaluationOrderForKey leaves an empty #if USE(BUN_JSC_ADDITIONS) / #endif pair behind. Fix: delete the dead preprocessor block.
Extended reasoning...
After the JS_EXPORT_PRIVATE int64_t asyncEvaluationOrderForKey(...) declaration is removed, JSModuleLoader.h lines 95-96 read #if USE(BUN_JSC_ADDITIONS) immediately followed by #endif with nothing between them. This compiles fine but is dead noise inconsistent with the rest of the header; the surrounding public API block no longer needs a Bun-specific guard here.
Verification: nit — Source/JavaScriptCore/runtime/JSModuleLoader.h lines 98-99 now read #if USE(BUN_JSC_ADDITIONS) immediately followed by #endif with nothing between them. The diff shows the sole guarded declaration JS_EXPORT_PRIVATE int64_t asyncEvaluationOrderForKey(const Identifier& key); was removed but the surrounding preprocessor guards were left behind. This is dead noise; it compiles fine and…
| case InternalMicrotask::AsyncModuleExecutionResume: { | ||
| JSCell* driver = unwrapContext(context); | ||
| if (auto* module = driver ? dynamicDowncast<AbstractModuleRecord>(driver) : nullptr) | ||
| found = resumesDependency(module); | ||
| break; |
There was a problem hiding this comment.
🟣 pre-existing: The AsyncModuleExecutionResume arm terminates the walk at resumesDependency(module) (which only climbs [[AsyncParentModules]]), so a cycle that goes through two dynamic imports — entry awaits import(mid), mid awaits import(child), child statically imports entry — still deadlocks exactly as on the base branch, because mid.[[AsyncParentModules]] is empty (dynamic imports never populate it) and the outer import chain hanging off mid's topLevelCapability is never followed. Fix: when the reaction resumes a module whose topLevelCapability() is pending, also follow() that capability so the walk can bridge nested dynamic imports; apply the same to the AsyncFunctionResume module branch.
Extended reasoning...
Repro: entry.mjs = export function h(){}; export const ns = await import('./mid.mjs');, mid.mjs = export const ns = await import('./child.mjs');, child.mjs = import { h } from './entry.mjs'; export const v = h();. When dynamicImportLoadSettled for the mid→child import calls child->evaluate(globalObject, mid_statePromise), innerModuleEvaluation reaches step 12.b.v with cyclic == entry (EvaluatingAsync) and calls importPromiseGatesAsyncDependency(mid_statePromise, entry). The DFS walks mid_statePromise → ModuleLoadCombinedStateSettled → combinedPromise → PromiseFulfillWithoutHandlerJob → intermediatePromise → ModuleLoadTopRejected → loadResult → ImportModuleNamespace → requestResult → PromiseResolveWithoutHandlerJob → outerPromise and hits its only reaction, AsyncModuleExecutionResume with context mid. Line 1273 runs resumesDependency(mid): mid != entry, and mid.asyncParentModules() is empty because entry reached mid via import() (no static edge, so 12.b.v.2 never appended entry), so it returns false and the walk ends with found == false. child then…
Verification: pre-existing — The two-level dynamic-import cycle (entry await import(mid), mid await import(child), child statically imports entry) still deadlocks after this change, exactly as it did on the base. Trace against the code: 1. dynamicImportLoadSettled for mid→child passes mid's inner capability promise into child->evaluate(globalObject, capabilityPromise) (JSMicrotask.cpp:1484). 2. That…
|
Rebased my for-await follow-up on top of b01e04d (commit 9eaa649): One finding from running Bun's suite against b01e04d. Dropping the lexical The two ways out:
I will follow whichever you prefer in the Bun PR. Everything else in |
Preview Builds
|
… import-promise walk The walk from #543 follows the dynamic import()'s promise reactions to decide whether waiting on an EvaluatingAsync dependency would deadlock. It cannot follow a chain that leaves JS: an import() issued from a server request handler while the entry module is suspended on `await fetch(ownServer)`, or one whose result is handed back through a resolver captured in a closure and a timer. In those shapes the only link between the import() and the suspended module is native, the walk ends, the dependency is waited on, and the process hangs. Restore referrerAsyncOrder (the asyncEvaluationOrder of the module that lexically contains the import() call, captured at the call site) and skip the wait when it equals the dependency's order, in addition to the walk. The jsc shell's import hook now threads it too so the new JSTests/modules/dynamic-import-tla-cycle-opaque.js covers the captured-resolver shape.
|
This change makes Bun's The fixture is a Nitro server bundle, Before this PR, This is the fire-and-forget shape the PR body describes as now waiting per spec, but it is also the shape Bun has supported since WebKit#32437 and tests for. A skip for an Repro with any Bun built against cd test/js/bun/http && PORT=0 bun js-sink-sourmap-fixture/index.mjs
# expected: exit 0 after "Listening on ..."; actual: "warn: Bun.serve() timed out a request after 10 seconds", exit 1 |
This reverts the move to fork main 01de4c1d (2918d3c). That commit carries oven-sh/WebKit#543, which replaces the referrer-based skip of the top-level-await wait in dynamic imports with a walk over the import promise's reactions. The Nitro fixture in test/js/bun/http/bun-server.test.ts does import("./chunks/stream.mjs") from a Bun.serve handler while index.mjs, which the chunk imports back, is awaiting the response. The walk cannot see through the HTTP hop, the wait is taken, and the request idles out. Bun built at 742c886cdc (the #543 merge) fails the test on every platform; at 167a4cef86 it passes. 167a4cef86 is upstream c119008088 merged into the fork's main and nothing after it. The oven-sh/WebKit#545 tests leave with the pin; they need that engine change.
|
Follow-up for readers of this thread: fork |
…inst the fix autobuild-preview-pr-548-4b30f36d is fork main e989e1e488 (which has oven-sh/WebKit#543 and the restored lexical referrer skip from 2718370ec0) plus the for-await step from oven-sh/WebKit#548. The pin moves to the fork main sha that merged #548 (491b5cc236) once its autobuild is published.
491b5cc236 is the fork main commit that merged oven-sh/WebKit#548. It includes oven-sh/WebKit#543 (the import-promise walk), the restored lexical referrer skip from 2718370ec0, and the for-await step. Its autobuild release carries the full platform and flavor matrix.
A top-level await that reaches a dynamic import() through a helper module hung forever when the imported module statically imported the awaiting module back (#41029). The module loader's deadlock skip only fired when the import() call was written in the awaiting module itself. oven-sh/WebKit#543 and oven-sh/WebKit#548 make the loader follow the import() promise's pending reactions (await in an async function or a module body, then/finally, Promise.all, for-await, loader plumbing) to the suspended module, so a helper between the awaiter and the import() no longer matters. The fix reaches Bun through the WebKit pin (#40987). The tests cover two and three import() levels, an async helper that awaits I/O before import(), a helper that returns the import() promise or chains .then() on it, Promise.all, for-await over lazily created imports, a non-entry awaiter, and a helper's fire-and-forget import() that must keep waiting for the awaiter.
2 participants
Problem
awaitthat reaches a dynamicimport()through a helper module hangs forever when the imported module statically imports the awaiting module back (Deadlock on 1.4.0: module dynamically imported two levels deep cannot statically import the entry back bun#41029). The reported shape is a plugin framework boot: entry awaits a loader, the loader imports each plugin, a plugin imports a helper from the entry. Bun 1.3.14 ran it, Bun 1.4 spins at 100% CPU with no output.innerModuleEvaluationstep 12.b.v makes the plugin wait for the entry, which isEvaluatingAsync. The entry waits for the loader, the loader waits for the plugin'simport(): a deadlock by spec (Node exits 13 on it). The previous skip (referrerAsyncOrder, from [ Gardening ][ macOS wk2 ] imported/w3c/web-platform-tests/webrtc/simulcast/setParameters-active.https.html is a flaky faliure WebKit/WebKit#32437's WebKit half) compared the dependency's order with the order of the module that lexically contains theimport()call. With a loader module in between, that module has no order and the skip never fired.Fix
ModuleLoaderPayloadalready carries the dynamic import's capability promise. Pass it intoevaluate()andinnerModuleEvaluation().EvaluatingAsyncdependency,importPromiseGatesAsyncDependencywalks the pending reactions hanging off that promise: anawaitin an async function (follows the function's own promise), anawaitin a module body (the module),.then()/.finally()and resolve-with-promise plumbing (the derived promise),Promise.all/allSettled(the combined promise), and the loader's ownModuleLoad*/DynamicImport*pipeline steps. When a chain ends in the dependency's body, or in a module the dependency waits on through[[AsyncParentModules]], the wait would deadlock and is skipped.referrerAsyncOrderrather than sitting beside it. The walk recognises every case the lexical check did (a directawait import()reaches the module body throughAsyncModuleExecutionResume) without its false positive: a fire-and-forgetimport()in a TLA module now waits, per spec, and sees the awaiter's post-await bindings.referrerAsyncOrderis removed fromimportModule,requestImportModule,loadModule,ModuleLoadingContext,ModuleLoaderPayload, andJSModuleLoader::asyncEvaluationOrderForKeyis gone. The Bun side drops the three lines inZigGlobalObject.cppthat computed and passed it.InternalMicrotaskkeeps the spec behaviour of waiting instead of silently skipping.Promise.race/anycan settle without the import and are not followed; resolving functions captured by closures and async generator queues end the walk.Promise.allis the one followed edge that is not strict: another element rejecting settles it without the import. It is followed anyway becauseawait Promise.all([import(a), import(b)])in a TLA module is common and would otherwise hang. The cost is that if another element rejects and the awaiter swallows the rejection, the imported module has already evaluated against the awaiter's pre-await bindings (a TDZ error) instead of waiting for them.Promise.allSettledis strict and followed.JSPromise::forEachPendingReactionexposes the reaction list (inline or heap) as(task, promiseOrCapability, contextOrHandler)triples, so the walk does not depend on the promise storage layout.Evaluate()meets anEvaluatingAsyncdependency, does not allocate on the JS heap, and is bounded at 4096 promises.Verification
JSTests/modules/dynamic-import-tla-cycle.js(direct, helper, helper-after-await,Promise.allshapes, with a watchdog that throws on deadlock) andJSTests/modules/dynamic-import-tla-siblings-wait.js(two dynamic imports sharing a genuinely async dependency must still wait for it) pass in all 11run-jsc-stress-testsmodule configurations. The jsc shell never passedreferrerAsyncOrder, so these exercise the walk alone. APromise.race([import()])negative control still deadlocks and trips the watchdog.JSTests/modules.yamlis unchanged by this branch (the pre-existing failures are error-message text and$vm-gated tests).child: functionandBOOT OK, exit 0;test/js/bun/resolve/dynamic-import-tla-cycle.test.tsin the Bun PR adds seven shapes that all hang on Bun 1.4.1 and pass here, and the six existing tests (including the [Cocoa] Pasting a Genmoji into Mail does not create a HEIC attachment WebKit/WebKit#30259 and Parse "dir" member of the web application manifest WebKit/WebKit#30634 sibling waits) still pass. The Bun PR needs thereferrerAsyncOrdercall-site removal to build against this revision.