diff --git a/src/jsc/bindings/ModuleLoader.cpp b/src/jsc/bindings/ModuleLoader.cpp index a4b4e91fbafe..90754ff7cd70 100644 --- a/src/jsc/bindings/ModuleLoader.cpp +++ b/src/jsc/bindings/ModuleLoader.cpp @@ -655,6 +655,34 @@ void evaluateCommonJSCustomExtension( RETURN_IF_EXCEPTION(scope, ); } +static void provideFetchForSyncLoad(Zig::GlobalObject* globalObject, const WTF::String& specifier, JSC::JSSourceCode* jsSourceCode) +{ + auto& vm = JSC::getVM(globalObject); + auto scope = DECLARE_THROW_SCOPE(vm); + auto key = JSC::Identifier::fromString(vm, specifier); + // The JavaScript-typed entry is the one loadModuleSync() reads; a typed import has its own entry. + auto* entry = globalObject->moduleLoader()->ensureRegistered(globalObject, key, JSC::ScriptFetchParameters::Type::JavaScript); + switch (entry->status()) { + case JSC::ModuleRegistryEntry::Status::New: + scope.release(); + entry->provideFetch(globalObject, jsSourceCode); + return; + case JSC::ModuleRegistryEntry::Status::Fetching: { + entry->ensureModulePromise(globalObject); + RETURN_IF_EXCEPTION(scope, void()); + JSC::JSPromise* fetchPromise = entry->ensureFetchPromise(globalObject); + RETURN_IF_EXCEPTION(scope, void()); + // fulfillPromise, not fulfill(): pipeFrom() already claimed this promise's resolving functions. + if (fetchPromise->status() == JSC::JSPromise::Status::Pending) + fetchPromise->fulfillPromise(vm, jsSourceCode); + return; + } + default: + // Fetched, or failed earlier: loadModuleSync() reports whatever the entry already holds. + return; + } +} + JSValue fetchCommonJSModule( Zig::GlobalObject* globalObject, JSCommonJSModule* target, @@ -711,7 +739,7 @@ JSValue fetchCommonJSModule( JSC::VM::SynchronousModuleQueue queue; queue.prev = vm.m_synchronousModuleQueue; vm.m_synchronousModuleQueue = &queue; - globalObject->moduleLoader()->provideFetch(globalObject, JSC::Identifier::fromString(vm, specifierWtfString), JSC::ScriptFetchParameters::Type::JavaScript, jsSourceCode); + provideFetchForSyncLoad(globalObject, specifierWtfString, jsSourceCode); if (!scope.exception()) JSC::JSModuleLoader::drainSynchronousModuleQueue(globalObject); vm.m_synchronousModuleQueue = queue.prev; RETURN_IF_EXCEPTION(scope, {}); @@ -766,7 +794,7 @@ JSValue fetchCommonJSModule( JSC::VM::SynchronousModuleQueue queue; queue.prev = vm.m_synchronousModuleQueue; vm.m_synchronousModuleQueue = &queue; - globalObject->moduleLoader()->provideFetch(globalObject, JSC::Identifier::fromString(vm, specifierWtfString), JSC::ScriptFetchParameters::Type::JavaScript, jsSourceCode); + provideFetchForSyncLoad(globalObject, specifierWtfString, jsSourceCode); if (!scope.exception()) JSC::JSModuleLoader::drainSynchronousModuleQueue(globalObject); vm.m_synchronousModuleQueue = queue.prev; RETURN_IF_EXCEPTION(scope, {}); @@ -800,7 +828,7 @@ JSValue fetchCommonJSModule( JSC::VM::SynchronousModuleQueue queue; queue.prev = vm.m_synchronousModuleQueue; vm.m_synchronousModuleQueue = &queue; - globalObject->moduleLoader()->provideFetch(globalObject, JSC::Identifier::fromString(vm, specifierWtfString), JSC::ScriptFetchParameters::Type::JavaScript, JSC::SourceCode(Ref(*cached))); + provideFetchForSyncLoad(globalObject, specifierWtfString, JSC::JSSourceCode::create(vm, JSC::SourceCode(Ref(*cached)))); if (!scope.exception()) JSC::JSModuleLoader::drainSynchronousModuleQueue(globalObject); vm.m_synchronousModuleQueue = queue.prev; RETURN_IF_EXCEPTION(scope, {}); @@ -898,7 +926,7 @@ JSValue fetchCommonJSModuleNonBuiltin( JSC::VM::SynchronousModuleQueue queue; queue.prev = vm.m_synchronousModuleQueue; vm.m_synchronousModuleQueue = &queue; - globalObject->moduleLoader()->provideFetch(globalObject, JSC::Identifier::fromString(vm, specifierWtfString), JSC::ScriptFetchParameters::Type::JavaScript, JSC::SourceCode(provider)); + provideFetchForSyncLoad(globalObject, specifierWtfString, JSC::JSSourceCode::create(vm, JSC::SourceCode(provider))); if (!scope.exception()) JSC::JSModuleLoader::drainSynchronousModuleQueue(globalObject); vm.m_synchronousModuleQueue = queue.prev; } diff --git a/test/js/bun/resolve/require-esm-in-flight-sibling.test.ts b/test/js/bun/resolve/require-esm-in-flight-sibling.test.ts new file mode 100644 index 000000000000..f1dac72935de --- /dev/null +++ b/test/js/bun/resolve/require-esm-in-flight-sibling.test.ts @@ -0,0 +1,96 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, tempDir } from "harness"; + +// A CommonJS module imported by an ESM graph runs its body while the graph is +// still loading, so a require() in it can hit an ESM sibling whose transpile is +// still in flight on the transpiler thread. The CJS module here is named +// reflect-metadata because the runtime transpiles that package on the main +// thread (ALWAYS_SYNC_MODULES in src/runtime/jsc_hooks.rs): its body runs in the +// microtask checkpoint right after the graph's fetches are issued, before the +// thread pool can have delivered e.mjs, so the sibling is mid-fetch on every run. +const inFlightSibling = { + "entry.mjs": ` + import "./e.mjs"; + import "reflect-metadata"; + console.log(globalThis.order.concat("entry").join(",")); + `, + "e.mjs": ` + (globalThis.order ??= []).push("e"); + export const e = 1; + `, + "node_modules/reflect-metadata/package.json": JSON.stringify({ name: "reflect-metadata", main: "index.js" }), + "node_modules/reflect-metadata/index.js": ` + const { e } = require("../../e.mjs"); + (globalThis.order ??= []).push("polyfill:" + e); + `, +}; + +test.concurrent("require() of an ESM sibling the import graph is still fetching loads it synchronously", async () => { + using dir = tempDir("require-esm-in-flight", inFlightSibling); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "entry.mjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ stdout: "e,polyfill:1,entry\n", stderr: "", exitCode: 0 }); +}); + +// With the transpiler thread disabled the graph's fetches settle on the spot, +// so when the require() lands the sibling's entry is still Fetching but its +// fetch promise is already settled and only the reaction that builds the module +// is still queued. That is the one state the require() path must leave alone. +test.concurrent("require() of a sibling whose fetch has settled but not been processed yet", async () => { + using dir = tempDir("require-esm-settled-sibling", inFlightSibling); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "entry.mjs"], + env: { ...bunEnv, BUN_FEATURE_FLAG_DISABLE_ASYNC_TRANSPILER: "1" }, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ stdout: "e,polyfill:1,entry\n", stderr: "", exitCode: 0 }); +}); + +// An import with a type attribute gets its own registry entry, keyed by that +// type, and is transpiled with that loader. A require() of the same file is a +// plain JavaScript-typed load and must not hand its default-loader output to +// the typed entry that happens to be in flight. `a < b > (c)` is a generic call +// under the ts loader and two comparisons under the default loader for a .js +// file, so each consumer reveals which transpile it received. +test.concurrent("require() racing a typed import of the same file does not overwrite the typed import", async () => { + using dir = tempDir("require-esm-typed-sibling", { + "entry.mjs": ` + import { y } from "./mod.js" with { type: "ts" }; + import "reflect-metadata"; + console.log(JSON.stringify({ typedImport: y, require: globalThis.required, evaluations: globalThis.evaluations })); + `, + "mod.js": ` + globalThis.evaluations = (globalThis.evaluations ?? 0) + 1; + const a = value => "called(" + value + ")"; + const b = 1; + const c = 3; + export const y = a < b > (c); + `, + "node_modules/reflect-metadata/package.json": JSON.stringify({ name: "reflect-metadata", main: "index.js" }), + "node_modules/reflect-metadata/index.js": ` + globalThis.required = require("../../mod.js").y; + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "entry.mjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ + stdout: JSON.stringify({ typedImport: "called(3)", require: false, evaluations: 2 }) + "\n", + stderr: "", + exitCode: 0, + }); +}); diff --git a/test/regression/issue/33180.test.ts b/test/regression/issue/33180.test.ts new file mode 100644 index 000000000000..04258d319770 --- /dev/null +++ b/test/regression/issue/33180.test.ts @@ -0,0 +1,75 @@ +// https://github.com/oven-sh/bun/issues/33180 +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, tempDir } from "harness"; + +test.concurrent("require() of an ESM module the surrounding import graph is still fetching", async () => { + using dir = tempDir("issue-33180-cycle", { + "entry.mjs": ` + import "./starter.cjs"; + import a from "./a.cjs"; + import { lateViaESM } from "./b.mjs"; + console.log(JSON.stringify({ evaluations: globalThis.evaluations, late: a.late, lateViaESM: lateViaESM() })); + `, + "starter.cjs": `require("./a.cjs");`, + "a.cjs": ` + globalThis.evaluations = (globalThis.evaluations ?? 0) + 1; + exports.early = "early"; + require("./b.mjs"); + exports.late = "late"; + `, + "b.mjs": ` + import a from "./a.cjs"; + export function lateViaESM() { return a.late; } + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "entry.mjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ + stdout: JSON.stringify({ evaluations: 1, late: "late", lateViaESM: "late" }) + "\n", + stderr: "", + exitCode: 0, + }); +}); + +test.concurrent("consecutive require()s of several ESM siblings that are still transpiling", async () => { + const files: Record = { + "entry.mjs": ` + import "./e1.mjs"; + import "./e2.mjs"; + import "./e3.mjs"; + import "./e4.mjs"; + import "./k.cjs"; + console.log("entry-done"); + `, + "k.cjs": ` + const ids = [1, 2, 3, 4].map(i => require("./e" + i + ".mjs").id); + console.log("required " + JSON.stringify(ids)); + module.exports = {}; + `, + }; + for (let i = 1; i <= 4; i++) { + let source = ""; + for (let j = 0; j < 500; j++) source += `export const v${j} = ${j};\n`; + files[`e${i}.mjs`] = source + `export const id = ${i};\n`; + } + using dir = tempDir("issue-33180-siblings", files); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "entry.mjs"], + env: bunEnv, + cwd: String(dir), + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ + stdout: "required [1,2,3,4]\nentry-done\n", + stderr: "", + exitCode: 0, + }); +});