fix: prevent double-fulfill when import() and require() race on same ESM module#27323
Open
dylan-conway wants to merge 1 commit intomainfrom
Open
fix: prevent double-fulfill when import() and require() race on same ESM module#27323dylan-conway wants to merge 1 commit intomainfrom
dylan-conway wants to merge 1 commit intomainfrom
Conversation
…e() race on the same ESM module When `import()` starts fetching an ESM module, JSC's `requestFetch` creates `entry.fetch` as a `.then()` chained promise with a pending microtask reaction. If `require()` then loads the same module, `fetchCommonJSModuleNonBuiltin` called `provideFetch` which fulfilled that same promise directly via `fulfillFetch`. When the pending microtask reaction later fired during microtask draining, it attempted to resolve the already-fulfilled promise, triggering `ASSERT(status() == Status::Pending)` in `JSPromise::fulfillPromise`. This was exposed by commit 21c3439 which added `Loader.registry.$delete(id)` to the require() error path. The underlying issue is that Bun was calling `provideFetch` on a module whose fetch pipeline was already owned by import(). The fix has two parts: 1. **ModuleLoader.cpp**: Extend the existing `hasAlreadyLoadedESMVersionSoWeShouldntTranspileItTwice` guard to also detect when `import()` has a fetch in progress (entry exists with a fetch promise but state is still `Fetch`). This prevents `fetchCommonJSModuleNonBuiltin` from calling `provideFetch` on that entry. 2. **CommonJS.ts**: In `loadEsmIntoCjs`, clear `entry.fetch` before calling `$fulfillModuleSync`, so that `provideFetch` → `fulfillFetch` creates a fresh promise instead of fulfilling import()'s pending chained promise. Closes #12910 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Collaborator
Contributor
|
No actionable comments were generated in the recent review. 🎉 WalkthroughModified module loading logic to improve handling of in-progress fetch promises. Extended Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Contributor
|
✅ 88c52 — Looks good! Reviewed 2 files across |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a
ASSERT(status() == Status::Pending)crash inJSPromise::fulfillPromisewhenimport()andrequire()target the same ESM module that throws during evaluation.Root cause: When
import()starts fetching an ESM module, JSC'srequestFetchcreatesentry.fetchas a.then()chained promise with a pending microtask reaction. Ifrequire()then loads the same module,fetchCommonJSModuleNonBuiltincalledprovideFetchwhich fulfilled that same promise directly. When the pending microtask reaction later fired, it attempted to resolve the already-fulfilled promise, crashing on the assertion.This was exposed by #27288 which added
Loader.registry.$delete(id)to the require() error path.Fix (two parts):
hasAlreadyLoadedESMVersionSoWeShouldntTranspileItTwiceguard to also detect whenimport()has a fetch in progress (entry exists with a fetch promise but state is stillFetch). This preventsfetchCommonJSModuleNonBuiltinfrom callingprovideFetchon that entry.loadEsmIntoCjs, clearentry.fetchbefore calling$fulfillModuleSync, so thatprovideFetch→fulfillFetchcreates a fresh promise instead of fulfilling import()'s pending chained promise.Test plan
test/regression/issue/12910/12910.test.tspassestest/regression/issue/27287.test.tspasses (the related regression test from fix: clean up ESM registry when require() of ESM module fails #27288)provideFetchCloses #12910
🤖 Generated with Claude Code