Module loader: follow a for-await step in the import-promise walk - #548
Conversation
for-await over sync values that are promises awaits each value through the async-from-sync iterator. Its pending step is an AsyncFromSyncIteratorContinue/Done reaction whose context is the iterator; the iterator holds the result promise the step settles, or the driver it resumes directly. Follow that target like an await in an async function, so a loader that does for await (const ns of lazyImports()) ... inside the suspended module's await chain is recognised. JSAsyncFromSyncIterator gains a const target() accessor. The JSTests fixture adds the shape; the imports are created lazily by a sync generator because an import() created before the loop reaches it can evaluate before anything awaits it.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It extends the Bun-specific TLA import-promise walk in module evaluation, and both Source/JavaScriptCore and JSTests are CODEOWNERS-covered, so a human look is still worthwhile.
What was reviewed:
followPromiseOrDriverrefactor: it is a superset of the three prior case bodies (adds aJSPromisebranch to the resume paths), so the walk only widens — no case lost coverage, andfoundstill cannot flip true→false becausevisitReactionreturns!found.- New
AsyncFromSyncIterator{Continue,Done}handling:unwrapContextnull-guarded beforedynamicDowncast, andtarget()returning null short-circuits infollowPromiseOrDriver. JSAsyncFromSyncIterator::target(): read-only overm_target.pointer()(unlikeextractTarget()it doesn't clear), correctly gated byUSE(BUN_JSC_ADDITIONS), as is the new include.- Test fixtures follow the existing
entry-*/child-*pattern; the harness count and scenario list stay in sync at 5.
Extended reasoning...
Overview
The PR extends importPromiseGatesAsyncDependency in Source/JavaScriptCore/runtime/AbstractModuleRecord.cpp — the Bun-only heuristic that walks the reaction graph off a dynamic import() promise to detect when a suspended TLA module is transitively waiting on itself. Two new InternalMicrotask reaction kinds (AsyncFromSyncIteratorContinue/Done) are handled by reading the async-from-sync iterator's pending target and following it. The three existing resume cases are folded into a shared followPromiseOrDriver lambda. JSAsyncFromSyncIterator.h gains a read-only target() accessor under USE(BUN_JSC_ADDITIONS). New JSTests fixtures add a for-await-over-sync-generator scenario, and the top-level test bumps its expected settle count from 4 to 5.
Security risks
None identified. All code is inside #if USE(BUN_JSC_ADDITIONS) and only reads existing GC-managed fields via dynamicDowncast with null guards, matching the surrounding pattern. The new accessor is a plain const read of a CompactPointerTuple pointer with no write barrier or lifetime implications. There is no untrusted input parsing, no allocation, and no change to module evaluation ordering outside the existing heuristic's true/false result.
Level of scrutiny
Moderate-to-high. The diff is small and mechanical, but it lives in InnerModuleEvaluation's cycle-vs-deadlock heuristic, where widening the walk changes when step 12.b.v is skipped. The followPromiseOrDriver consolidation adds a JSPromise branch that the prior AsyncFunctionResume/AsyncModuleExecutionResume/AsyncGeneratorDriverResume bodies did not have; this only broadens what the walk can reach and cannot cause it to miss a previously-detected dependency, but confirming that broadening is intentional (and cannot produce a false positive that skips a wait it shouldn't) is best done by someone who owns this heuristic. Both changed source trees are covered by .github/CODEOWNERS (/Source/JavaScriptCore and /JSTests → WebKit/jsc-reviewers), which per policy means I should not auto-approve.
Other factors
The change ships with a regression test that exercises the exact shape (sync generator yielding import() consumed by for await), and the PR description states it was verified to hang without the fix. The test follows the existing dynamic-import-tla-cycle fixture pattern and updates the settle count consistently. The include of JSAsyncFromSyncIterator.h is placed inside the existing #if USE(BUN_JSC_ADDITIONS) include block, so non-Bun builds are unaffected.
|
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 (6)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughChangesDynamic import TLA cycle handling
Merge Risk: ⚪ Minimal · up to The PR extends the import-promise walk through async-from-sync iterator steps and adds regression coverage; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the problem, implementation, test changes, and verification. However, it does not include the required Bugzilla bug title and URL,
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 |
Preview Builds
|
…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.
main (#548) folds the async-driver reactions into followPromiseOrDriver and follows for-await steps; the instance-aware driver mapping (moduleForDriver) now lives there.
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
for awaitstep. A loader that doesfor await (const ns of lazyImports())inside the suspended module's await chain still deadlocks: the walk reaches theAsyncFromSyncIteratorContinuereaction on the import promise and ends there, so the chunk waits on the suspended module per 12.b.v and the module waits on the loop.for awaitover sync values that are promises awaits each value through the async-from-sync iterator. The iterator holds what the pending step settles or resumes (m_target): the result promise ofnext(), or the driver it resumes directly. There was no read accessor for it.Fix
JSAsyncFromSyncIterator::target()exposes the pending target without clearing it.AsyncFromSyncIteratorContinue/Donethroughtarget(): a promise is queued, an async function generator contributes its own promise, a module body is checked against the dependency. The same helper servesAsyncFunctionResume,AsyncModuleExecutionResumeandAsyncGeneratorDriverResume.JSTests/modules/dynamic-import-tla-cycle.jsgains thefor-awaitshape. The fixture creates the import lazily from a sync generator on purpose: animport()created before the loop reaches it can evaluate before anything awaits it, and then no reaction shows that the entry waits for it. That ordering is inherent to a snapshot of the reaction graph and is the same limit asconst p = import(x); await other; await p.Verification
for await (const m of plugins())wherepluginsis a sync generator yieldingimport()calls, each plugin importing the entry back, prints every plugin and exits 0 (hangs without it). The remainingtest/js/bun/resolve/dynamic-import-tla-cycle.test.tsshapes still pass.This was pushed to the #543 branch after that PR merged, so it is re-submitted here on top of
main(including 2718370).