Repository navigation
process.nextTick: run the tick pass only when a tick is scheduled - #43826
Conversation
The first read of `process.nextTick` creates the tick queue. When the tick queue exists, every event loop task drains it. `node:fs` read the property at load, so a program that imports `node:fs` and never calls `process.nextTick` paid for the drain in every `setImmediate` and `setTimeout` callback. The callback APIs that still need `process.nextTick` (`cp`, `rm`, recursive `rmdir`, `Dir.read`, `Dir.close`, `glob`, `opendir`, the aborted-signal path of `stat`, and the Windows `realpath` loop) read it when they call it.
|
Status: the rework is pushed (head a0fe7ab). The PR now changes the checkpoint in CI on a0fe7ab (build 120123): 180 of 181 jobs passed, on every platform. The diff is green. The one failed job does not come from this PR:
How I reproduced the problems, on a release build of main (
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughThe changes update next-tick queue draining and ChangesProcess tick ordering
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No actionable issue is identified; the change is ready for normal merge checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Beyond the inline findings, I also checked the fs.access aborted-signal early return and the three Windows realpath LOOP re-entries — they share the replaced-nextTick exposure of the first finding rather than adding a new one. The beforeExit and first-tick ordering findings trace to the lazy tick-queue creation and the m_nextTickQueue guard in Process__dispatchOnBeforeExit (src/jsc/bindings/BunProcess.cpp:865), not to fs.ts logic, so the layer to fix them at is worth a human call.
Extended reasoning...
The change removes the module-load capture of process.nextTick in src/js/node/fs.ts and reads it at 11 call sites, plus two subprocess tests in test/js/node/fs/fs.test.ts; no security-sensitive surface is touched. Three confirmed findings are posted inline, and the remaining call sites were checked and found to be the same class as the first finding. The two ordering/beforeExit findings are pre-existing runtime behaviors that this PR newly exposes for fs-only programs, which is a layering decision a maintainer should weigh.
Findings marked 🟡 are optional suggestions and need no follow-up push.
Every checkpoint called `JSNextTickQueue::drain` once a tick queue existed, and a tick queue exists from the first read of `process.nextTick`. With an empty queue that call drained the microtasks, and the caller drained them a second time. After the first tick of a process the scheduled flag stayed set, so every later checkpoint also called `processTicksAndRejections` in JS to find an empty queue. `GlobalObject::drainMicrotasks` now runs the tick pass only when the flag is set, drains the microtasks once, and reads the flag again for a tick that a microtask scheduled. `processTicksAndRejections` clears the flag when the queue is empty. `JSNextTickQueue::isEmpty` is inline. `node:fs` keeps its load-time capture of `process.nextTick`: this reverts the call-site reads of the previous commit. The capture costs nothing now, and a `process.nextTick` that user code replaces does not hold the callbacks of `fs.cp`, `fs.rm`, `fs.opendir`, `Dir.read`, `Dir.close` and `fs.glob`. `Process__dispatchOnBeforeExit` ends with that same checkpoint. It drained only when a tick queue existed, so without one the microtasks that a 'beforeExit' listener queued never ran.
|
Updated 6:11 PM PT - Sep 23rd, 2026
❌ @robobun, your commit a0fe7ab has 1 failures in 🧪 To try this PR locally: bunx bun-pr 43826That installs a local version of the PR into your bun-43826 --bun |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked whether the scheduled flag can read 0 while the queue holds a tick: it is set on every push (the native Process::queueNextTick goes through the same JS nextTick), cleared only after processTicksAndRejections sees the queue empty, and a throwing tick leaves it set so JSNextTickQueue::drain re-enters — nested checkpoints from inside a tick stay consistent. The re-check after vm.drainMicrotasks() reproduces the base branch's ordering for ticks queued by microtasks, and the beforeExit change reports exceptions inside drainMicrotasks() rather than propagating them, so the trailing RETURN_IF_EXCEPTION is dead but harmless.
Extended reasoning...
The net diff no longer touches src/js/node/fs.ts; it reworks GlobalObject::drainMicrotasks in src/jsc/bindings/ZigGlobalObject.cpp to skip the JS drain call unless the tick queue's scheduled flag is set and re-check after draining VM microtasks, clears that flag at the end of processTicksAndRejections in src/js/builtins/ProcessObjectInternals.ts, and makes Process__dispatchOnBeforeExit drain microtasks unconditionally. No security-sensitive surface is touched. The checkpoint is core event-loop code and the PR title, description and perf numbers describe the reverted fs.ts change rather than this diff, so a maintainer should weigh the actual change.
…ange A promise reaction at an idle checkpoint must not have `processTicksAndRejections` on its stack. It fails on main, and it fails when the reset of the scheduled flag is deleted. The idle-path cases use two immediates of one batch, so that one checkpoint decides the order. The case with a tick that a microtask schedules fails when the second read of the flag is deleted. The fs test for the first tick of a process is gone. With the capture back in `node:fs` it only repeated the `rm` case of the table above it.
There was a problem hiding this comment.
I re-reviewed after a0fe7ab and found no bugs; the new process-nexttick tests now pin each clause of the checkpoint change, which resolves my earlier test nit. Because this rewrites the microtask/nextTick checkpoint that every event loop callback runs through, and changes what 'beforeExit' drains, a human look is still worthwhile.
What was reviewed:
- Traced old vs new
GlobalObject::drainMicrotaskscontrol flow: the gated first pass plus the post-microtask re-read reproduces the old empty-queue path (drain → vm.drainMicrotasks → tick loop), and the queue pointer is re-read from the WriteBarrier after JS ran, not held across it. isEmpty()now compares againstjsNumber(0)instead ofasNumber(); every writer ($putInternalField(..., 0, 0/1),discard) stores an int32, and the flag reset inprocessTicksAndRejectionssits after the do-while exits with the queue empty, so no tick can be stranded with flag 0.Process__dispatchOnBeforeExitcalling the full checkpoint: a termination from a listener is taken viatakeTerminationOutsideScriptand the Rust loop stops onscript_allowed(), matching the old pending-exception path.- Note the PR description still describes removing the
process.nextTickcapture fromsrc/js/node/fs.ts; the net diff leaves fs.ts untouched (the capture at fs.ts:28 remains), so the description should be updated to match the checkpoint change the title names.
Extended reasoning...
The net diff is about 40 lines of C++ across ZigGlobalObject.cpp, BunProcess.cpp and JSNextTickQueue.{h,cpp}, a two-line flag reset in the ProcessObjectInternals.ts tick loop, and three new subprocess tests; src/js/node/fs.ts is not in the diff despite the description. It touches no security-sensitive surface (no auth, parsing of untrusted input, or crypto) but does touch the event loop microtask checkpoint and beforeExit dispatch, which every callback in the runtime runs through. The change traced as behavior-preserving on the idle, tick-first, microtask-scheduled-tick, exception and termination paths, and the a0fe7ab tests cover each clause, which is why this is not a block; the hot-path criticality and the stale PR description are why a human should still look rather than an auto-approve. No CODEOWNERS entry covers the changed files, and no third-party CHANGES_REQUESTED review is outstanding.
Problem
process.nextTickcreates the tick queue, andnode:fs(since node:fs: call callbacks from the native completion, not a promise reaction #43728) andnode:streamread it at load. Every checkpoint (Zig::GlobalObject::drainMicrotasks) then callsJSNextTickQueue::drain: +124 instructions persetImmediatecallback.processTicksAndRejectionsin JS for an empty queue: +2796 instructions per callback (+598 with the JIT).Process__dispatchOnBeforeExitdrains nothing, so microtasks queued by abeforeExitlistener never run. Node runs them.Fix
processTicksAndRejectionsclears the flag when the queue is empty. Costs now: +5 and +8.beforeExitends with that same checkpoint.node:fskeeps its capture. Without it (the first commit), a replacedprocess.nextTickheld seven callbacks.process-nexttick.test.js,process.test.js(two new tests fail on main),fs.test.ts, 108 node tests.Background
Zig::GlobalObject::drainMicrotasksis Bun's checkpoint, not JSC's microtask drain. After each event loop callback it runs the scheduledprocess.nextTickcallbacks, thenvm.drainMicrotasks().process.nextTick()call.node:streamusers would then get the first-tick order below.Downsides
Notes
All numbers compare release builds, Linux x64: main
dc55830bband this PR (based on6d504dd98, four unrelated commits later).Calls, exact
Breakpoint hit counts from gdb on the unstripped release binaries, over 10,000
setImmediatecallbacks, each scheduled from the one before it.GlobalObject::drainMicrotasksruns 20,004 times in every row.JSNextTickQueue::drainon mainimport "node:fs"import "node:stream"process.nextTickprocess.nextTick(() => {})callA
node:httpserver with a keep-alive client in the same process, 300 requests one after the other: 1,509 checkpoints on both builds.JSNextTickQueue::drainruns in 1,509 of them on main and in 603 with this PR. Those 603 have a tick scheduled.Instructions per callback
perf_event_openreturnsEPERMon this machine, so the counts come fromqemu-x86_64 -one-insn-per-tb -d exec,nochain, which logs each guest instruction with its thread. I count the main thread only.BUN_JSC_useGC=0keeps collections out of the count. Each value is the slope between two N, so startup cancels, and it is the median of 3 runs. The table gives the difference against a script that never touchesprocess.nextTick, on the same build.JIT off (
BUN_JSC_useJIT=0), N = 4,000 and 14,000:setImmediateup front (one checkpoint per callback, 1809 instructions with no tick queue)import "node:fs"import "node:stream"process.nextTick()callsetTimeout(fn, 0)up front (2401)import "node:fs"process.nextTick()callsetImmediate(two checkpoints per callback, 3214, runs spread by about 100)import "node:fs"process.nextTick()callDefault JIT, N = 20,000 and 60,000,
setImmediateup front (1433 instructions with no tick queue):import "node:fs"process.nextTick()callEvery callback schedules a tick (
setImmediate(() => process.nextTick(noop))), JIT off. With 14,000 of them up front, every checkpoint has a tick pending: 9756.9 instructions per callback on main, 9840.5 with this PR (+83.6, +0.9%). With the default JIT and N = 20,000 and 60,000: 2432.1 and 2436.2 (+4.1, +0.2%). With chainedsetImmediatethe second checkpoint of each callback is idle, and the same callback costs 15,063 on main and 11,983 with this PR.The binary size does not change: the stripped
bunis 80,823,840 bytes on both builds.What a script can see
After one tick, main runs every promise reaction inside the JS tick pass, so
processTicksAndRejectionsis on its stack:at <anonymous> (file:1:86), thenat processTicksAndRejections (native:7:39)beforeExitNode v26.3.0 and this PR print
microtask,tick,exit. Main and bun 1.4.3-canary.1+367d939d9 printexit. With one read ofprocess.nextTickat the top, main prints all three, because the tick queue then exists.The first commit and the review
The first commit removed the capture from
fs.tsand readprocess.nextTickat the call sites. The review found three things, and I confirmed each on node, the last release, main and that commit:process.nextTickthat user code replaces held the callbacks ofcp,rm, recursivermdir,opendir,Dir.read,Dir.closeandglob. Node holdscpand a bufferedDir.read. The capture is back, and a new test infs.test.tscovers the eight callbacks.beforeExitbug above. node:fs: call callbacks from the native completion, not a promise reaction #43728 hid it for programs that loadnode:fs, because its capture created the tick queue.onEachMicrotaskTick) fixes it, but the hook is also what runs the ticks of a CommonJS entry point before its promise jobs: without it a.cjsfile that doesprocess.nextTick(t); Promise.resolve().then(m)printsmicrotask tick, and the existing test "a tick that throws goes to uncaughtException..." fails. Run process.nextTick callbacks before promise jobs in a CommonJS entry point after a preload or in a node worker #43366 arms that drain from the evaluation of the entry point. After it, the hook can go.Verification
process-nexttick.test.js:a promise reaction does not run inside processTicksAndRejectionsfails on main (reaction: true) and when the flag reset is deleted. The idle-path case with a tick that a microtask schedules fails when the second read of the flag is deleted (microtask microtask 2 next immediate). 11 pass with this PR.process.test.js: the newbeforeExittest fails on main (printsexitonly). Whole file with the ASAN debug build: 173 pass, 1 fail. The failure isprocess, which needsprocess.env.USER, and this container has none.fs.test.ts, theprocess.nextTicktests: 46 pass.test/parallelfiles for next-tick, microtask, promise, async-hooks,AsyncLocalStorage, timers,beforeExit, exit, stream destroy, vm microtask and worker exit: 108 files pass.test/js/node/{async_hooks,timers,vm,events}andtest/js/web/timers, ASAN debug build: 680 pass, 7 fail. All 7 are memory or time limits of the debug build: five RSS leak checks whose threshold is not widened for a debug binary that is not namedbun-asan, and two 5 s timeouts. The threesetTimeoutleak fixtures give 0 to 3 MB on both release builds (limit 10 MB).BUN_JSC_validateExceptionChecks=1on thebeforeExitand idle-path scripts: clean.no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/process/process.test.js, test/js/node/fs/fs.test.ts