Skip to content

timers: take the event loop ref in Immediate.ref() only while the Immediate is queued - #48576

Open
robobun wants to merge 1 commit into
mainfrom
robobun/be0279f6/immediate-ref-in-callback
Open

robobun wants to merge 1 commit into
mainfrom
robobun/be0279f6/immediate-ref-in-callback

Conversation

@robobun

@robobun robobun commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • ref() after unref() inside an Immediate's own callback takes an event loop ref that stays while the handle is reachable: const a = setImmediate(() => a.ref()); setImmediate(() => {}); a.unref(); never exits. Node exits. Regressed in v1.2.10 (Fix setImmediate slowness #18889). No user report.
  • do_ref (src/runtime/timer/timer_object_internals.rs:999) takes the ref when !get_destroyed(). That is true inside the callback, and for an unref'ed Immediate that bun removed from the queue unrun.

Fix

  • ref() takes the ref for a setImmediate only while the wrapper pin says it is queued. A debug assertion catches a call site that skips the gate.
  • Only the timer heap sets in_callback. Inside its callback an Immediate now reports _destroyed === true and hasRef() === false, as in Node.
  • Verified: test/js/node/timers/node-timers.test.ts (10 tests fail on main). Also test/js/web/timers/, fake-timers.test.ts, 56 vendored test-timers-*.js.
  • Self-reviewed: 4 concerns raised, 4 addressed, one part declined (Notes).

Background

  • A setImmediate runs from the immediate queue. Timeouts and faked Immediates fire from the timer heap.
  • The wrapper pin (this_value) is a strong GC handle until the Immediate leaves the queue.
  • in_callback keeps _destroyed false during a callback, as Node does for Timeouts.
  • Considered a get_destroyed() arm alone: the removed Immediate still hangs.
Notes

Where it came from. A generated program that mixes ref() and unref() on Immediates did not end on bun and ended on Node. Review of #30069 had named the same gap (unref(); ref() inside a setImmediate callback) and left it for a follow-up that aligns _destroyed for Immediates. The two repro shapes exit on bun 1.2.9 and hang on 1.2.10.

How long the ref stays. deinit() gives the ref back when the wrapper is collected. So the process stays alive for good while the handle is reachable: the script above as an ES module or with bun -e was still running after 60 s on main. As a CommonJS file, where nothing holds a after the module body, it exited after about 11 s. In a Worker the worker had not exited after 45 s on main, and it exits here.

Measurements. Two release builds from one toolchain: main 89452d2 and this change. Instruction counts come from gdb (nexti inside the function, or stepi until it returns), two equal runs each.

main PR
__bun_run_immediate_task, bytes 1197 1158
__bun_run_immediate_task, instructions per ref'ed setImmediate (exclusive) 142 136
in_callback stores in __bun_run_immediate_task 2 0
fire_catching, bytes 2042 2040
fire_catching, instructions per setTimeout(fn, 0) callback 218 217
fire_catching, instructions per setInterval tick 253 252
Timeout.ref() after unref(), instructions (inclusive) 71 84
Immediate.ref() after unref(), still queued 77 88
ref() when already ref'ed, Timeout and Immediate 27 27
_destroyed getters, instructions 19 19 (0 differing instructions in 4 functions)
Bun__Timer__setImmediate, instructions (exclusive / inclusive, second call) 32 / 1004 32 / 1004
Bun__Timer__setTimeout, instructions (exclusive) 36 36
TimerObjectInternals::init, instructions (setImmediate / setTimeout) 106 / 97 106 / 97
init and set_enable_keeping_event_loop_alive, bytes 1201, 220 1201, 220
ref host functions plus the new out-of-line function, bytes 147 + 147 94 + 94 + 111
allocations per chained setImmediate (mimalloc malloc_normal_count slope) 1.007 to 1.011 1.007 to 1.011
epoll_pwait2 catchpoint hits for 20000 and 40000 chained setImmediate 39998, 79998 39998, 79998
release text, size build/release/bun 143094946 143094946

TimerObjectInternals stays 40 bytes. TimerFlags stays at 32 of 32 bits. The diff of EventLoopTimer.rs is one comment line.

Two earlier shapes of this change were dropped for cost. A pin of the wrapper before the kind branch in init() added 3 instructions per setImmediate() and 2 per setTimeout(). So init() is unchanged, and the assertion accepts the empty slot that init() has when it takes the ref. With the gate inline, LLVM stopped inlining do_ref into the two host functions, which added 18 instructions to every ref(). So the path after unref() is one out-of-line function.

A smaller shape that was not taken. Mark the removed Immediate cleared in run_immediate_task and keep the !get_destroyed() test in do_ref. That needs no new function. It makes _destroyed true for an Immediate that bun removed unrun, where Node prints false, and it adds a store to the path of every cleared Immediate.

The assertion. It reads the same pin as the gate, so it catches a future call site that enables the ref without the gate. It does not catch a pin that no longer matches the queue.

Probes against Node v26.3.0. 26 scripts that mix ref(), unref(), clearImmediate, beforeExit, uncaughtException, a Worker and timers. On main 17 print something else than Node, and in 11 the loop is still held when the guard timer of the script fires or at the 6 s limit. With this change 8 differ and none of the 26 hangs. The 8 are:

  • ref() on an Immediate that bun removed unrun, 4 scripts. One of them matched Node on main: it called unref() again at once.
  • An unref'ed Immediate that never runs where Node runs it, with no ref() involved, 2 scripts.
  • Timeout.hasRef() after the timer fired or was cleared, 2 scripts. Node keeps the last ref()/unref() state. timers: Timeout.hasRef() reports the sticky ref flag after clearTimeout #34082 changed that and closed unmerged.

Each clause has a failing test. On debug builds with one clause removed:

  • Without the pin read: the beforeExit test fails. The child aborts with panic: an Immediate that left the queue took an event loop ref.
  • With in_callback set from the immediate queue again: the 8 script rows and "is true during the callback of an immediate" fail on the _destroyed value. No row hangs, so the in_callback move can be split out if the _destroyed change is not wanted.
  • Without !fake(): "ref() inside the callback of a faked Immediate takes the ref" fails. That test passes on main too. It pins the timer heap behaviour of a faked Immediate.

Suites. Release builds, main against this change, node-timers.test.ts plus all of test/js/web/timers/ plus fake-timers.test.ts: 610 pass and 10 fail on main, 620 pass and 0 fail here. Debug (ASAN) build: node-timers.test.ts 44 of 44, setTimeout.test.js, setImmediate.test.js, fake-timers.test.ts 497 of 497, 56 vendored test-timers-*.js, test-process-ref-unref.js. On the debug build timer-gc-roots.test.ts has 2 tests and setInterval.test.js has 1 test that exceed their time limits. Their fixtures finish with the right output without the limit, they do not use setImmediate, and both files pass on the two release builds.

The tests spawn with timeout. bun test does not kill the child of a concurrent test that times out, so a failing run without it leaves hung children.

Still unlike Node after this change.

  • bun drops an unref'ed Immediate when nothing holds the loop at the moment it is reached. A ref'ed Immediate that already ran in the same batch no longer counts, so setImmediate(a); setImmediate(b).unref() runs only a, where Node runs both in one pass. For the same reason timers/promises setImmediate(v, { ref: false }) behind a plain setImmediate does not settle. When nothing else holds the loop, Node keeps the Immediate queued and runs it if beforeExit revives the loop.
  • Two shapes that never exit and do not go through ref() are unchanged: a beforeExit listener that queues an unref'ed Immediate, and a top-level await of setImmediate(v, { ref: false }).
  • Timeout.hasRef() after fire or clear (above).
  • A faked Immediate (jest.useFakeTimers({ toFake: ["setImmediate"] })) keeps _destroyed === false and hasRef() === true inside its callback.

The first two need the drop rule in run_immediate_task and loop liveness (is_event_loop_alive) to change. That is a separate change.

Self-review. Four concerns. (1) The spawned children had no timeout, so a failing run left hung children: added. (2) The beforeExit test printed nothing after ref(): it now prints hasRef() after ref() and states what Node prints. (3) The comment above the gate cited Node's Immediate rule under a sentence about _destroyed, and the source did not say that ref() on a removed Immediate differs from Node: the comment now covers Timeouts only and two one-line comments cover Immediates. (4) "for good" was too strong: see "How long the ref stays". Declined: an in-process test of the removed Immediate. It passes only if nothing else holds the loop of the test runner, which the test does not control.

Merge order. #48149 and #44806 also edit fire_catching.

Windows. Not run. There the Immediate loop ref is a started uv_idle handle (increment_immediate_ref), which keeps uv_run from sleeping in poll. So by reading, the leaked ref on main also keeps the loop busy there.

Downsides

  • Immediate._destroyed inside its own callback changes from false to true. This reverses an assertion from node:timers fixes #16855.
  • ref() on an unref'ed Immediate that bun removed unrun now does nothing: hasRef() stays false. Node runs it. Before, bun hung.
  • ref() after unref(): 13 more instructions for a Timeout (71 to 84), 11 for a queued Immediate.

no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/timers/node-timers.test.ts

…ediate is queued

ref() after unref() took an event loop ref for an Immediate that had left the
immediate queue: inside its own callback, or after the idle loop dropped it.
The ref came back only when the wrapper was collected, so the process stayed
alive while the handle was reachable.

An Immediate on the immediate queue now reports _destroyed === true inside its
callback, as in Node.js: only the timer heap sets in_callback.
@robobun

robobun commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:10 AM PT - Nov 11th, 2026

✅ @robobun, your commit 9fac4f208d1499742e1d3e0cd707b7434f7a8638 passed in Build #140143! 🎉


🧪   To try this PR locally:

bunx bun-pr 48576

That installs a local version of the PR into your bun-48576 executable, so you can run:

bun-48576 --bun

@robobun

robobun commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. One decision is open: Immediate._destroyed inside its own callback changes from false to true (see Downsides).

How I reproduced it: const a = setImmediate(() => a.ref()); setImmediate(() => {}); a.unref(); run with bun file.js or bun -e. On a release build of main (89452d2) the process is still running after 60 s. Node v26.3.0 exits at once. bun 1.2.9 exits, bun 1.2.10 hangs.

With this change the same script exits in about 20 ms. test/js/node/timers/node-timers.test.ts has 10 tests that fail on main and pass here.

CI, build 140143: 184 of 188 jobs passed, on Linux (glibc, musl, ASAN), Windows and macOS arm64. No lane reported a failure in node-timers.test.ts. The red check comes from the 4 darwin x64 - test-bun shards. They did not run: they expired twice after one hour in the queue while every agent of that queue was busy, and they are queued a third time. Six other tests failed once and passed on a retry (bun-build-compile, spawn_waiter_thread, node-http2, fetch-leak, inspect, filesink). None of them uses ref() or _destroyed on an Immediate.

@coderabbitai

coderabbitai Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Timer internals now bracket callback execution with in_callback updates and determine whether ref() can keep the event loop alive based on timer type and queue state. Tests cover Immediate reference behavior across callbacks, exceptions, beforeExit, and fake timers.

Changes

Immediate timer lifecycle

Layer / File(s) Summary
Callback flag lifecycle
src/event_loop/EventLoopTimer.rs, src/runtime/timer/timer_object_internals.rs, test/js/node/timers/node-timers.test.ts
fire_catching now sets and clears in_callback around Self::run. The documentation clarifies that queued setImmediate timers do not set the flag, and the callback-time _destroyed test no longer includes setImmediate.
Immediate keep-alive eligibility
src/runtime/timer/timer_object_internals.rs, test/js/node/timers/node-timers.test.ts
do_ref now checks whether a non-fake Immediate is queued before enabling keep-alive. Added tests cover Immediate ref() behavior during callbacks, exceptions, beforeExit, and fake-timer execution.

Possibly related PRs

  • oven-sh/bun#47109: Changes the shared-reference model used by EventLoop and TimerObjectInternals, which this timer implementation uses.
  • oven-sh/bun#47871: Changes the timer run path and reference updates that this PR also modifies.
  • oven-sh/bun#47231: Changes the event-loop reference-counting methods used by timer keep-alive handling.

Suggested reviewers: jarred-sumner

Merge Risk: Minimal · up to 9fac4

This change gates Immediate.ref() on queue state to fix a case where the process stayed alive. No actionable merge-blocking risk was found in the supplied evidence.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: limiting event-loop references from Immediate.ref() to queued Immediates.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description provides a detailed explanation of what the PR changes and how the changes were verified. It does not use the template headings exactly, but it covers both required topics through the …
Full details: Description check

Explanation

The description provides a detailed explanation of what the PR changes and how the changes were verified. It does not use the template headings exactly, but it covers both required topics through the Problem, Fix, and Suites 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.

Share

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 447ff87 and 9fac4f2.

Files selected for processing: 3

  • src/event_loop/EventLoopTimer.rs
  • src/runtime/timer/timer_object_internals.rs
  • test/js/node/timers/node-timers.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I traced every transition of this_value for a real setImmediate (init pins it after taking the ref; run_immediate_task on both the dropped and run branches, cancel_pending_immediate, and cancel all downgrade it) and found no path that leaves a stale strong pin, so the is_queued_immediate gate cannot re-take the loop ref on a cleared or already-run Immediate; the in_callback bracket moved into fire_catching has no early return between set and clear and update_flags re-reads the Cell, so a re-entrant cancel() during a Timeout callback is still not clobbered. Not run here: the new subprocess tests, the USE_SYSTEM_BUN=1 failure check, and the clause-removal check the description reports.

Extended reasoning...

The change is confined to the timer keep-alive logic in src/runtime/timer/timer_object_internals.rs (a new pin-based gate in ref() for real Immediates, a debug assertion, and moving the in_callback set/clear from run() to fire_catching), a one-line doc update in src/event_loop/EventLoopTimer.rs, and new node-timers tests; it touches no security-sensitive surface. Two confirmed findings are posted inline (the beforeExit divergence from Node and the faked-vs-real Immediate _destroyed inconsistency), which is why this is a defer rather than an approve; no CODEOWNERS entry covers the changed files.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants