Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Dynamic import queue handling src/jsc/bindings/BunClientData.h, src/jsc/bindings/ZigGlobalObject.cpp |
JSVMClientData records the dynamic-import queue. moduleLoaderImportModule scopes a nested queue, and moduleLoaderFetch uses asynchronous fetching while that queue is active. The scope queues captured reactions as microtasks when it exits. |
Dynamic import ordering tests test/js/bun/resolve/require-esm-microtask-order.test.ts |
Tests run with the default and sync transpilers. They check evaluation and settlement order for delayed, throwing, and top-level-await imports, shared modules, and imports involving modules that are still executing. |
Suggested reviewers: jarred-sumner, dylan-conway
Priority:
Merge Risk: 
16b65
-
No actionable issue is established for this change; it is ready for normal merge checks.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: preventing import() started during require(esm) from executing inside require(). Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The pull request description is complete and relevant. It explains the problem, root cause, fix, scope, trade-offs, limitations, and verification results. Although it does not use the exact template h… Full details: Description check
-
Explanation
The pull request description is complete and relevant. It explains the problem, root cause, fix, scope, trade-offs, limitations, and verification results. Although it does not use the exact template headings, it provides the required information under equivalent sections.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Status Reproduced on main cd "$(mktemp -d)"; echo '{"name":"r"}' > package.json
cat > entry.cjs <<'E'
globalThis.t=[]; process.on("exit",()=>console.log(t.join(" ")));
t.push("before-require"); require("./a.mjs"); t.push("after-require"); require("./z.cjs"); t.push("end");
E
echo 'globalThis.t.push("a"); import("./x.mjs").then(()=>globalThis.t.push("x-resolved")); export const a=1;' > a.mjs
echo 'import "./y.cjs"; globalThis.t.push("x"); export const x=1;' > x.mjs
echo 'globalThis.t.push("y"); module.exports={};' > y.cjs
echo 'globalThis.t.push("z"); module.exports={};' > z.cjs
node entry.cjs # before-require a after-require z end y x x-resolved
bun entry.cjs # before-require a y x after-require z end x-resolved (main)
# before-require a after-require z end y x x-resolved (this branch)The same cause has three more effects on main. This branch has Node's result for each:
The regression test is State: closed. This change cannot be the fix. Executables built with |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes how vm.m_synchronousModuleQueue frames are pushed/popped across moduleLoaderImportModule — a JSC-fork mechanism with GC-visiting and drain-order invariants — a human familiar with that fork addition should confirm the frame-chain assumptions hold.
What was reviewed:
DynamicImportQueueScopeRAII: constructor is a no-op when no sync load is active; destructor queues before unlinking so tasks stay reachable throughprevduringqueueMicrotaskallocation; bothm_synchronousModuleQueueanddynamicImportModuleQueueare restored on every exit path.- Nesting: a
require()inside theimport()pushes a newer head, so themoduleLoaderFetchpointer-equality check still routes it to the sync path;m_outerDynamicImportQueuerestores correctly forimport()→require()→import(). - Tests:
describe.eachover both transpiler modes,test.concurrent,Promise.allon stdout/stderr/exited, stderr asserted before exitCode,.toEqualon ordered arrays, Node's order cited per case.
Extended reasoning...
Overview
This PR fixes dynamic import() incorrectly evaluating synchronously when called from a module being loaded by require(esm). It adds a dynamicImportModuleQueue pointer to JSVMClientData (BunClientData.h), a stack-only DynamicImportQueueScope RAII class in ZigGlobalObject.cpp that pushes an isolation frame onto vm.m_synchronousModuleQueue for the duration of moduleLoaderImportModule, and a pointer-equality check in moduleLoaderFetch to skip the synchronous fetch fast path when serving the import() request. Eight new test cases (four scenarios × two transpiler modes) verify the ordering matches Node.
Security risks
None identified. This is a module-evaluation ordering fix; no user input parsing, auth, crypto, or network boundary is touched. The state added is a raw pointer on JSVMClientData that only ever points at a stack frame with strictly nested lifetime.
Level of scrutiny
High. The synchronous module queue is a BUN_JSC_ADDITIONS fork mechanism with two non-local invariants: (1) VM::visitAggregate must be able to walk the prev chain to mark every queued task's JSValues during any GC, and (2) JSPromise's divert sites and JSModuleLoader::drainSynchronousModuleQueue must always see a consistent head. The RAII scope preserves both — the frame stays linked while tasks are flushed, and restore happens on every path including early RETURN_IF_EXCEPTION returns from moduleLoaderImportModule — but these invariants live in the WebKit fork rather than this repo, so someone who owns that fork code should confirm no drain/visit path assumes the head frame is one it pushed itself. The PR description also explicitly names this as a Bun-side stopgap ahead of a fork change and documents a second door (shared registry entry force-settled by hostLoadImportedModule) that this PR does not close; that scoping decision is worth a human sign-off.
Other factors
The implementation follows repo conventions closely: per-VM state on JSVMClientData rather than ZigGlobalObject, WTF_MAKE_NONCOPYABLE + WTF_FORBID_HEAP_ALLOCATION for the scope, comments limited to the load-bearing GC-ordering note. Tests extend the existing require-esm-microtask-order.test.ts file, use tempDir/bunEnv/bunExe/test.concurrent, drain subprocess pipes with Promise.all, assert stderr and exitCode in the right order, and pin only the ordering facts that are stable across concurrent fetches. The bug hunter ran to a dry streak with no findings.
|
On the one open question above (does any drain or visit path assume the head frame is one it pushed itself): I checked every reader of
The Bun-side frames in |
…raining them in place The wait now hands what lands in the macro's frame to the microtask queue, which is where the reactions go when no require() is on the stack. This drops the re-entrancy flag and the exception handling that a direct call to drainSynchronousModuleQueue needed, and it is the same idiom as the import() scope in #41997.
…it leaves open The queue is no longer named for macros. NestedModuleQueueScope is a standalone RAII type that MacroModeGuard enters, the innermost queue is recorded in JSVMClientData::nestedModuleQueue where C++ callers can see it, and the replay loop is Bun::requeueSynchronousModuleQueueAsMicrotasks in ModuleLoader.h, so the import() isolation in #41997 can use the same pieces. Tests: a macro that starts an import() and does not await it, and two todo tests for the shapes that still spin (the macro's own file already in the require()'s graph).
…nside the require() A dynamic import() called while require(esm) was synchronously loading a module graph joined that synchronous load: moduleLoaderFetch transpiled it on the spot and the loader's reactions for it landed on the queue that the require() was draining, so the imported module and its whole static graph evaluated before require() returned. moduleLoaderImportModule now pushes its own queue frame for the duration of the request when one is active, keeps the fetch asynchronous, and hands the reactions the request queued to the microtask queue on the way out.
…ll-running module Adds the cases where the import() target is a module the entry is still loading, a module that imports the requiring module, or the requiring module itself. On main these read half-filled exports, hit the temporal dead zone, or never settle.
4c63e31 to
16b6521
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| auto* globalObject = static_cast<Zig::GlobalObject*>(jsGlobalObject); | ||
|
|
||
| VM& vm = JSC::getVM(globalObject); | ||
| DynamicImportQueueScope dynamicImportQueueScope(globalObject, vm); |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: Users whose require()d ES module calls Bun.ModuleGraph's graph.import() still get that graph's module evaluated inside the require(), the ordering bug this change fixes for import(). The isolation frame is pushed only in moduleLoaderImportModule at src/jsc/bindings/ZigGlobalObject.cpp:4097. JSModuleGraph::import (src/jsc/bindings/ModuleGraph.cpp:652) calls loader->loadModule directly, so moduleLoaderFetch at ZigGlobalObject.cpp:4242 sees the require's frame as head and fetches synchronously. Fix: push the frame for every dynamic-load entry point, e.g. move DynamicImportQueueScope to a header and instantiate it in JSModuleGraph::import as well.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
Trigger: a CJS entry does require("./app.mjs") and app.mjs calls graph.import("./plugin.ts") on a Bun.ModuleGraph at top level, so vm.m_synchronousModuleQueue is the require()'s frame while jsModuleGraphPrototypeFunction_import runs (src/jsc/bindings/ModuleGraph.cpp:709-716). JSModuleGraph::import at src/jsc/bindings/ModuleGraph.cpp:652 calls loader->loadModule(..., { ModuleLoadFlag::Evaluate, ModuleLoadFlag::Dynamic }) without going through GlobalObject::moduleLoaderImportModule, so no DynamicImportQueueScope is installed and JSVMClientData::dynamicImportModuleQueue stays null. In GlobalObject::moduleLoaderFetch (src/jsc/bindings/ZigGlobalObject.cpp:4242) the condition vm.m_synchronousModuleQueue != dynamicImportModuleQueue is true, so Bun::fetchESMSourceCodeSync runs and returns a settled promise; the loader's reactions settle while the require's frame is head and are diverted into it. The graph's main module and its static graph therefore evaluate inside the require(), before require() returns, exactly as import() did on the base branch.
Verification: pre-existing (the base already evaluates a graph.import() target inside the require()). The isolation frame is pushed only in GlobalObject::moduleLoaderImportModule (src/jsc/bindings/ZigGlobalObject.cpp:4097). JSModuleGraph::import (src/jsc/bindings/ModuleGraph.cpp:652) calls loader->loadModule directly, so the condition at ZigGlobalObject.cpp:4242 is true and Bun::fetchESMSourceCodeSync runs.
There was a problem hiding this comment.
Confirmed on this branch. A required ES module that calls graph.import() still gets the graph's module evaluated inside the require(): before-require app app-end plugin after-require plugin-resolved. The PR notes now list it as not covered.
I do not want to add one scope for each entry point. JSModuleLoader__import and the bake hook do not pass moduleLoaderImportModule either. Every dynamic load passes JSModuleLoader::loadModule with ModuleLoadFlag::Dynamic, so I am checking a boundary there, in the fork, before I change this PR.
The isolation scope no longer switches moduleLoaderFetch to the thread pool for an import() issued during require(esm). The scope alone keeps the import()'s loader reactions out of the require()'s queue, and the thread-pool path loads a .cjs file that names neither module, exports nor require as an ES module, so a file that was also require()d ran twice. This removes JSVMClientData::dynamicImportModuleQueue. Tests: assert that the in-flight import() settles, and add the side-effect-only .cjs target in both require orders.
|
Closing. This change cannot be the fix, for two reasons. 1. Compiled code does not pass the hook. Executables built with 2. The scope starves the cd "$(mktemp -d)"; echo '{}' > package.json
cat > entry.cjs <<'E'
try { require("./a.mjs"); console.log("require ok"); } catch (e) { console.log("require threw:", e.message); }
E
printf 'import "./p1.mjs";\nimport "./p2.mjs";\n' > a.mjs
printf 'import "./c.cjs";\n' > p1.mjs
printf 'import "./c.cjs";\nimport "./b.mjs";\n' > p2.mjs
printf 'import("./b.mjs");\n' > c.cjs
printf 'export const v = 1;\n' > b.mjs
bun entry.cjs # main and Node: require ok
# this branch: require threw: require() async module ".../a.mjs" is unsupported. use "await import()" instead.
The diversion in the fork does not know which load a reaction belongs to. A scope in Bun cannot decide that either. The fix belongs in the fork's loader: The 29 cases in |
Problem
import()called from a module that a synchronousrequire()is loading ran inside thatrequire(): wrong order (a y x after-require), an entry linked against half-filled CommonJS exports (Export named 'v' not found), animport()that never settles.GlobalObject::moduleLoaderImportModule(src/jsc/bindings/ZigGlobalObject.cpp) runs whilevm.m_synchronousModuleQueueis set, so the new graph's loader reactions join the queue therequire()drains.Fix
moduleLoaderImportModuleinstalls aDynamicImportQueueScope. While a synchronous load is active it pushes its own queue frame for the request, then moves what landed there to the microtask queue.import()target is still fetched synchronously, as on main.test/js/bun/resolve/require-esm-microtask-order.test.ts(29 cases, 22 fail on main).Background
VM::m_synchronousModuleQueue(JSC fork) points at a stack frame,JSPromiseappends module-loader microtasks to it anddrainSynchronousModuleQueueruns them. The test is the task type, not the owning load.JSModuleLoader::loadModulefor dynamic loads): no re-queue, coversgraph.import()and compiled code, needs a WebKit release.Downsides
--aotexecutables keep the old order, so plain and compiled runs differ andtest/aotfails. The fork's AOT runtime must change with it.require()can run the deferred importer inside it, sorequire("./m3.mjs").visundefined, and a side-effect-only.cjsdependency runs twice (Decide a file's module type with one rule on every runtime load path #40940).import(), no allocation, +994 bytes.text(release x64,--lto=off).Notes
The three symptoms. Each one is the
import()'s graph loaded, linked and evaluated by the drain loop of therequire().entry.cjsrequiresa.mjs, which callsimport("./x.mjs"), andx.mjsimportsy.cjs. main printsbefore-require a y x after-require z end x-resolved. Node and this branch printbefore-require a after-require z end y x x-resolved. A throwing or top-level-await target also ran its body inside therequire().c.cjs.c.cjscallsrequire("./e.mjs")and then setsexports.v.e.mjscallsimport("./c.cjs")orimport("./m0.mjs"). The drain builtc.cjs's module record while its body was still running, then linked and evaluated the entry against it. A default import readundefined. A named import failed withExport named 'v' not found. With a CommonJS entry the namespace stayed withoutvfor good. WithcreateRequirein an ES entry the importer read a binding in its temporal dead zone.e.mjsrequiresd.mjs, andd.mjscallsimport("./e.mjs"). The drain enteredEvaluate()again for a module that was still evaluating. That promise never settled, and no laterimport()of the module did either.Why it cannot land alone:
--aot.test/aotruns each program plain and compiled withbun build --compile --aotand requires the same output. With this change the plain run has Node's order and the compiled run keeps the old one. In CI, 9test/aotfiles of the drawn sample fail, for example 29 of 81 seeds ofgenerated-module-graphs-of-both-kinds.test.ts.moduleLoaderImportModule. The AOT runtime in the fork (Source/JavaScriptCore/aot/runtime/AOTModules.cpp) callsJSModuleLoader::requestImportModuleForKeyitself and reproduces what the loader does under a synchronous load. The corpus states the old order as the expected one, for exampletest/aot/programs/modules/a-module-that-an-import-loads-inside-a-require-reads-variables-of-the-module-that-requires/main.mjs.JSModuleLoader::loadModulefor loads withModuleLoadFlag::Dynamiccoversimport(),graph.import(), native imports and the compiled imports that go through the loader.The two losses against main. On main an
import()underrequire()ran to its end before therequire()returned. With this change it is an asynchronous load like any other, so it meets two older bugs that main has forimport()outsiderequire().require()of that module from other code makes it synchronously, which triggers the waiting load's reactions inside the drain, so the waiting importer is evaluated inside thatrequire(). Repro:main.cjsrequiresm1.mjs.m1.mjscallsimport("./m2.mjs")and thenimport("./k.cjs").then(() => require("./m3.mjs")).m2.mjsimportsm3.mjsand also callsrequire("./m3.mjs"). main printsrequire gives v3, this branch printsrequire gives undefined. What closes it: the diversion in the fork'sJSPromise.cppmust know which load a reaction belongs to, and give reactions of other loads to the microtask queue. No scope in Bun can reach it..cjsfile that names neithermodule,exportsnorrequireas an ES module. If the file was alsorequire()d, it runs twice. Theimport()target itself is safe here because its fetch stays synchronous. A static dependency of the target is not:main.cjsrequiresx.cjsanda.mjs,a.mjscallsimport("./m.mjs"),m.mjsimportsx.cjs: main runsx.cjsonce, this branch twice. Decide a file's module type with one rule on every runtime load path #40940 closes it and should land first.Not covered.
Bun.ModuleGraph:graph.import()callsJSModuleLoader::loadModuledirectly and does not pass this hook. A required ES module that calls it still gets the graph's module evaluated inside therequire()(before-require app app-end plugin after-require).JSModuleLoader__importand the bake hook do not pass it either.How it was measured.
ZigGlobalObject.cpp.obuilt with--profile=release --lto=offat the merge base and at this branch:llvm-sizetext goes from 190,865 to 191,859 bytes.moduleLoaderImportModulegoes from 2,174 to 2,847 bytes.moduleLoaderFetchdoes not change.import()when no synchronous load is active, read from that object's disassembly: 8 at entry (5 stores, one load, test, branch) and 6 at exit.Other suites. On the previous head (same scope, with the fetch guard) I also ran the other
require-esm-*files,import-defer,dynamic-import-tla-cycle,pluginsandmodule-graphone at a time. They passed, except fourBun.ModuleGraphdynamicImportcases that exceed the 5 s timeout on the debug build here. The stock release build needs 5.6 s for the same program.Details of the fix.
prevwhile installed, soVM::visitAggregatekeeps marking the outer frames' queued values. Clearingvm.m_synchronousModuleQueuefor the request would hide them from the GC.JSGlobalObject::queueMicrotask, the calldrainSynchronousModuleQueueuses on its exception path. Tasks are queued before the frame is unlinked becausequeueMicrotaskcan allocate.NodeVMGlobalObject::moduleLoaderImportModuleis untouched. Its default-loader case delegates to this hook. ShadowRealm globals use this hook too.[human-review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file