Repository navigation
[JSC] Settle an Evaluate() of an EVALUATING module when the walk pops its cycle - #871
Draft
robobun wants to merge 1 commit into
Conversation
Collaborator
Author
|
Status
|
|
Preview build of 09fb39b: |
robobun
force-pushed
the
robobun/f48be7fb/module-finishes-inside-walk
branch
from
October 22, 2026 01:09
e441007 to
ab1eb44
Compare
… its cycle ECMA-262 has no job that runs while a module graph is evaluated. A host can run one from a module body: a microtask checkpoint, or a synchronous require() of an ES module. The continuation of an import() is such a job, and it calls Link() and Evaluate() for the module that it imports. That module can be on the stack of the InnerModuleEvaluation that the body runs in: its status is EVALUATING. Evaluate() asserted that the status is not EVALUATING. In a release build it made a new capability, stored it in [[TopLevelCapability]] of the record, and returned it: the walk of an EVALUATING record returns at once. Nothing resolves the capability of a record that another Evaluate() finishes, so the import() never settled, and step 4 gave the same promise to each later Evaluate() of the module. Evaluate() of an EVALUATING record now returns a promise that VM::m_moduleEvaluationWaiters keeps, and writes nothing to the record. The record of the outer Evaluate() has a capability, and step 4 still returns that one. Link() accepts the status: InnerModuleLinking already returns at once for it. The Evaluate() that has the record on its stack takes the promise out when the record leaves the stack. Before step 16 of InnerModuleEvaluation pops a cycle, the promises of its records go to the root. The first one becomes the capability of the root, as for an Evaluate() that is called right after the pop, and the others follow that capability. After the pop a root that is EVALUATED fulfils it. AsyncModuleExecutionFulfilled or AsyncModuleExecutionRejected settles it for a root that is EVALUATING-ASYNC. Step 9.a of Evaluate() rejects the promise of each record that it gives the error to. This change is on top of the one for a job that finishes a module inside the walk. No record on a stack has a capability from a job now, so the two checks for that in asyncExecutionFulfilled() go. A record that a job rejected stays EVALUATING there, and step 9.a can give it another error. ModuleRegistryEntry::error() does not answer an import() of such a record with the first error: the import() waits for the evaluation. A graph with no such job pays one check of an empty map for each cycle that the walk pops, and one status compare in each Evaluate() that walks.
robobun
force-pushed
the
robobun/ff435ce1/import-of-evaluating-module
branch
from
October 22, 2026 13:15
c791704 to
09fb39b
Compare
This was referenced Oct 24, 2026
Draft
Collaborator
Author
|
Cross-reference: #961 changes the end of With this change |
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
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.
Stacked on #832. Notes say why.
Problem
require()of an ES module. When it continues animport()of a module that is still on the stack of that evaluation, the promise never settles. A debug build failsASSERTION FAILED: status() == Status::Unlinked || status() == Status::Linked || status() == Status::EvaluatingAsync || status() == Status::Evaluated(CyclicModuleRecord.cpp(545)).CyclicModuleRecord::evaluate()stores a capability on the EVALUATING record (CyclicModuleRecord.cpp:609onmain). Nothing resolves it.Fix
evaluate()of an EVALUATING record returns a promise thatVM::m_moduleEvaluationWaiterskeeps, and writes nothing to the record.JSTests/stress/import-evaluating-module-in-job-inside-evaluation*.js(31 graphs,maingets 23 wrong) andJSTests/modules.Background
Evaluate()walks a graph depth first. A record is EVALUATING while it is on the stack of that walk. The walk pops a finished cycle and names its root, which has the promise ofEvaluate().import()job (wrong when the module throws later), and keeping the promise on the record until the pop ([JSC] A job that finishes a module from inside a module body does not strand or start its importers early #832 reads that slot).Downsides
import()in promises and jobs: +0. Instructions per popped cycle: not measured yet (Notes).import()of a cycle member that waits for it now deadlocks, as without a job. Bun 1.4.3 completes it.import()of a failing module now rejects: exit code 1, as in Node.js. Bun 1.4.3 exits with 0.Notes
The program and the releases. Four files:
entry.mjsimportsa.mjsand then callsimport("./a.mjs").a.mjsimportsb.mjs.b.mjscallscreateRequire(import.meta.url)("./r.mjs").r.mjscallsimport("./a.mjs").mainimport()inr.mjsimport()inentry.mjsdrainMicrotasks()ofbun:jscinb.mjsand norequire()A top-level
awaitof that laterimport()hangs the process on Bun 1.4.2. No user has reported any of this.The review of the loader rewrite saw the assertion with
drainMicrotasks()and left it (WebKit#57827, 2026-04-02): it "occurs only if microtasks are explicitly drained while a microtask is already being processed", which "wouldn't ever occur on the web", and the fix there was "a linear scan during dynamic imports". Bun has such calls. This change adds one status compare to animport(), and no scan.The calls in Bun that run a job while a module is evaluated. Each leaves the
import()pending on Bun 1.4.2, and each fulfils it with this change:drainMicrotasks()ofbun:jscrequire()of an ES module, also asimport.meta.require()in the output ofbun build --splitting --target=bunHTMLRewriterhandler, intransform()of a stringBun.build()with a plugin whosesetup()is asyncexpect(promise).resolvesand.rejects,expect(asyncFn).toThrow(), an asyncexpect.extendmatcherrequire()of a file that calls an async macroThe graphs of the tests. Each is a directory under
JSTests/stress/resources/import-evaluating-module-in-job-inside-evaluation/. A body callsimport()for a module on the stack and thendrainMicrotasks(). The test reads the state of each import inside the walk, after it, and after one moreimport()of the same module. "the import" is the one that a body started inside the walk.main, release shelldependency,job-later-in-the-walk,cycle-members,collected-while-waitingcycle-with-the-module-of-the-evaluate,async-cycle-members,async-cycle-with-the-module-of-the-evaluateasync-cycle-memberawaited-by-later-modulebody-throws,dependency-throws,cycle-root-body-throws,import-started-before-the-failuresibling-throws-laternested-evaluation,nested-evaluation-fails(a secondEvaluate()inside the walk, through a deferred namespace)deferred-namespace-of-failed-moduleawaited-by-dependency-of-cycle-rootfinishes-on-the-stack,imported-after-it-finished-on-the-stack,rejects-on-the-stack,rejects-on-the-stack-then-body-throws,imported-after-a-job-rejected-itThe other eight give the same on
main:module-of-the-evaluate,top-level-await,waits-for-async-dependency,top-level-await-rejects,awaited-by-dependency,importer-of-module-on-the-stack,import-defer(a debug shell ofmainasserts inlink()) andterminated(the watchdog ends the walk while an import waits).Why this is on top of #832. #832 is for a job that finishes or rejects a module inside the walk. Its review asked to settle the promise of such an
import()"when the walk pops the cycle", and #832 left that to a change for theimport(). Five graphs need both changes. Forfinishes-on-the-stack:mainfulfils the import before the cycle has run and never finishes the root. #832 alone finishes the root and leaves the import pending. Both together fulfil it after the root.isOnStack ? nullptr : topLevelCapability()). Only anEvaluate()from a job put a capability there, so this change removes them. The assertion of step 7.a,cycleRoot() == this, now fails if a record on a stack has one.ModuleRegistryEntry::error()answered animport()of that record with the first error at once. It now gives no error for a record that is EVALUATING, and theimport()gets the error of the evaluation (imported-after-a-job-rejected-it).main. This branch mergesmainat 7d0c8a7, so those commits show in the diff until [JSC] A job that finishes a module from inside a module body does not strand or start its importers early #832 has them.Random graphs. 7,800 graphs of 2 to 6 modules with cycles, top-level awaits (of a settled value, or of a promise that the test resolves later), throws, and bodies that call
import()for modules of the graph and thendrainMicrotasks(). A checker imports each module once more after the evaluation. Against #832 alone: the bodies run in the same order in all 7,800, and no later import changes. #832 alone leaves an import pending, or settles it differently from a later import, in 3,133 graphs. This change does in none. A debug shell ran 4,500 of them with no failed assertion. oven-sh/bun has the generator in its pull request for this (test/js/bun/resolve/import-evaluating-fuzz.ts), with seeds in a test.Which test fails without which line. WebKit CI runs
JSTestswithout assertions. On a release shell, each of these makes graphs fail by value when it is taken out: the branch inevaluate(), the call before the pop, the call after the pop, the call in step 9.a, the loop over the members of the cycle, thepipeFrombranch (cycle-members,async-cycle-members,async-cycle-with-the-module-of-the-evaluate), each arm ofsettleAdoptedEvaluationWaiter(), the lookup inevaluationWaiter()(dependency), the change inModuleRegistryEntry::error()(imported-after-a-job-rejected-it) and the loop inVM::visitAggregateImpl(collected-while-waiting). With the two checks of #832 put back, no result changes.Cost.
sizeof(VM)109,184 to 109,192 (oneHashMap).sizeof(JSModuleRecord)is 384 before and after.JSPromise::createcalls and microtask queue entries perimport(), from gdb breakpoints on debug shells (20 and 60 imports): 8 and 12 for an evaluated module, 15 and 16 for a new one-module graph, before and after.Evaluate()of a record on a stack makes one promise, asmaindoes. A record whose cycle root has a capability already costs one more job (pipeFrom).maintakes 0.93 s and 0.62 s and leaves the 20,000 imports pending. A pop looks up the records that it pops, and only while a promise waits.Evaluate(), and the sizes of the functions. They need the release build of this pull request, to compare with the one of [JSC] A job that finishes a module from inside a module body does not strand or start its importers early #832.Self-review. Not done: an issue for this bug (this account cannot open one), and landing oven-sh/bun#41997 before this (it is a separate pull request).
Not changed.
import()of a module that is not evaluated yet and imports a module on the stack runs that module at once, inside the job ([JSC] Tolerate cross-DFS Evaluating module records on require(esm) re-entry #204). When its body reads a binding of the module on the stack, the import rejects with aReferenceError.importer-of-module-on-the-stackpins that this change leaves it. For arequire()of an ES module, require(esm): do not run an import() started by the required module inside the require() bun#41997 keeps the job out of therequire(), which is the change for that case and for the order of the bodies there.node:vm:SourceTextModule.evaluate()for a record that is EVALUATING now returns a promise that settles after the walk (it never settled before). Node.js rejects it withERR_VM_MODULE_STATUS. node:vm: module.evaluate() runs one Evaluate() of the module record bun#46004 reworks that function.