event loop: park on the next timer deadline when nothing refs the loop - #39107
event loop: park on the next timer deadline when nothing refs the loop#39107robobun wants to merge 1 commit into
Conversation
auto_tick only slept in the poll while the uws loop had active handles. Timers do not ref the loop (unref'd setTimeout, the bun:test per-test timeout, the idle GC timer), so a wait loop built on tick(); auto_tick(); (the test runner, wait_for_promise behind expect().resolves and the entry point's top-level await) polled without blocking and spun at 100% CPU until the deadline. Compute the poll deadline for the idle case as well and, when a timer is armed and no work is queued, park until it, bracketing the tick with inc()/dec() so it does not early-return on an empty poll set (the same shape MiniEventLoop::tick_once uses). Without a deadline the idle branch still returns at once, so a wait nothing will ever wake does not park forever. auto_tick_active is unchanged: the run-to-completion loops must not sleep once the loop is idle.
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: closed as a duplicate of #32014.
|
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes when the core event loop parks vs. returns immediately — and removes a comment that previously said get_timeout must stay guarded by is_active() — a maintainer look would still be worthwhile.
What was reviewed:
get_timeoutnow runs unconditionally; verified dueWTFTimers would fire right after the poll indrain_timersanyway, so this only moves them earlier.inc()/dec()bracketing matches the existingMiniEventLoop::tick_oncepattern;timespec != EPOCHand!has_pending_immediateguards keep the Windows path (which ignores the timespec) from parking with work queued.process_gc_timer()now runs beforeget_timeouton the idle path too, so the GC timer's deadline is included in the park.- Test fixtures cover each affected wait loop (test runner,
expect().resolves, top-level await) and assert CPU time, not wall time.
Extended reasoning...
Overview
This PR changes auto_tick in src/runtime/jsc_hooks.rs so that when the uws loop has no active handles but a timer (unref'd setTimeout, the bun:test per-test timeout, or the idle GC timer) is armed, the loop parks in the poll until that timer's deadline instead of returning immediately via tick_without_idle(). The other two files are a doc-comment update in src/jsc/event_loop.rs and four new subprocess fixtures in test/js/bun/test/test-timers.test.ts that measure process.cpuUsage() over a 1s idle wait.
Security risks
None. This is event-loop scheduling; no untrusted input, auth, or crypto is involved.
Level of scrutiny
High. auto_tick is the poll step of every tick(); auto_tick(); wait loop in the runtime — the test runner's per-file loop, wait_for_promise (backing expect().resolves/.rejects and entry-point loading), and AnyEventLoop::tick_raw. A mistake here manifests as either a hang (parking when nothing will wake the poll) or a busy-spin regression. The diff also deletes a comment that explicitly said get_timeout must stay inside the is_active() guard because it has side effects; the PR description argues convincingly that firing due WTFTimers a moment earlier is harmless (drain_timers right after the poll would fire them anyway), but that is exactly the kind of invariant a maintainer who wrote the original guard should confirm.
Other factors
The reasoning is unusually thorough: it enumerates every wake source that must end the park (timer deadline as the poll timeout, registered fds, cross-thread wakeup() from VmHandle::deliver/add_keep_alive/request_termination/JSCTaskScheduler), explains why inc()/dec() are needed on both POSIX (num_polls == 0 early-return) and Windows (libuv needs a live handle), and cites MiniEventLoop::tick_once as precedent for the same shape. The Windows path is subtle — tick_with_timeout ignores the timespec there and relies on timer::All::insert having armed a real libuv timer via ensure_uv_timer — and auto_tick_active is deliberately left unchanged so bun run's run-to-completion semantics are preserved. The tests are well-designed (concurrent, hermetic, assert CPU not wall time, cover all four affected wait loops, verified to fail on unfixed main). I did not find a case where the new branch would park without a wake source, but given the blast radius and the removed guard comment, a human should sign off.
|
Closing in favor of #32014, which fixes the same spin (reported again via a separate trace of
#32014 currently conflicts with main and needs a rebase; noted there. |
Problem
bun testspins at 100% CPU while it waits for a test that nothing will settle:time bun test e.test.ts --timeout 1000ontest("t", async () => { await new Promise(() => {}); })reportsuser 1.00sforreal 1.02s. The same spin happens for a test awaiting an unref'dsetTimeout, forexpect(p).resolveswhilepwaits on one, and forbun runof a file whose top-level await waits on one (all measured at ~1000 ms of CPU per 1000 ms of waiting on the release binary).auto_tick(src/runtime/jsc_hooks.rs, theif (*loop_).is_active()branch) only sleeps in the poll while the uws loop has active handles; otherwise it callstick_without_idle(), which returns immediately. Timers never ref the loop: the bun:test timeout (BunTest::update_min_timeout->timer::All::insert), an unref'dTimeoutObject, the idle GC timer. So every wait loop built ontick(); auto_tick();(test_command.rswhilephase != Done,EventLoop::wait_for_promiseused byexpect().resolves/.rejectsandload_entry_point,AnyEventLoop::tick_raw) polls non-stop until the deadline arrives.Fix
auto_ticknow computes the poll deadline (timer::All::get_timeout) whether or not the loop is active. When it is idle but a timer is armed and no task or immediate is queued, it parks in the poll until that deadline, bracketing the tick withinc()/dec()sous_loop_run_bun_tickdoes not early-return onnum_polls == 0(on Windows: souv_runsees a live handle). This is the shapeMiniEventLoop::tick_oncealready uses to park.VmHandle::deliver,add_keep_alive,request_termination, JSC deferred work viaJSCTaskScheduler) already callswakeup(), which the active branch has always relied on. Same-thread work queued before the poll makesget_timeoutreturn a zero timeout, which is excluded from parking. With no armed timer the idle branch still returns at once, so a wait that nothing will ever wake keeps its old behavior rather than parking forever.auto_tick_active(thebun run/on_before_exitrun-to-completion loops) is unchanged; those must return as soon as the loop is idle.get_timeoutrunning for an idle loop only fires dueWTFTimers before the poll instead of in the timer drain right after it.test/js/bun/test/test-timers.test.ts, four fixtures (per-test timeout, unref'd timer in a test,expect().resolveson one, top-level await on one inbun run) each wait 1 s and reportprocess.cpuUsage()over the wait; assert under 500 ms. Unfixed debug build of main: ~975-1000 ms each (all four fail). Fixed debug (ASAN) build: 26-40 ms each.bun bd test test/cli/test/ test/js/bun/test/ test/js/web/timers/ test/js/bun/test/expect/ test/bundler/bundler_plugin*.test.ts test/js/bun/repl/repl.test.ts test/cli/run/run-eval.test.tsand a few more: the remaining failures (parallel/randomize timeouts, RSS leak fixtures,spawn_waiter_thread) fail identically on a debug build of unmodified main in this container.import(),Bun.file().text(),WebAssembly.instantiate, zlib, subtle crypto,Bun.spawnexit, workers and immediates complete in the same time as with the loop held active (details below).tick_without_idle(), so it would still spin; this change is theelsebranch of the same poll and carries over.Background
KeepAlive, ref'd timers viaincrement_timer_ref, sockets, subprocesses)ref()the uws loop.is_active()is whatbun runuses to decide the program is done, which is why unref'd and internal timers deliberately do not count.num_pollsis a second counter (ref'd handles plus registered fds);us_loop_run_bun_tickreturns without polling when it is 0, so parking needs it non-zero, henceinc()/dec().auto_tickvsauto_tick_active: both do one poll of the I/O loop.auto_tickserves loops waiting for a condition (promise settled, test file done);auto_tick_activeserves loops that exit when the loop goes idle.timer::All::get_timeoutreturns the time until the soonest entry of the JS timer heap and the JSC (WTFTimer) heap, zero if work is already queued, andfalsewhen both heaps are empty.tick_with_timeoutignores the timespec and runsuv_run(UV_RUN_ONCE); libuv derives the sleep from its own timer, whichtimer::All::insertarms for the heap's soonest deadline (ensure_uv_timer), andinc()is the virtual handle that letsuv_runblock at all. That is why the idle branch also checks for pending work explicitly instead of relying on the zero timespec.Probe numbers (debug ASAN build with the fix, same binary, loop idle vs held active by a ref'd timer)
Release binary before the fix, CPU burned during a 1000 ms wait, per fixture in the new test: 1000.2 / 998.1 / 998.5 / 1001.6 ms. Fixed debug build: 26.5 / 40.3 / 35.3 / 32.0 ms.