Skip to content
Open
Show file tree
Hide file tree
Changes from 8 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
40 changes: 40 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,43 @@
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, then the timers phase.
///
/// Module evaluation runs on the microtask queue, so draining microtasks
/// reaches the first `await` (or the end of a synchronous entry point).
/// Leaving that to the `tick()` in the wait loop would dispatch the *task*
/// queue in the same tick, running an I/O completion or port message the
/// body queued ahead of a timer that expired while it ran. `uv_run` opens
/// with `uv__run_timers`, so Node dispatches that timer before either.
fn run_entry_point_body(&mut self, evaluation: *mut JSInternalPromise) {
// The body has to evaluate with the loop entered, the way it did inside
// `tick()`: a nested callback's `exit()` drains microtasks at count 1, and
// draining them mid-body reorders a constructor against its own
// continuations. `exit()` drains what the body left behind. Reporting
// rejections is left to the wait loop's `tick()`, as on the pre-existing
// path, so this stays a pure microtask drain.
self.event_loop_mut().enter();
let drained = self.event_loop_mut().drain_microtasks();
self.event_loop_mut().exit();
if drained.is_err() {
return;
}
// 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();
Comment thread
claude[bot] marked this conversation as resolved.
}

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

View check run for this annotation

Claude / Claude Code Review

d85c4483 reverted the rejection-ordering guards; PR description and resolved threads are stale

d85c4483 dropped the `handle_rejected_promises()` call, the `unhandled_error_counter` snapshot/guard, and the second `drain_microtasks()` from `run_entry_point_body` (its commit message says the proactive call "broke test-event-capture-rejections" and "Timer-vs-rejection ordering is no longer claimed"), and deleted the three tests covering it — but the PR description still says "unhandled rejections are notified first", "it is snapshotted on entry rather than compared against zero", and lists th
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 +4605,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 +4631,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 +4655,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 @@ pub(crate) unsafe fn __bun_run_wtf_timer(
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::<()>()) };
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
}

// ════════════════════════════════════════════════════════════════════════════
// EventLoopTimer dispatch
// ════════════════════════════════════════════════════════════════════════════
Expand Down
124 changes: 123 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,125 @@ 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 poll and check phases", () => {
// 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 anything queued next to it.
const blockPastTheDeadline = `const start = Date.now(); while (Date.now() - start < 10) {}`;

test.concurrent.each([
[
"setImmediate",
`setTimeout(() => order.push("timeout"), 1);
setImmediate(() => order.push("immediate"));`,
["timeout", "immediate"],
],
[
"a MessagePort self-delivery",
`const { port1, port2 } = new MessageChannel();
port1.onmessage = () => { order.push("message"); port1.close(); port2.close(); };
port2.postMessage(1);
setTimeout(() => order.push("timeout"), 1);`,
["timeout", "message"],
],
[
"completed fs I/O",
`require("fs").readFile(__filename, () => order.push("readFile"));
setTimeout(() => order.push("timeout"), 1);`,
["timeout", "readFile"],
],
])("before %s", async (_name, scheduled, expected) => {
using dir = tempDir("timers-phase-order", {
"index.js": `
const order = [];
process.on("exit", () => console.log(JSON.stringify(order)));
${scheduled}
${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(expected));
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