diff --git a/src/jsc/bindings/ModuleLoader.cpp b/src/jsc/bindings/ModuleLoader.cpp index 7c5ba80bb6a3..73191162ab82 100644 --- a/src/jsc/bindings/ModuleLoader.cpp +++ b/src/jsc/bindings/ModuleLoader.cpp @@ -476,21 +476,9 @@ extern "C" void Bun__onFulfillAsyncModule( auto* specifierValue = Bun::toJS(globalObject, *specifier); RETURN_IF_EXCEPTION(scope, ); - // The new C++ module loader does not create a registry entry until *after* - // this fetch promise resolves (provideFetch runs inside the - // ModuleLoadTopSettled microtask). Two concurrent dynamic imports of the - // same key therefore each get their own embedder fetch promise, and the - // loser of that race must still resolve so its loadModule chain can reach - // the (idempotent) provideFetch and reuse the already-loaded record. - // The old #6946/#12910 short-circuit was for the JS loader's *shared* - // entry.fetch promise; under the new loader returning here would strand - // the loser's promise pending forever. - // - // FIXME(module-loader): the loser still re-transpiled the file. The right - // fix is for JSModuleLoader::loadModule to ensureRegistered() *before* - // calling fetch so concurrent importers share the entry's fetchPromise - // instead of each round-tripping through the embedder. - + // Always settle: moduleLoaderFetch handed this promise to the module loader + // and to every importer coalesced onto it, so nothing else resolves it. The + // old #6946/#12910 short-circuit was for the JS loader's shared entry.fetch. if (res->result.value.isCommonJSModule) { auto created = Bun::createCommonJSModule(globalObject, specifierValue, res->result.value); EXCEPTION_ASSERT(created.has_value() == !scope.exception()); diff --git a/src/jsc/bindings/ZigGlobalObject.cpp b/src/jsc/bindings/ZigGlobalObject.cpp index d333318de831..ce97f3938a7f 100644 --- a/src/jsc/bindings/ZigGlobalObject.cpp +++ b/src/jsc/bindings/ZigGlobalObject.cpp @@ -2412,6 +2412,12 @@ void GlobalObject::finishCreation(VM& vm) init.set(map); }); + m_inFlightModuleFetches.initLater( + [](const JSC::LazyProperty::Initializer& init) { + auto* map = JSC::JSMap::create(init.vm, init.owner->mapStructure()); + init.set(map); + }); + m_requireFunctionUnbound.initLater( [](const JSC::LazyProperty::Initializer& init) { init.set( @@ -3326,6 +3332,8 @@ void GlobalObject::reload() } this->requireMap()->clear(this); RETURN_IF_EXCEPTION(scope, ); + this->clearInFlightModuleFetches(); + RETURN_IF_EXCEPTION(scope, ); // If we run the GC every time, we will never get the SourceProvider cache hit. // So we run the GC every other time. @@ -3575,6 +3583,58 @@ static JSC::JSPromise* resolvedInternalPromise(JSC::JSGlobalObject* globalObject return promise; } +// Mirrors the module registry key: (specifier, fetch type, host-defined import +// type). Length-prefixed so no triple can alias another's flattened key. Coarser +// keying would share one JSSourceCode across two registry entries. The generation +// scopes the key to one clearInFlightModuleFetches() epoch. +static String inFlightModuleFetchKey(unsigned generation, const String& moduleKey, ScriptFetchParameters::Type type, const String& typeAttribute) +{ + return makeString(generation, ':', static_cast(type), ':', typeAttribute.length(), ':', typeAttribute, moduleKey); +} + +// Passed as both the fulfill and the reject handler, with the flattened fetch +// key as the reaction's user context (argument 1). +JSC_DEFINE_HOST_FUNCTION(jsFunctionInFlightModuleFetchSettled, (JSC::JSGlobalObject * globalObject, JSC::CallFrame* callFrame)) +{ + auto& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + auto* thisObject = defaultGlobalObject(globalObject); + thisObject->inFlightModuleFetches()->remove(globalObject, callFrame->argument(1)); + RETURN_IF_EXCEPTION(scope, {}); + return JSValue::encode(jsUndefined()); +} + +JSC::JSPromise* GlobalObject::inFlightModuleFetch(JSC::JSString* fetchKey) +{ + // A miss (and a pending exception) yields jsUndefined(), never an empty + // JSValue, so the downcast is the miss check. Callers check the exception. + return dynamicDowncast(inFlightModuleFetches()->get(this, fetchKey)); +} + +void GlobalObject::trackInFlightModuleFetch(JSC::JSString* fetchKey, JSC::JSPromise* promise) +{ + auto scope = DECLARE_THROW_SCOPE(vm()); + inFlightModuleFetches()->set(this, fetchKey, promise); + RETURN_IF_EXCEPTION(scope, void()); + + // Attached before the loader attaches its own reaction, so the entry is gone + // before the module registry entry that supersedes it is created. + JSFunction* onSettled = thenable(jsFunctionInFlightModuleFetchSettled); + scope.release(); + promise->performPromiseThenWithContext(vm(), this, onSettled, onSettled, jsUndefined(), fetchKey); +} + +void GlobalObject::clearInFlightModuleFetches() +{ + // Clearing the map cannot detach the settle reactions already attached to the + // promises it held. Retire the generation so a pre-clear fetch that settles + // later removes its own (now absent) key instead of a newer fetch's entry. + inFlightModuleFetchGeneration++; + if (!m_inFlightModuleFetches.isInitialized()) + return; + inFlightModuleFetches()->clear(this); +} + JSC::JSPromise* GlobalObject::moduleLoaderFetch(JSGlobalObject* globalObject, JSModuleLoader* loader, JSValue key, RefPtr parameters, RefPtr) @@ -3636,8 +3696,19 @@ JSC::JSPromise* GlobalObject::moduleLoaderFetch(JSGlobalObject* globalObject, return rejectedInternalPromise(globalObject, result ? result : JSC::jsUndefined()); } + // JSC registers the module registry entry (whose fetch promise later + // importers share) only once this fetch settles, so coalesce until then: + // concurrent dynamic imports of one specifier run the loader exactly once. + auto* zigGlobalObject = static_cast(globalObject); + auto fetchType = parameters ? parameters->type() : ScriptFetchParameters::Type::JavaScript; + JSString* fetchKey = jsString(vm, inFlightModuleFetchKey(zigGlobalObject->inFlightModuleFetchGeneration, moduleKey, fetchType, typeAttributeString)); + JSC::JSPromise* inFlight = zigGlobalObject->inFlightModuleFetch(fetchKey); + RETURN_IF_EXCEPTION(scope, rejectedInternalPromise(globalObject, scope.exception()->value())); + if (inFlight) + return inFlight; + JSValue result = Bun::fetchESMSourceCodeAsync( - static_cast(globalObject), + zigGlobalObject, moduleKeyJS, &res, &moduleKeyBun, @@ -3647,6 +3718,8 @@ JSC::JSPromise* GlobalObject::moduleLoaderFetch(JSGlobalObject* globalObject, RETURN_IF_EXCEPTION(scope, rejectedInternalPromise(globalObject, scope.exception()->value())); ASSERT(result); if (auto* promise = dynamicDowncast(result)) { + zigGlobalObject->trackInFlightModuleFetch(fetchKey, promise); + RETURN_IF_EXCEPTION(scope, rejectedInternalPromise(globalObject, scope.exception()->value())); return promise; } return rejectedInternalPromise(globalObject, result); @@ -3871,6 +3944,8 @@ GlobalObject::PromiseFunctions GlobalObject::promiseHandlerID(Zig::FFIFunction h return GlobalObject::PromiseFunctions::Bun__HTTPRequestContextDebugH3__onResolve; } else if (handler == Bun__HTTPRequestContextDebugH3__onResolveStream) { return GlobalObject::PromiseFunctions::Bun__HTTPRequestContextDebugH3__onResolveStream; + } else if (handler == jsFunctionInFlightModuleFetchSettled) { + return GlobalObject::PromiseFunctions::jsFunctionInFlightModuleFetchSettled; } else { RELEASE_ASSERT_NOT_REACHED(); } diff --git a/src/jsc/bindings/ZigGlobalObject.h b/src/jsc/bindings/ZigGlobalObject.h index fb4271ed1d0f..459c9d143009 100644 --- a/src/jsc/bindings/ZigGlobalObject.h +++ b/src/jsc/bindings/ZigGlobalObject.h @@ -279,6 +279,19 @@ class GlobalObject : public Bun::GlobalScope { // moduleLoader()->registryEntry(key) / moduleMap() / removeEntry(key) / // clearAll() instead. + // Embedder module fetches that have not settled yet, keyed by module key + + // type attribute. JSC only registers a module registry entry once the first + // fetch settles, so without this every concurrent import() of one specifier + // re-runs the loader. Entries are dropped when the fetch settles. + JSC::JSMap* inFlightModuleFetches() const { return m_inFlightModuleFetches.getInitializedOnMainThread(this); } + JSC::JSPromise* inFlightModuleFetch(JSC::JSString* fetchKey); + void trackInFlightModuleFetch(JSC::JSString* fetchKey, JSC::JSPromise*); + void clearInFlightModuleFetches(); + // Part of the fetch key, retired by clearInFlightModuleFetches(). A fetch left + // in flight across a reload settles into a reaction that removes its own key, + // which by then must no longer name a live entry. + unsigned inFlightModuleFetchGeneration = 0; + JSC::Structure* callSiteStructure() const { return m_callSiteStructure.getInitializedOnMainThread(this); } JSC::JSObject* performanceObject() const { return m_performanceObject.getInitializedOnMainThread(this); } @@ -412,8 +425,9 @@ class GlobalObject : public Bun::GlobalScope { Bun__HTTPRequestContextDebugH3__onRejectStream, Bun__HTTPRequestContextDebugH3__onResolve, Bun__HTTPRequestContextDebugH3__onResolveStream, + jsFunctionInFlightModuleFetchSettled, }; - static constexpr size_t promiseFunctionsSize = 42; + static constexpr size_t promiseFunctionsSize = 43; static PromiseFunctions promiseHandlerID(SYSV_ABI EncodedJSValue (*handler)(JSC::JSGlobalObject* arg0, JSC::CallFrame* arg1)); @@ -595,6 +609,7 @@ class GlobalObject : public Bun::GlobalScope { V(private, LazyPropertyOfGlobalObject, m_wasmStreamingConsumeStreamFunction) \ V(private, LazyPropertyOfGlobalObject, m_streamsRuntime) \ V(private, LazyPropertyOfGlobalObject, m_requireMap) \ + V(private, LazyPropertyOfGlobalObject, m_inFlightModuleFetches) \ V(private, LazyPropertyOfGlobalObject, m_JSArrayBufferControllerPrototype) \ V(private, LazyPropertyOfGlobalObject, m_JSHTTPSResponseControllerPrototype) \ V(private, LazyPropertyOfGlobalObject, m_JSFetchTaskletChunkedRequestControllerPrototype) \ diff --git a/test/js/bun/resolve/concurrent-dynamic-import.test.ts b/test/js/bun/resolve/concurrent-dynamic-import.test.ts index b21ead8ddde6..c5eb8df06521 100644 --- a/test/js/bun/resolve/concurrent-dynamic-import.test.ts +++ b/test/js/bun/resolve/concurrent-dynamic-import.test.ts @@ -2,11 +2,8 @@ import { expect, test } from "bun:test"; import { bunEnv, bunExe, tempDir } from "harness"; // Two dynamic imports of the same specifier issued before the first async -// transpile/fetch settles must both resolve. Under the new C++ module loader -// each call gets its own embedder fetch promise (the registry entry is created -// only after the first fetch settles), so the loser of that race must still be -// resolved by Bun__onFulfillAsyncModule rather than left pending forever. -test("concurrent dynamic imports of the same module both resolve", async () => { +// transpile/fetch settles must both resolve, sharing one in-flight fetch. +test.concurrent("concurrent dynamic imports of the same module both resolve", async () => { using dir = tempDir("concurrent-dyn-import", { "shared.ts": `export const heavy = "H";`, "modules.ts": `import { heavy } from "./shared";\nexport const lazy = heavy + "-lazy";`, @@ -30,3 +27,222 @@ test("concurrent dynamic imports of the same module both resolve", async () => { expect(stdout.trim()).toBe("ok"); expect(exitCode).toBe(0); }); + +// JSC only registers a module registry entry once the first embedder fetch +// settles, so every importer that arrives before then used to start its own +// fetch. The loader (a plugin's onLoad, or the transpiler) must run once per +// module, not once per importer. +const pluginPrelude = ` + globalThis.counts = { resolve: 0, load: 0 }; + Bun.plugin({ + name: "virt", + setup(build) { + build.onResolve({ filter: /.*/, namespace: "virt" }, args => { + globalThis.counts.resolve++; + return { path: args.path, namespace: "virt" }; + }); + build.onLoad({ filter: /.*/, namespace: "virt" }, async args => { + globalThis.counts.load++; + await Promise.resolve(); + return { contents: "export const n = " + globalThis.counts.load + ";", loader: "js" }; + }); + }, + }); +`; + +async function runFixture(files: Record, args = ["entry.mjs"]) { + using dir = tempDir("concurrent-dyn-import-plugin", files); + await using proc = Bun.spawn({ + cmd: [bunExe(), ...args], + cwd: String(dir), + env: bunEnv, + stdio: ["ignore", "pipe", "pipe"], + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + return { stdout, stderr, exitCode }; +} + +test.concurrent("concurrent dynamic imports of a plugin-provided specifier run onLoad once", async () => { + const { stdout, stderr, exitCode } = await runFixture({ + "entry.mjs": ` + ${pluginPrelude} + const mods = await Promise.all([import("virt:x"), import("virt:x"), import("virt:x")]); + console.log(JSON.stringify({ + load: globalThis.counts.load, + n: mods.map(m => m.n), + sameIdentity: mods[0] === mods[1] && mods[1] === mods[2], + })); + `, + }); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + expect(JSON.parse(stdout)).toEqual({ load: 1, n: [1, 1, 1], sameIdentity: true }); +}); + +test.concurrent("coalescing an in-flight fetch is per specifier, and survives a later import", async () => { + const { stdout, stderr, exitCode } = await runFixture({ + "entry.mjs": ` + ${pluginPrelude} + await Promise.all([import("virt:a"), import("virt:a"), import("virt:b"), import("virt:b")]); + const afterConcurrent = globalThis.counts.load; + // Already in the registry: no fetch, so no additional load. + await import("virt:a"); + console.log(JSON.stringify({ afterConcurrent, afterReimport: globalThis.counts.load })); + `, + }); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + expect(JSON.parse(stdout)).toEqual({ afterConcurrent: 2, afterReimport: 2 }); +}); + +test.concurrent("a plugin-provided specifier that fails to load rejects every concurrent importer", async () => { + const { stdout, stderr, exitCode } = await runFixture({ + "entry.mjs": ` + let load = 0; + Bun.plugin({ + name: "boom", + setup(build) { + build.onResolve({ filter: /.*/, namespace: "boom" }, args => ({ path: args.path, namespace: "boom" })); + build.onLoad({ filter: /.*/, namespace: "boom" }, async () => { + load++; + throw new Error("nope"); + }); + }, + }); + const results = await Promise.allSettled([import("boom:x"), import("boom:x"), import("boom:x")]); + console.log(JSON.stringify({ + load, + statuses: results.map(r => r.status), + messages: results.map(r => r.reason.message), + })); + `, + }); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + expect(JSON.parse(stdout)).toEqual({ + load: 1, + statuses: ["rejected", "rejected", "rejected"], + messages: ["nope", "nope", "nope"], + }); +}); + +test.concurrent("concurrent dynamic imports of a file run its onLoad plugin once", async () => { + const { stdout, stderr, exitCode } = await runFixture({ + "mod.mjs": `export const v = "from disk";`, + "entry.mjs": ` + let load = 0; + Bun.plugin({ + name: "file-counter", + setup(build) { + build.onLoad({ filter: /mod\\.mjs$/ }, () => { + load++; + return { contents: 'export const v = "from plugin";', loader: "js" }; + }); + }, + }); + const mods = await Promise.all([import("./mod.mjs"), import("./mod.mjs"), import("./mod.mjs")]); + console.log(JSON.stringify({ load, v: mods.map(m => m.v) })); + `, + }); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + expect(JSON.parse(stdout)).toEqual({ load: 1, v: ["from plugin", "from plugin", "from plugin"] }); +}); + +// The in-flight fetch is keyed on the same triple as the module registry, so +// these two imports of one path stay separate modules with different sources. +test.concurrent("concurrent dynamic imports of one path with different type attributes are not coalesced", async () => { + const { stdout, stderr, exitCode } = await runFixture({ + "data.json": `{"a":1}`, + "entry.mjs": ` + const [plain, text] = await Promise.all([ + import("./data.json"), + import("./data.json", { with: { type: "text" } }), + ]); + console.log(JSON.stringify({ plain: plain.default, text: text.default })); + `, + }); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + expect(JSON.parse(stdout)).toEqual({ plain: { a: 1 }, text: `{"a":1}` }); +}); + +// Attributes without a `type` key are their own registry entry even though the +// type attribute string is empty for both, so the two fetches must stay apart. +test.concurrent( + "concurrent dynamic imports of one path with and without import attributes stay separate modules", + async () => { + const { stdout, stderr, exitCode } = await runFixture({ + "mod.mjs": `export const v = 1;`, + "entry.mjs": ` + let load = 0; + Bun.plugin({ + name: "file-counter", + setup(build) { + build.onLoad({ filter: /mod\\.mjs$/ }, () => { + load++; + return { contents: "export const v = 1;", loader: "js" }; + }); + }, + }); + const [plain, attributed] = await Promise.all([ + import("./mod.mjs"), + import("./mod.mjs", { with: { unknown: "x" } }), + ]); + console.log(JSON.stringify({ load, sameModule: plain === attributed, v: [plain.v, attributed.v] })); + `, + }); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + expect(JSON.parse(stdout)).toEqual({ load: 2, sameModule: false, v: [1, 1] }); + }, +); + +// require(esm) forces a synchronous fetch of a key whose async fetch may still +// be in flight; it must not get handed the pending promise. +test.concurrent("require(esm) racing a dynamic import of the same module still resolves synchronously", async () => { + const { stdout, stderr, exitCode } = await runFixture( + { + "esm.mjs": `export const v = "esm";`, + "entry.cjs": ` + const pending = import("./esm.mjs"); + const required = require("./esm.mjs"); + pending.then(imported => { + console.log(JSON.stringify({ required: required.v, imported: imported.v })); + }); + `, + }, + ["entry.cjs"], + ); + expect({ stderr, exitCode }).toEqual({ stderr: "", exitCode: 0 }); + expect(JSON.parse(stdout)).toEqual({ required: "esm", imported: "esm" }); +}); + +// A settled fetch must be dropped again, otherwise mock.module() (which wipes +// the module registry entry) would be defeated by the coalescing cache. +test.concurrent("a settled fetch stops being shared once the module registry owns it", async () => { + const { stdout, stderr, exitCode } = await runFixture( + { + "mod.mjs": `export const v = "real";`, + "invalidate.test.ts": ` + import { expect, mock, test } from "bun:test"; + let loads = 0; + Bun.plugin({ + name: "counter", + setup(build) { + build.onLoad({ filter: /mod\\.mjs$/ }, () => { + loads++; + return { contents: 'export const v = "real";', loader: "js" }; + }); + }, + }); + test("mock.module after a settled fetch wins", async () => { + expect((await import("./mod.mjs")).v).toBe("real"); + expect(loads).toBe(1); + mock.module("./mod.mjs", () => ({ v: "mocked" })); + expect((await import("./mod.mjs")).v).toBe("mocked"); + expect(loads).toBe(1); + }); + `, + }, + ["test", "invalidate.test.ts"], + ); + // `bun test` prints its banner to stdout and everything else to stderr. + expect(stderr).toContain("1 pass"); + expect(stderr).toContain("0 fail"); + expect({ stdout, exitCode }).toEqual({ stdout: expect.stringContaining("bun test"), exitCode: 0 }); +});