JSModuleLoader: hand each synchronous load step to the registry entry, and register a fetch before its hook runs - #492
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 7688227 and 7e1556a7736fae3aa9b38e40feae08416391b76f. 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. WalkthroughThe module loader now uses ChangesModule loading completion
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed, on-topic, and explains the problem, fix, reentrant loading behavior, tests, and affected outcomes. It does not include the required Bugzilla URL, Reviewed by line, or the template-style changed-file list, but these omissions do not make the otherwise complete description largely incomplete. Warning Git: Failed to clone repository. Please run the Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Preview Builds
|
c2db3d9 to
8f93ef9
Compare
…of it keeps one record (WebKit bump for oven-sh/WebKit#492) A mock.module() factory object getter runs while the mocked module's record is created. If it require()s an ES module that imports the mocked specifier, the synchronous loader meets the same registry entry still Fetching and creates the module a second time. The outer call then overwrote the entry with its own record, so the CommonJS importer and the ES module held two different namespaces for one key, and a debug build tripped ASSERT(m_status == Status::Fetching) in ModuleRegistryEntry::fetchComplete. oven-sh/WebKit#492 makes the replay keep the first completion, as the import() path already did. Pin its preview build and add the two shapes (require() and import) to mock-module.test.ts.
8f93ef9 to
a2d2f8d
Compare
…of it keeps one record (WebKit bump for oven-sh/WebKit#492) A mock.module() factory object getter runs while the mocked module's record is created. If it require()s an ES module that imports the mocked specifier, the synchronous loader meets the same registry entry still Fetching and creates the module a second time. The outer call then overwrote the entry with its own record, so the CommonJS importer and the ES module held two different namespaces for one key, and a debug build tripped ASSERT(m_status == Status::Fetching) in ModuleRegistryEntry::fetchComplete. oven-sh/WebKit#492 makes the replay keep the first completion, as the import() path already did. Pin its preview build and add the two shapes (require() and import) to mock-module.test.ts.
a2d2f8d to
c9c461a
Compare
…of it keeps one record (WebKit bump for oven-sh/WebKit#492) A mock.module() factory object getter runs while the mocked module's record is created. If it require()s an ES module that imports the mocked specifier, the synchronous loader meets the same registry entry still Fetching and creates the module a second time. The outer call then overwrote the entry with its own record, so the CommonJS importer and the ES module held two different namespaces for one key, and a debug build tripped ASSERT(m_status == Status::Fetching) in ModuleRegistryEntry::fetchComplete. oven-sh/WebKit#492 makes the replay keep the first completion, as the import() path already did. Pin its preview build and add the two shapes (require() and import) to mock-module.test.ts.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
I re-reviewed after 7a2596e4 took the symmetric fetchPromise guard from the earlier note — no further issues found. The change now mirrors moduleRegistryFetchSettled/moduleRegistryModuleSettled (JSMicrotask.cpp:1008, 1045) exactly, including the setEvaluationError on rejection. Given this is re-entrancy handling in the core module-loader state machine, a human sign-off would still be worthwhile.
Checked: the new guards are strictly defensive — when the promise is still Pending behavior is unchanged, and when it isn't the double-settle that tripped ASSERT(m_status == Fetching) / ASSERT(status() == Pending) is skipped. The rejection branch's added setEvaluationError matches moduleRegistryModuleSettled at :1054. The fall-through to the loadPromise()/modulePromise path below is unaffected.
Extended reasoning...
Overview
The PR modifies one block inside JSModuleLoader::hostLoadImportedModule — the USE(BUN_JSC_ADDITIONS) synchronous-replay path taken when require(esm) hits an entry that's already Fetching. Three changes: (1) after makeModule() returns, re-check modulePromise->status() == Pending before calling fetchComplete()/fulfillPromise(); (2) after fetch() returns, re-check fetchPromise->status() == Pending before fulfillPromise()/rejectPromise() (this was my earlier note, taken in 7a2596e4); (3) on makeModule() rejection, call mapEntry->setEvaluationError() before rejecting, matching the normal-queue handler. A block comment documents why.
I confirmed against JSMicrotask.cpp that both guards and the setEvaluationError call are exact parity with moduleRegistryFetchSettled (:1008) and moduleRegistryModuleSettled (:1045, :1054), which are the normal-microtask-queue copies of these same steps.
Security risks
None identified. This is control-flow hardening in the module loader — no new inputs are trusted, no parsing, no allocation-shape changes. The change strictly narrows when settlement code runs.
Level of scrutiny
High. This is the JSC module loader, and the bug class is re-entrancy across nested loadModuleSync calls that share a ModuleRegistryEntry. The state machine involves three promises (fetchPromise, modulePromise, loadPromise), an entry status, and two microtask queues (normal + synchronous), and correctness depends on which of those a nested call may have advanced. The PR description traces both failure shapes precisely and the fix is the minimal defensive guard, but this is exactly the kind of subtle invariant a maintainer with full Bun/JSC-integration context should confirm.
Other factors
- My prior inline note (the sibling
fetchPromisebranch) was confirmed reachable by the author with a concrete Bun repro and fixed in 7a2596e4; the thread is resolved. - Verification is thorough: debug-asan rebuild, the Sentry-reported repro passes, and the Bun test suites for
mock/,resolve/builtin-esm-lazy-exports,plugin/plugins, andnode/modulepass. Companion tests land in oven-sh/bun#40170. - The change is strictly additive/defensive: every path that previously ran still runs when the promise is
Pending; only the double-settle is skipped. I don't see a way for this to regress the non-re-entrant case. - No CODEOWNERS file in this fork; no outstanding reviewer comments beyond the resolved thread.
7a2596e to
ba5c600
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
robobun
changed the title
|
Reworked into the structural version (one commit, ba5c6007), per the discussion on the Bun side: the two per-site checks are gone. |
…of it keeps one record (WebKit bump for oven-sh/WebKit#492) A mock.module() factory object getter runs while the mocked module's record is created. If it require()s an ES module that imports the mocked specifier, the synchronous loader meets the same registry entry still Fetching and creates the module a second time. The outer call then overwrote the entry with its own record, so the CommonJS importer and the ES module held two different namespaces for one key, and a debug build tripped ASSERT(m_status == Status::Fetching) in ModuleRegistryEntry::fetchComplete. oven-sh/WebKit#492 makes the replay keep the first completion, as the import() path already did. Pin its preview build and add the two shapes (require() and import) to mock-module.test.ts.
…loading keeps one record Two shapes. The pending fetch of k.ts belongs to a dependency edge (k.ts has a Fetching entry) or to a top-level import() of k.ts (no entry until the require() creates one). On an engine without oven-sh/WebKit#492 a debug build asserts in JSPromise::fulfillPromise for the first and on the New-status check in hostLoadImportedModule for the second.
ba5c600 to
6156712
Compare
There was a problem hiding this comment.
I re-reviewed after 61567122 and didn't find further issues — failFetch now guards the throw path the same way settleFetch/settleModule guard theirs, so all three inline steps honour first-completion-wins. Given this reworks re-entrancy handling in the module loader state machine, a human pass would still be worthwhile.
What was reviewed:
failFetchguardssetFetchError+rejectPromiseonFetching+ pending fetch promise — the concern from the previous review is addressed.settleModulemirrorsmoduleRegistryModuleSettled(same pending-modulePromise guard, samefetchComplete/setEvaluationErrorsplit).- The
New-path registration order change:ensureFetchPromise+setStatus(Fetching)beforefetch(), withpipeFromgated on the fetch promise still being pending; termination exception leaves the entry as-is.
Extended reasoning...
Overview
The PR reworks how hostLoadImportedModule's Bun-specific synchronous-drive path handles re-entrancy. Three new ModuleRegistryEntry methods (settleFetch, failFetch, settleModule) own applying each inline load step and check that the entry is still at that step before mutating it. The New path now registers the fetch (creates the fetch promise, sets Fetching) before calling the embedder hook, so a nested load during the hook joins a Fetching entry rather than a New one. All new code is under #if USE(BUN_JSC_ADDITIONS).
Security risks
None identified. This is internal state-machine sequencing in the module loader; no auth, crypto, or untrusted-input parsing is touched. The change narrows the set of reachable states rather than widening it.
Level of scrutiny
High. JSModuleLoader::hostLoadImportedModule is on the critical path for every ES module import in Bun, and the correctness argument depends on subtle re-entrancy reasoning across fetch()/makeModule() hook calls, promise settlement variants (fulfill vs fulfillPromise), and the synchronous microtask queue. The PR went through two prior review rounds here where re-entrancy edges were found and fixed (the pending-fetch-promise branch, then the throw path); each fix was applied structurally rather than patched at the call site.
Other factors
- Both prior inline concerns from this bot are resolved: the latest commit adds
failFetch, which gatessetFetchError+rejectPromiseonstatus == Fetching && fetchPromise pending, matching the guards on the other two steps. settleModuleis line-for-line consistent with the existingmoduleRegistryModuleSettledreaction handler in JSMicrotask.cpp, andsettleFetch's use of unguardedfulfillPromise/rejectPromiseis documented and matches the pre-existing rationale (fetch promise alreadypipeFrom'd).- Companion tests exist in oven-sh/bun#40170 covering the getter shape, the two load-hook shapes, and the throw-after-nested-complete shape; the PR description reports they pass on a debug-asan build linked against this change.
- No outstanding reviewer comments; both inline threads are marked resolved.
Given the criticality of the code path and the design-level nature of the change (registration order, ownership of step application), this warrants a human maintainer's confirmation rather than auto-approval, even though no defects were found on the current revision.
…of it keeps one record (WebKit bump for oven-sh/WebKit#492) A mock.module() factory object getter runs while the mocked module's record is created. If it require()s an ES module that imports the mocked specifier, the synchronous loader meets the same registry entry still Fetching and creates the module a second time. The outer call then overwrote the entry with its own record, so the CommonJS importer and the ES module held two different namespaces for one key, and a debug build tripped ASSERT(m_status == Status::Fetching) in ModuleRegistryEntry::fetchComplete. oven-sh/WebKit#492 makes the replay keep the first completion, as the import() path already did. Pin its preview build and add the two shapes (require() and import) to mock-module.test.ts.
…loading keeps one record Two shapes. The pending fetch of k.ts belongs to a dependency edge (k.ts has a Fetching entry) or to a top-level import() of k.ts (no entry until the require() creates one). On an engine without oven-sh/WebKit#492 a debug build asserts in JSPromise::fulfillPromise for the first and on the New-status check in hostLoadImportedModule for the second.
6156712 to
f10951d
Compare
There was a problem hiding this comment.
I re-reviewed after 61567122 and didn't find any new issues — both prior concerns (the pending-fetch re-issue branch and the unguarded setFetchError on the throw path) are now handled by settleFetch/failFetch, and the "first completion wins" rule holds on every path I traced. Given how subtle the re-entrancy semantics here are, a human look is still worthwhile before merge.
What was reviewed:
settleFetch/settleModule/failFetchguards vs. entry status and promise state — each re-reads state after the hook returns, no path writes from pre-call state.- The reordered
Newpath: entry isFetchingbeforefetch()runs,pipeFromis guarded onPending, and the throw path leaves a nested-completed entry intact. settleModule's rejected branch callingsetEvaluationError— matches whatmoduleRegistryModuleSettleddoes on the reaction path.
Extended reasoning...
Overview
The PR fixes a re-entrancy hazard in Bun's synchronous module loading (require(esm)): when hostLoadImportedModule drives a Fetching entry inline via fetch() or makeModule(), those calls run embedder hooks (plugin onLoad, synthetic-module getters) that can synchronously load the same key again and settle the step first. The old code then completed the entry a second time from state read before the hook call, producing two live SyntheticModuleRecords for one key and tripping debug asserts. The fix moves each step's completion into ModuleRegistryEntry methods (settleFetch, failFetch, settleModule) that apply only while the entry is still at that step, and reorders the New path so the entry is registered as Fetching before the fetch hook runs.
Files touched: JSModuleLoader.cpp (the hostLoadImportedModule sync-drive block and New path, all under USE(BUN_JSC_ADDITIONS)), ModuleRegistryEntry.{h,cpp} (three new guarded settlement helpers, also Bun-gated).
Security risks
None identified. This is internal module-loader state machine correctness, not an auth/crypto/input-validation surface. The change tightens invariants (one record per key) rather than loosening them.
Level of scrutiny
High. The module loader is a critical, hard-to-reason-about code path, and this change is specifically about re-entrancy ordering — the kind of logic where a missed edge produces user-visible namespace divergence or a stuck-Pending promise. The PR has already been through two rounds of review that each found a real edge (the pending-fetch re-issue branch, then the unguarded throw path); both are now fixed and covered by tests in oven-sh/bun#40170. The current revision looks internally consistent and the bug-hunting pass found nothing, but the subtlety warrants a human sign-off.
Other factors
- All changes are gated by
USE(BUN_JSC_ADDITIONS), so upstream JSC behavior is unchanged. - The author verified against Bun's module/plugin/resolve test suites on a debug-asan build and added regression tests for all three re-entrancy shapes in the companion Bun PR.
- Both of my prior inline comments are resolved and the fixes match what was suggested (guarded re-read after hook return;
failFetchchecksFetching+ pending fetch promise before mutating). settleModuleuses guardedfulfill/rejecton the module promise (matchingmoduleRegistryModuleSettled), whilesettleFetch/failFetchuse unguardedfulfillPromise/rejectPromiseafter an explicitPendingcheck becausepipeFromhas already setisFirstResolvingFunctionCalled— the asymmetry is deliberate and documented in the code.
…of it keeps one record (WebKit bump for oven-sh/WebKit#492) A mock.module() factory object getter runs while the mocked module's record is created. If it require()s an ES module that imports the mocked specifier, the synchronous loader meets the same registry entry still Fetching and creates the module a second time. The outer call then overwrote the entry with its own record, so the CommonJS importer and the ES module held two different namespaces for one key, and a debug build tripped ASSERT(m_status == Status::Fetching) in ModuleRegistryEntry::fetchComplete. oven-sh/WebKit#492 makes the replay keep the first completion, as the import() path already did. Pin its preview build and add the two shapes (require() and import) to mock-module.test.ts.
…loading keeps one record Two shapes. The pending fetch of k.ts belongs to a dependency edge (k.ts has a Fetching entry) or to a top-level import() of k.ts (no entry until the require() creates one). On an engine without oven-sh/WebKit#492 a debug build asserts in JSPromise::fulfillPromise for the first and on the New-status check in hostLoadImportedModule for the second.
f10951d to
7e1556a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
I reviewed the latest push and didn't find any bugs — both concerns I raised earlier (the unguarded fetch() re-issue in the Pending branch, and the unconditional setFetchError on the throw path) are now addressed by routing through settleFetch/failFetch, which check the entry is still at the step before applying. Given this is subtle reentrancy handling in the module loader's state machine, a human look is still worthwhile.
What was reviewed:
settleFetch/failFetch/settleModuleguards match the invariant stated ("first completion wins"): each checks the promise is stillPendingand the entry is still at the step before mutating.- The
New-path reorder (register →Fetching→ hook → conditionalpipeFrom) closes the window where a nested load could seeNewand re-fetch; thePendingguard beforepipeFromprevents double-settlement. - Throw handling now goes through
failFetch(state-guarded) and skips on termination exceptions, so aFetchedentry from a nested load is not stampedFetchFailed.
Extended reasoning...
Overview
This PR hardens JSModuleLoader::hostLoadImportedModule against reentrancy in Bun's synchronous require(esm) path. It refactors the inline fetch/makeModule settlement into three new ModuleRegistryEntry helpers (settleFetch, failFetch, settleModule) that check the entry is still at the expected step before applying a hook's result, and reorders the New-status branch to transition to Fetching before invoking the embedder fetch hook so nested loads join the pending entry instead of duplicating the fetch. All new code is gated behind USE(BUN_JSC_ADDITIONS).
Security risks
None identified. This is internal module-loader state-machine correctness with no auth, crypto, permission, or untrusted-input parsing surface. The change narrows behavior (more guards, fewer unconditional state writes) rather than widening it.
Level of scrutiny
High. The module loader's registry state machine is load-bearing for correctness of every ESM import in Bun, and reentrancy bugs here manifest as duplicate module records, assertion failures, or subtle identity mismatches (c.v !== n.vns in the PR's repro). The change is small in line count but dense in invariants — promise settlement ordering, guarded vs. unguarded fulfill/reject, pipeFrom semantics with isFirstResolvingFunctionCalled, and exception handling while state is partially advanced. This is exactly the kind of change where a second pair of eyes from someone who owns this code is valuable, even though the automated review found nothing wrong.
Other factors
Two earlier review rounds from this bot flagged (1) the sibling Pending branch's unguarded fulfillPromise after fetch() and (2) the unconditional setFetchError on the throw path. The latest commit (7e1556a7736f) addresses both by moving the state checks into the entry itself: settleFetch guards on m_status == Fetching && m_fetchPromise pending, and failFetch applies the same guard before setFetchError. The PR description references paired tests in oven-sh/bun#40170 covering the three reentrancy shapes, and the author reports local verification against Bun's module/plugin/resolve test suites. No CHANGES_REQUESTED reviews from other humans are outstanding in the timeline.
…of it keeps one record (WebKit bump for oven-sh/WebKit#492) A mock.module() factory object getter runs while the mocked module's record is created. If it require()s an ES module that imports the mocked specifier, the synchronous loader meets the same registry entry still Fetching and creates the module a second time. The outer call then overwrote the entry with its own record, so the CommonJS importer and the ES module held two different namespaces for one key, and a debug build tripped ASSERT(m_status == Status::Fetching) in ModuleRegistryEntry::fetchComplete. oven-sh/WebKit#492 makes the replay keep the first completion, as the import() path already did. Pin its preview build and add the two shapes (require() and import) to mock-module.test.ts.
…loading keeps one record Two shapes. The pending fetch of k.ts belongs to a dependency edge (k.ts has a Fetching entry) or to a top-level import() of k.ts (no entry until the require() creates one). On an engine without oven-sh/WebKit#492 a debug build asserts in JSPromise::fulfillPromise for the first and on the New-status check in hostLoadImportedModule for the second.
7e1556a to
5513a85
Compare
There was a problem hiding this comment.
I re-reviewed after 5513a854 and didn't find any bugs — both earlier inline points are now covered (settleFetch() re-checks state after the sync fetch() re-issue, and failFetch() guards setFetchError on the throw path). Given this reworks re-entrant state transitions in the module loader, a human look is still worthwhile.
What was reviewed:
settleFetch/failFetch/settleModuleguards vs.moduleRegistryFetchSettled/moduleRegistryModuleSettled— they check the same conditions and take the same actions.failFetch→setFetchError: the innerm_status == Newreject can't fire (guard requiresFetching), so no double-reject beforerejectPromise.- The
New-branch reordering:pipeFromis skipped when a nested load already settledfetchPromise; termination exceptions leave the entry untouched.
Extended reasoning...
Overview
The latest push replaces the earlier per-site Pending re-checks with three guarded helpers on ModuleRegistryEntry (settleFetch, failFetch, settleModule, all under USE(BUN_JSC_ADDITIONS)) and reorders the Status::New branch in hostLoadImportedModule so the entry is Fetching with a fetch promise before the embedder fetch() hook runs. Three files touched: JSModuleLoader.cpp, ModuleRegistryEntry.cpp, ModuleRegistryEntry.h. The upstream (non-Bun) path is preserved verbatim in the #else.
Security risks
None identified. This is internal module-loader state-machine bookkeeping; no new inputs are parsed, no auth/crypto/permission surface is touched. The uncheckedDowncast<AbstractModuleRecord> in settleModule mirrors the existing one in moduleRegistryModuleSettled and consumes the same makeModule() result, so it introduces no new type-confusion surface.
Level of scrutiny
High. This is JavaScriptCore's module loader, and the failure mode being fixed is a re-entrancy race that only shows up when embedder hooks recursively load the same key mid-step. The invariant ("first completion wins, decide nothing from state read before a hook call") now holds by construction on all three inline paths, and the helpers line up one-for-one with the async reaction handlers in JSMicrotask.cpp. That said, module-loader changes have long tails (link/instantiate ordering, error caching via FetchFailed/removeFailedFetchEntry, interaction with loadPromise caching), so a maintainer familiar with Bun's loadModuleSync should sign off.
Other factors
Both of my earlier inline comments are addressed in this revision: the sync fetch() re-issue now defers to settleFetch() (state re-checked after the hook), and the New-path throw handler now goes through failFetch() so setFetchError cannot stamp FetchFailed onto an entry a nested load already Fetched. The PR description references companion tests in oven-sh/bun#40170 covering the three re-entrancy shapes; those aren't in this repo, so verification here is by inspection. Bug hunt exited on dry_streak with no findings.
…of it keeps one record (WebKit bump for oven-sh/WebKit#492) A mock.module() factory object getter runs while the mocked module's record is created. If it require()s an ES module that imports the mocked specifier, the synchronous loader meets the same registry entry still Fetching and creates the module a second time. The outer call then overwrote the entry with its own record, so the CommonJS importer and the ES module held two different namespaces for one key, and a debug build tripped ASSERT(m_status == Status::Fetching) in ModuleRegistryEntry::fetchComplete. oven-sh/WebKit#492 makes the replay keep the first completion, as the import() path already did. Pin its preview build and add the two shapes (require() and import) to mock-module.test.ts.
…loading keeps one record Two shapes. The pending fetch of k.ts belongs to a dependency edge (k.ts has a Fetching entry) or to a top-level import() of k.ts (no entry until the require() creates one). On an engine without oven-sh/WebKit#492 a debug build asserts in JSPromise::fulfillPromise for the first and on the New-status check in hostLoadImportedModule for the second.
5513a85 to
f554fc2
Compare
There was a problem hiding this comment.
I reviewed the latest revision and didn't find any bugs — both points I raised earlier are now addressed by moving the state guards into ModuleRegistryEntry (settleFetch/failFetch/settleModule), so every hook return is checked against the entry's current step rather than per-site. Given this is re-entrancy handling in JSC's module loader on a path that now runs for every New-status entry under USE(BUN_JSC_ADDITIONS), a human sign-off is still worthwhile.
What was reviewed:
- Confirmed the sync
fetch()re-issue path (line 737) and theNew-path throw handler (line 795) now both go through guarded entry methods, resolving the two re-entrancy edges flagged on earlier revisions. - Checked calling
failFetch()with the hook's exception still live onscope—setFetchErrorandrejectPromiseon this path don't declare a throw scope or run JS, and them_status == Newbranch insetFetchErroris unreachable behindfailFetch'sFetchingguard. - Verified
settleModule'sfetchComplete()call is safe against a concurrently-FetchFailedentry via the existing early-return infetchComplete.
Extended reasoning...
Overview
This revision refactors the earlier per-site "still pending?" re-checks into three guarded methods on ModuleRegistryEntry (settleFetch, failFetch, settleModule) and reorders the New-status branch of hostLoadImportedModule so the entry is marked Fetching with a fetch promise before the embedder fetch() hook runs. All changes are under USE(BUN_JSC_ADDITIONS). The two inline threads I opened on earlier revisions — the unguarded fulfillPromise after the sync fetch() re-issue, and the unconditional setFetchError() on the throw path — are both structurally resolved: the guards now live in the entry, and every caller in hostLoadImportedModule delegates to them.
Security risks
None identified. This is control-flow hardening in the module loader; no new inputs are parsed, no auth/crypto/permissions are touched, and no user-controlled data reaches a new sink. The change reduces a state-machine hazard (double-settling promises / duplicate module records) rather than introducing one.
Level of scrutiny
High. Module loading is a core runtime path, and the New-status reordering changes the observable state a re-entrant embedder hook sees for every Bun module load, not just the require(esm) sync path. The guards themselves are straightforward and mirror the existing moduleRegistryFetchSettled/moduleRegistryModuleSettled reaction checks the PR description references, but the interaction between pipeFrom, isFirstResolvingFunctionCalled, and the unguarded fulfillPromise/rejectPromise is subtle enough that a maintainer familiar with the loader's promise wiring should confirm the Pending-only pipeFrom at line 799 is the right join point when the hook returns a still-pending promise after a nested load already settled the entry.
Other factors
The bug-hunt exit reason was dry_streak (ran to completion). One candidate — calling failFetch() while the hook's exception is still live — was investigated and ruled out: neither setFetchError (on the Fetching branch) nor rejectPromise declares a throw scope or calls back into JS, and the function returns nullptr immediately after, propagating the original exception. Tests exist in the companion oven-sh/bun#40170 rather than in this repo. No third-party reviewer has an outstanding CHANGES_REQUESTED; the resolved threads on my earlier comments correspond to real code changes in this revision.
…of it keeps one record (WebKit bump for oven-sh/WebKit#492) A mock.module() factory object getter runs while the mocked module's record is created. If it require()s an ES module that imports the mocked specifier, the synchronous loader meets the same registry entry still Fetching and creates the module a second time. The outer call then overwrote the entry with its own record, so the CommonJS importer and the ES module held two different namespaces for one key, and a debug build tripped ASSERT(m_status == Status::Fetching) in ModuleRegistryEntry::fetchComplete. oven-sh/WebKit#492 makes the replay keep the first completion, as the import() path already did. Pin its preview build and add the two shapes (require() and import) to mock-module.test.ts.
…loading keeps one record Two shapes. The pending fetch of k.ts belongs to a dependency edge (k.ts has a Fetching entry) or to a top-level import() of k.ts (no entry until the require() creates one). On an engine without oven-sh/WebKit#492 a debug build asserts in JSPromise::fulfillPromise for the first and on the New-status check in hostLoadImportedModule for the second.
f554fc2 to
5026256
Compare
…of it keeps one record (WebKit bump for oven-sh/WebKit#492) A mock.module() factory object getter runs while the mocked module's record is created. If it require()s an ES module that imports the mocked specifier, the synchronous loader meets the same registry entry still Fetching and creates the module a second time. The outer call then overwrote the entry with its own record, so the CommonJS importer and the ES module held two different namespaces for one key, and a debug build tripped ASSERT(m_status == Status::Fetching) in ModuleRegistryEntry::fetchComplete. oven-sh/WebKit#492 makes the replay keep the first completion, as the import() path already did. Pin its preview build and add the two shapes (require() and import) to mock-module.test.ts.
…loading keeps one record Two shapes. The pending fetch of k.ts belongs to a dependency edge (k.ts has a Fetching entry) or to a top-level import() of k.ts (no entry until the require() creates one). On an engine without oven-sh/WebKit#492 a debug build asserts in JSPromise::fulfillPromise for the first and on the New-status check in hostLoadImportedModule for the second.
5026256 to
a3bf055
Compare
…of it keeps one record (WebKit bump for oven-sh/WebKit#492) A mock.module() factory object getter runs while the mocked module's record is created. If it require()s an ES module that imports the mocked specifier, the synchronous loader meets the same registry entry still Fetching and creates the module a second time. The outer call then overwrote the entry with its own record, so the CommonJS importer and the ES module held two different namespaces for one key, and a debug build tripped ASSERT(m_status == Status::Fetching) in ModuleRegistryEntry::fetchComplete. oven-sh/WebKit#492 makes the replay keep the first completion, as the import() path already did. Pin its preview build and add the two shapes (require() and import) to mock-module.test.ts.
…loading keeps one record Two shapes. The pending fetch of k.ts belongs to a dependency edge (k.ts has a Fetching entry) or to a top-level import() of k.ts (no entry until the require() creates one). On an engine without oven-sh/WebKit#492 a debug build asserts in JSPromise::fulfillPromise for the first and on the New-status check in hostLoadImportedModule for the second.
…loading keeps one record Two shapes. The pending fetch of k.ts belongs to a dependency edge (k.ts has a Fetching entry) or to a top-level import() of k.ts (no entry until the require() creates one). On an engine without oven-sh/WebKit#492 a debug build asserts in JSPromise::fulfillPromise for the first and on the New-status check in hostLoadImportedModule for the second.
a3bf055 to
49e04fe
Compare
49e04fe to
78c1adc
Compare
…of it keeps one record (WebKit bump for oven-sh/WebKit#492) A mock.module() factory object getter runs while the mocked module's record is created. If it require()s an ES module that imports the mocked specifier, the synchronous loader meets the same registry entry still Fetching and creates the module a second time. The outer call then overwrote the entry with its own record, so the CommonJS importer and the ES module held two different namespaces for one key, and a debug build tripped ASSERT(m_status == Status::Fetching) in ModuleRegistryEntry::fetchComplete. oven-sh/WebKit#492 makes the replay keep the first completion, as the import() path already did. Pin its preview build and add the two shapes (require() and import) to mock-module.test.ts.
…loading keeps one record Two shapes. The pending fetch of k.ts belongs to a dependency edge (k.ts has a Fetching entry) or to a top-level import() of k.ts (no entry until the require() creates one). On an engine without oven-sh/WebKit#492 a debug build asserts in JSPromise::fulfillPromise for the first and on the New-status check in hostLoadImportedModule for the second.
…, and register a fetch before its hook runs Bun's require(esm) drives the module loader synchronously (loadModuleSync). In hostLoadImportedModule it meets entries that an async graph already started and advances them inline: it re-issues the embedder's fetch() for a pending fetch, or runs makeModule() for a fetched source, instead of waiting for the ModuleRegistryFetchSettled / ModuleSettled reactions. Both calls run embedder code. fetch() runs a plugin's load hook or a module mock's factory; makeModule() runs a synthetic module's generator, which reads a user object's getters. That code can load the same key again before the call returns. The nested load reaches the same entry, takes the same branch and settles the step first. The inline code then completed the entry a second time from the state it had read before the call: fetchComplete() on a Fetched entry, fulfillPromise() on a settled promise (both debug assertions), and for makeModule() a second record for one key. The registry held the outer record, every importer held the inner one (Bun: oven-sh/bun#40170, Sentry BUN-4QBM). The same hazard sat in the New path: fetch() ran before the entry was Fetching, so a nested load found a New entry (ASSERT(status != New)), fetched a second time, and the outer return path reset a Fetched entry to Fetching. Make the steps belong to ModuleRegistryEntry. settleFetch() and settleModule() take the promise a hook call produced and apply the step only while the entry is still at it; they return whether they did. hostLoadImportedModule no longer decides anything from state read before a hook call. The reactions already have this shape (moduleRegistryFetchSettled / moduleRegistryModuleSettled), so the first completion now wins on every path. Register a fetch before its hook runs: create the fetch promise and move the entry to Fetching first, pipe the hook's promise in afterwards only if nothing settled the fetch promise meanwhile, and fail the entry if the hook throws. A nested load during the hook now sees a Fetching entry with a pending fetch promise, which is the state the synchronous path knows how to drive.
78c1adc to
4783710
Compare
…of it keeps one record (WebKit bump for oven-sh/WebKit#492) A mock.module() factory object getter runs while the mocked module's record is created. If it require()s an ES module that imports the mocked specifier, the synchronous loader meets the same registry entry still Fetching and creates the module a second time. The outer call then overwrote the entry with its own record, so the CommonJS importer and the ES module held two different namespaces for one key, and a debug build tripped ASSERT(m_status == Status::Fetching) in ModuleRegistryEntry::fetchComplete. oven-sh/WebKit#492 makes the replay keep the first completion, as the import() path already did. Pin its preview build and add the two shapes (require() and import) to mock-module.test.ts.
…loading keeps one record Two shapes. The pending fetch of k.ts belongs to a dependency edge (k.ts has a Fetching entry) or to a top-level import() of k.ts (no entry until the require() creates one). On an engine without oven-sh/WebKit#492 a debug build asserts in JSPromise::fulfillPromise for the first and on the New-status check in hostLoadImportedModule for the second.
1 participant
Problem
SyntheticModuleRecords: a Bunmock.module()factory getterrequire()s an ES module which imports the mocked key, and the two importers get different namespaces. A debug build tripsASSERT(m_status == Status::Fetching)inModuleRegistryEntry::fetchComplete. Sentry BUN-4QBM is on this path.require(esm)runs the loader synchronously, sohostLoadImportedModuleadvances an entry inline withfetch()andmakeModule(). Both run embedder code that can load the same key again before the call returns. The nested load settles the step first, and the inline code then completed the entry again from state read before the call. TheNewpath shared the shape:fetch()ran before the entry wasFetching.Fix
ModuleRegistryEntry::settleFetchandsettleModuletake the promise a hook call produced and apply the step only while the entry is still at it.hostLoadImportedModuledecides nothing from state read before a hook call, the same shape asmoduleRegistryFetchSettledandmoduleRegistryModuleSettled. The first completion wins on every path by construction.Fetching, the hook's promise piped in afterwards only if the fetch promise is still pending. If the hook throws,failFetchfails the entry, again only while it is still at that step, instead of leaving itNew.ceb9f90fb7, the current base, and pass with this change.Background
ModuleRegistryEntrygoesNew->Fetching->Fetched.fetchComplete()stores the record, and the module promise settles with it.require(esm)isloadModuleSync: promise reactions drain in a per-call queue before the call returns, andhostLoadImportedModuledrives aFetchingentry forward inline.SyntheticModuleRecordis built bySyntheticSourceProvider::generate; for Bun's object modules that reads every property of a user object, so user getters run insidemakeModule.Notes
History: the first version of this PR added a "module promise still pending" check after
makeModule()and, after review, the same check after the re-issuedfetch(). This version replaces both per-site checks with the entry methods and the registration order, so the property holds for every step rather than for the two that had been hit.Repro for the getter shape (Bun 1.4.0,
bun test, preload):Without the change:
c.g === "outer-value",n.vg === "inner-value",c.v !== n.vns. With it: both"inner-value", one namespace. Sequence: the outerrequire("virt")replaysmakeModule("virt"); the generator calls the getter, which loadsn.ts;n.tsimportsvirt, sohostLoadImportedModule(n, "virt")finds the entryFetchingwith a fulfilled fetch promise and a pending module promise, and replaysmakeModuleagain: record V2,fetchComplete(V2), module promise fulfilled.n.tslinks against V2. The outermakeModulereturns V1 and the old code calledfetchComplete(V1)andfulfillPromise(V1)on the settled promise.The load hook shapes:
import("./a.ts")wherea.tsimportsk.tsandk.ts'sonLoadreturns a promise that never settles, sok.tsisFetchingwith a pending fetch promise.require("./m.ts")(importsk.ts) re-issues the fetch synchronously; thatonLoadcall doesrequire("./n.ts")(importsk.ts), which re-issues it a third time and fulfills the fetch promise. The outer call returned tofetchPromise->fulfillPromise()on a settled promise (ASSERT(status() == Pending), JSPromise.cpp). Withimport("./k.ts")instead ofa.ts, no entry exists whenrequire("./m.ts")runs (the top-level load registers its entry only once its fetch settles), sohostLoadImportedModulecreated one and calledfetch()while it was stillNew; the nested load then hitASSERT(mapEntry->status() != New), release fetched a second time and the outer return path setFetchingon theFetchedentry. Release builds were otherwise unaffected in both shapes because the reactions had already fired once.settleModuleuses the guardedfulfill/rejecton the module promise, asmoduleRegistryModuleSettleddoes;settleFetchandfailFetchusefulfillPromise/rejectPromisebecause a piped fetch promise hasisFirstResolvingFunctionCalledset. TheNewpath pipes only if the fetch promise is still pending, for the same reason. Iffetch()throws (not a termination),failFetchstores the error and rejects the fetch promise, but only while the entry is stillFetchingwith a pending fetch promise: a hook thatrequire()s an importer of the module and then throws leaves the record the nested load made in place, and the outerrequire()throws the hook's error. Before, the entry stayedNewin the map.The top-level
loadModulestill registers its entry only when its fetch settles (provideFetchinmoduleLoadTopSettled), which is upstream's registration point and the subject of the FIXME in Bun'sBun__onFulfillAsyncModule. Not changed here.Local verification:
UnifiedSource-runtime-26and-33(the bundles withJSModuleLoader.cppandModuleRegistryEntry.cpp) recompiled from this branch with the compile command theautobuild-preview-pr-492-c9c461a9linux debug-asan tarball ships, swapped into itslibJavaScriptCore.a, and a Bun debug build linked against it. Against it Bun'stest/js/bun/test/mock/,test/js/bun/plugin/,test/js/bun/resolve/(39 files),test/js/node/module/andtest/cli/run/require-cache.test.tspass except the RSS leak tests andload the same empty JS file 2000 times, which time out on this machine on the unmodified engine too.