Fix duplicate fetchComplete when require() re-enters a shared dep's fetch#223
Fix duplicate fetchComplete when require() re-enters a shared dep's fetch#223robobun wants to merge 1 commit into
Conversation
…eady settled When require() of an ESM graph re-enters the module loader for a module the outer async graph is already fetching, the BUN_JSC_ADDITIONS inline sync path in hostLoadImportedModule calls fetchComplete() and fulfills modulePromise directly. The ModuleRegistryModuleSettled reaction from the original async path is still queued and, when it later fires, calls fetchComplete() a second time — tripping ASSERT(m_status == Fetching) in debug builds and overwriting the entry's record with a duplicate parse in release. Mirror the guard already present in moduleRegistryFetchSettled: skip when modulePromise is no longer Pending. The inline path has already produced a valid record; this queued copy is redundant.
|
Duplicate of #217. Same fix, same reasoning; closing this one. |
|
Duplicate of #217 — same fix. Different tree of reproducers (this one came via bun#30493, diamond ESM through a barrel where shared.js imports node:path; #217 came via bun#30281, react + @mui/material). Same underlying bug: |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
WalkthroughThis PR adds a guard check in ChangesModule Promise Synchronous Re-entry Guard
Comment |
Fixes oven-sh/bun#30493.
Repro
require('./app.js')whereapp.jsdirectly importsshared.jsand also transitively througha.js/b.js→barrel.js→shared.js, whereshared.jsimports a synthetic builtin (e.g.import path from 'path'). On Linux debug:ASSERT(m_status == Status::Fetching)inModuleRegistryEntry::fetchComplete. On Darwin: deadlock.Root cause
When
require()of an ESM graph reaches a module whose status isFetching,hostLoadImportedModule'sBUN_JSC_ADDITIONSinline path (added to drive the fetch/makeModule pipeline synchronously) callsmakeModule, thenmapEntry->fetchComplete(record)andmodulePromise->fulfillPromise(record)directly.The original async path had already queued
moduleRegistryFetchSettledfor the same entry. That handler checksmodulePromise->status() != Pendingand bails — good. But it runsmakeModulebefore the check and attachesModuleRegistryModuleSettledto the newmakeModulePromise, which still fires as a separate microtask.moduleRegistryModuleSettledhas no corresponding bail-out: it callsentry->fetchComplete(record)a second time (with a different record, from the redundant parse). In debug the assertion trips; in release the entry's record silently gets swapped to a stale duplicate that other modules never linked against.Actually the real sequence in the repro: the inline sync path is the source of the duplicate
makeModule. WhenhostLoadImportedModuleenters the inline branch atJSModuleLoader.cpp:712(fetchPromisealready Fulfilled,modulePromisestill Pending), it callsmakeModule→fetchCompleteinline. ThemoduleRegistryFetchSettledreaction that was queued earlier by the async pipeline already made its ownmakeModulePromiseand attachedModuleRegistryModuleSettled. That settled callback then runs and triesfetchCompleteon an already-Fetched entry.Fix
Mirror the
modulePromise->status() != Pendingbail frommoduleRegistryFetchSettledintomoduleRegistryModuleSettled. The inline path has already produced a valid record and resolved the module promise; the queued copy's record is an unreachable duplicate parse.Gated on
USE(BUN_JSC_ADDITIONS)sinceVM::m_synchronousModuleQueueis Bun-only.Verification
ASSERT(m_status == Status::Fetching)with current WebKit HEAD + Bun built locally.test/regression/issue/30493.test.ts(in the accompanying Bun PR) passes.test/js/bun/resolve/require-esm-*.test.tsandtest/js/node/module/require-extensions.test.tsremain green.