Skip to content
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions src/jsc/VirtualMachine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2412,6 +2412,7 @@
if self.is_watcher_enabled() {
// accessed here (no overlapping `&mut EventLoop`).
self.event_loop_mut().perform_gc();
self.run_entry_point_body(self.pending_internal_promise.unwrap_or(promise));
Comment thread
robobun marked this conversation as resolved.
loop {
let Some(p) = self.pending_internal_promise else {
break;
Expand All @@ -2435,12 +2436,34 @@
return Ok(promise);
}
self.event_loop_mut().perform_gc();
self.run_entry_point_body(promise);
self.wait_for_promise(jsc::AnyPromise::Internal(promise));
}

Ok(self.pending_internal_promise.unwrap_or(promise))
}

/// Run the entry point's synchronous body with a full tick, then the timers
/// phase.
///
/// `tick()` evaluates the module body (on the microtask queue) and reports
/// rejections exactly as the wait loop would, so nothing about exception or
/// unhandled-rejection handling changes. It does dispatch the task queue, so
/// a port message or I/O completion the body queued still runs before the
/// timers phase (see the Known-exclusion note on the PR); `setImmediate`

Check warning on line 2453 in src/jsc/VirtualMachine.rs

View check run for this annotation

Claude / Claude Code Review

In-tree comments reference 'the PR' — non-durable after merge

The doc comment here says "(see the Known-exclusion note on the PR)", and node-timers.test.ts:255 says "see the PR's Known-exclusion note" — after merge, "the PR" is unresolvable from source without git-blame. Suggest dropping both parentheticals (the sentences already state the exclusion self-contained, as the sibling web_worker.rs comment in this PR does), or use the numbered form `PR #33509` matching existing precedent in `src/`.
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
/// callbacks wait in the immediate queue, which `auto_tick` drains, so the
/// timers phase runs ahead of them. `uv_run` opens with `uv__run_timers`.
fn run_entry_point_body(&mut self, evaluation: *mut JSInternalPromise) {
self.tick();
// SAFETY: `evaluation` is a live JSC heap cell returned by
// `reload_entry_point*` and kept alive by the module loader. A body that
// threw rejects it; Node reports the error and exits without a loop.
if crate::JSPromise::status_ptr(evaluation) == crate::js_promise::Status::Rejected {
return;
}
self.event_loop_mut().drain_expired_timers();

Check warning on line 2464 in src/jsc/VirtualMachine.rs

View check run for this annotation

Claude / Claude Code Review

run_entry_point_body lacks the unhandled_error_counter guard the worker path has

🟡 The worker path's guard at web_worker.rs:1144 checks `vm.unhandled_error_counter == 0` before `drain_expired_timers()`, but `run_entry_point_body` only checks `status_ptr == Rejected` — so identical `Promise.reject(...); setTimeout(..., 1); /*busy-wait*/` code now fires the timer under `bun run`/`bun test` but not in a worker (both matched pre-PR: neither fired). After 57ff06aa switched to `self.tick()`, that tick's trailing `handle_rejected_promises()` has already bumped the counter by the ti
Comment thread
claude[bot] marked this conversation as resolved.
}
Comment thread
claude[bot] marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.

/// Drain pending tasks/microtasks if the event loop is not currently
/// re-entered. Convenience used after top-level evaluation on
/// the `bun -e` path.
Expand Down Expand Up @@ -4573,6 +4596,12 @@
) -> Result<*mut JSInternalPromise, bun_core::Error> {
let promise = self.reload_entry_point(entry_path)?;
self.event_loop_mut().perform_gc();
// Deliberately not `run_entry_point_body`: a worker's body has to evaluate
// inside the termination-aware wait below, which stops before the next
// `tick()` once `terminate()` lands. Evaluating it on the microtask queue
// instead lets a `tick()` start with the termination exception already
// pending, and its first task dispatch trips `scope.assertNoException()`.
// `web_worker.rs` runs the timers phase before it enters the loop.
self.event_loop_mut()
.wait_for_promise_with_termination(jsc::AnyPromise::Internal(promise));
if let Some(worker) = self.worker_ref() {
Comment thread
claude[bot] marked this conversation as resolved.
Expand All @@ -4593,6 +4622,7 @@
// pending_internal_promise can change if hot module reloading is enabled
if self.is_watcher_enabled() {
self.event_loop_mut().perform_gc();
self.run_entry_point_body(self.pending_internal_promise.unwrap_or(promise));
loop {
let Some(p) = self.pending_internal_promise else {
break;
Expand All @@ -4616,6 +4646,7 @@
return Ok(promise);
}
self.event_loop_mut().perform_gc();
self.run_entry_point_body(promise);
self.wait_for_promise(jsc::AnyPromise::Internal(promise));
}

Expand Down
20 changes: 20 additions & 0 deletions src/jsc/event_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,9 @@ unsafe extern "Rust" {
/// `WTFTimer::run` — `timer` is an erased `*mut bun_runtime::timer::WTFTimer`.
/// Defined in `bun_runtime::dispatch`. Link-time resolved.
fn __bun_run_wtf_timer(timer: *mut (), vm: *mut VirtualMachine);
/// `timer::All::drain_timers`: fire every timer whose deadline has passed.
/// Defined in `bun_runtime::dispatch`. Link-time resolved.
fn __bun_drain_expired_timers(vm: *mut VirtualMachine);
/// Tag-specific shutdown release for a queued-but-never-run task. Called
/// from `release_queued_tasks_for_shutdown` (after `shutdown_for_exit`,
/// before `destructOnExit`) for every entry left in `self.tasks`.
Expand Down Expand Up @@ -463,6 +466,23 @@ impl EventLoop {
}
}

/// Fire every timer whose deadline has already passed: libuv's timers phase.
///
/// `auto_tick` runs it at the end of an iteration rather than the start, so
/// the cyclic order matches `uv_run`'s for every iteration but the first.
/// Callers about to enter the loop for the first time run one themselves,
/// so a timer that expired while the entry point was still running
/// dispatches before the tasks and `setImmediate` callbacks it queued, as
/// it does in Node.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn drain_expired_timers(&mut self) {
// The real `timer::All` lives in `bun_runtime` (cycle), so the body
// dispatches through `__bun_drain_expired_timers` (link-time extern).
let vm = self.vm();
// SAFETY: `vm` is the live owning VM; the definer no-ops when this
// thread has no `RuntimeState` (bun_jsc unit tests).
unsafe { __bun_drain_expired_timers(vm) };
}

pub fn tick_concurrent_with_count(&mut self) -> usize {
self.update_counts();

Expand Down
13 changes: 13 additions & 0 deletions src/jsc/web_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1132,6 +1132,19 @@ impl WebWorker {
let _ = vm.global().vm().run_gc(false);
}

// `uv_run` opens with `uv__run_timers`, so a timer that expired while the
// worker's entrypoint ran synchronously is due before the `setImmediate`
// callbacks it queued alongside it. Only `setImmediate` is reordered
// here: a MessagePort or fs completion the body queued dispatches inside
// the `tick()` of the wait above, the same as `--preload`. The two guards
// match the `is_event_loop_alive()` check below, so the entrypoint never
// fires a timer on a path that otherwise exits: skip once `terminate()`
// has landed (firing a callback would re-enter JS with the termination
// exception pending), and skip when the body left an unhandled error.
if !self.has_requested_terminate() && vm.unhandled_error_counter == 0 {
vm.event_loop_mut().drain_expired_timers();
}

// Always do a first tick so we call CppTask without delay after
// dispatchOnline.
vm.as_mut().tick();
Expand Down
18 changes: 18 additions & 0 deletions src/runtime/dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -894,6 +894,24 @@
unsafe { crate::timer::WTFTimer::run(real, vm) }
}

/// `__bun_drain_expired_timers` body: run the timers phase over this thread's
/// `timer::All`. No-op before `init_runtime_state` (bun_jsc unit tests).
///
/// # Safety
/// `vm` is the live per-thread VM and stays live across the JS callbacks
/// `drain_timers` fires.
#[unsafe(no_mangle)]
pub(crate) unsafe fn __bun_drain_expired_timers(vm: *mut bun_jsc::virtual_machine::VirtualMachine) {
let all = crate::jsc_hooks::timer_all();
if all.is_null() {
return;
}
// SAFETY: `all` is the live per-thread `All`; `drain_timers` forms
// short-lived `&mut` only around heap pop/peek, so no `&mut All` is held
// across `fire()` (which re-enters `All` via `runtime_state()`).
unsafe { (*all).drain_timers(vm.cast::<()>()) };

Check warning on line 912 in src/runtime/dispatch.rs

View check run for this annotation

Claude / Claude Code Review

__bun_drain_expired_timers duplicates existing drain_timers_export

The body of `__bun_drain_expired_timers` is byte-identical to the pre-existing `drain_timers_export` at `src/runtime/timer/Timer.rs:503-511` (both: `timer_all()` → null-check → `(*all).drain_timers(vm.cast::<()>())`). Since both live in `bun_runtime`, the new `#[no_mangle]` shim could be a one-line `crate::timer::timer::drain_timers_export(vm)` delegation instead of duplicating the null-guard and cast — otherwise a future change to either has to be made in two places.
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
}

// ════════════════════════════════════════════════════════════════════════════
// EventLoopTimer dispatch
// ════════════════════════════════════════════════════════════════════════════
Expand Down
107 changes: 106 additions & 1 deletion test/js/node/timers/node-timers.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import jsc from "bun:jsc";
import { describe, expect, it, mock, test } from "bun:test";
import { bunEnv, bunExe, isWindows } from "harness";
import { bunEnv, bunExe, isWindows, tempDir } from "harness";
import path from "node:path";
import { clearInterval, clearTimeout, promises, setImmediate, setInterval, setTimeout } from "node:timers";
import { promisify } from "util";
Expand Down Expand Up @@ -245,3 +245,108 @@ describe.each(["with", "without"])("setImmediate %s timers running", mode => {
it("should defer microtasks when an exception is thrown in an immediate", async () => {
expect(["run", path.join(import.meta.dir, "timers-immediate-exception-fixture.js")]).toRun();
});

describe.concurrent("an already-expired timer runs before the first check phase", () => {
// Block well past the 1ms deadline so the timer is unambiguously expired by
// the time the event loop is entered. libuv opens every `uv_run` iteration
// with the timers phase, so the timer runs before the `setImmediate` queued
// next to it. (Task-queue work such as a MessagePort self-post or completed
// fs I/O is dispatched by the entry tick before the timers phase; see the
// PR's Known-exclusion note.)
const blockPastTheDeadline = `const start = Date.now(); while (Date.now() - start < 10) {}`;

test.concurrent("before setImmediate", async () => {
using dir = tempDir("timers-phase-order", {
"index.js": `
const order = [];
process.on("exit", () => console.log(JSON.stringify(order)));
setTimeout(() => order.push("timeout"), 1);
setImmediate(() => order.push("immediate"));
${blockPastTheDeadline}
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "index.js"],
cwd: String(dir),
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

if (exitCode !== 0) {
expect(stderr).toBe("");
}
expect(stdout.trim()).toBe(JSON.stringify(["timeout", "immediate"]));
expect(exitCode).toBe(0);
});
Comment thread
robobun marked this conversation as resolved.

test("inside a worker", async () => {
const worker = new Worker(new URL("timers-phase-order-worker-fixture.js", import.meta.url).href);
const { promise, resolve, reject } = Promise.withResolvers<string[]>();
worker.onmessage = event => resolve(event.data);
worker.onerror = reject;

try {
expect(await promise).toEqual(["timeout", "immediate"]);
} finally {
await worker.terminate();
}
});

test("inside a test file, same as under `bun run`", async () => {
using dir = tempDir("timers-phase-order-test", {
"order.test.js": `
const { test, expect } = require("bun:test");
const order = [];
setTimeout(() => order.push("timeout"), 1);
setImmediate(() => order.push("immediate"));
${blockPastTheDeadline}
test("the expired timer ran first", () => {
console.log("order=" + JSON.stringify(order));
expect(order).toEqual(["timeout", "immediate"]);
});
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "test", "order.test.js"],
cwd: String(dir),
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

if (exitCode !== 0) {
expect(stderr).toBe("");
}
expect(stdout).toContain(`order=${JSON.stringify(["timeout", "immediate"])}`);
expect(exitCode).toBe(0);
});

// An entry point that throws rejects the module promise and exits without
// entering the loop, as Node does; the timer never fires.
test.concurrent("but not when the entry point throws", async () => {
using dir = tempDir("timers-phase-order-throw", {
"index.js": `
setTimeout(() => console.log("timer fired"), 1);
${blockPastTheDeadline}
throw new Error("boom");
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "index.js"],
cwd: String(dir),
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toContain("boom");
expect({ stdout, exitCode }).toEqual({ stdout: "", exitCode: 1 });
});
});
18 changes: 18 additions & 0 deletions test/js/node/timers/timers-phase-order-worker-fixture.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading