diff --git a/src/runtime/hw_exports.rs b/src/runtime/hw_exports.rs index dd343b1a658c..555cf8e5e5c1 100644 --- a/src/runtime/hw_exports.rs +++ b/src/runtime/hw_exports.rs @@ -189,8 +189,8 @@ pub(crate) mod sql_hooks { unsafe fn timer_insert(heap: *mut c_void, timer: *mut EventLoopTimer) { // SAFETY: `heap` is `&runtime_state().timer` (live for the VM); `timer` // is a live intrusive heap node owned by the caller. Route through - // `All::insert` (NOT the raw `.timers` field) so the lock is taken and - // `(*timer).state` / `in_heap` bookkeeping is updated. + // `All::insert` (NOT the raw `.timers` field) so the fake-timers + // routing and the `(*timer).state` / `in_heap` bookkeeping happen. unsafe { (*heap.cast::()).insert(timer) }; } unsafe fn timer_remove(heap: *mut c_void, timer: *mut EventLoopTimer) { diff --git a/src/runtime/jsc_hooks.rs b/src/runtime/jsc_hooks.rs index 8c8cf9ae1c20..5adc48000d94 100644 --- a/src/runtime/jsc_hooks.rs +++ b/src/runtime/jsc_hooks.rs @@ -197,10 +197,11 @@ pub(crate) fn global_dns_data() -> &'static core::cell::OnceCell bun_threading::MutexGuard { - // SAFETY: `timer_all()` returns the boxed per-thread `RuntimeState.timer`, - // never null while a VM is installed (asserted above). `lock` is accessed - // via shared `&Mutex` only (interior mutability), so this forms no aliased - // `&mut` with the surrounding `fake_timers` writes. - unsafe { &(*timer_all()).lock }.lock_guard() -} - #[inline] fn from_el_timespec(t: &ElTimespec) -> Timespec { Timespec { sec: t.sec, nsec: t.nsec } } impl FakeTimers { - fn assert_locked(&self) { - if !Environment::CI_ASSERT { - return; - } - // SAFETY: self points to the `fake_timers` field of `timer::All` (always embedded there) - let owner: &timer::All = unsafe { - &*(bun_core::from_field_ptr!(timer::All, fake_timers, std::ptr::from_ref::(self))) - }; - debug_assert!(!owner.lock.try_lock()); - } - pub fn is_active(&self) -> bool { - self.assert_locked(); - // validity re-checked at fn exit - let r = self.active; - self.assert_locked(); - r + self.active } fn activate(&mut self, js_now: f64, global: &JSGlobalObject) { - self.assert_locked(); - self.active = true; CURRENT_TIME.set(global, &Timespec::EPOCH, Some(js_now)); - - self.assert_locked(); } fn deactivate( &mut self, global: &JSGlobalObject, ) -> Vec> { - self.assert_locked(); - let pinned = self.clear(); CURRENT_TIME.clear(global); self.active = false; - - self.assert_locked(); pinned } - /// Drain the fake-timer heap. Returns every `TimeoutObject` that was - /// linked so the caller can release the heap's `+1` ref and the `Strong` - /// JS pin via [`TimerObjectInternals::release_heap_pin`] *after* dropping - /// `&mut self` and `All.lock` — that path reaches `&mut All`, which would - /// alias `&mut self.fake_timers` here (same hazard `execute_next` notes). - /// /// Marking `state = CANCELLED` alone strands the `Box`: its /// refcount sticks at 2 (wrapper +1 from `init_with`, heap +1 from /// `reschedule`) and `internals.this_value` still GC-roots the wrapper, so /// neither side ever frees. #[must_use] fn clear(&mut self) -> Vec> { - self.assert_locked(); - let mut pinned = Vec::new(); while let Some(timer) = self.timers.delete_min() { - // SAFETY: `delete_min` returns a live `*mut EventLoopTimer` just - // unlinked; for `TimeoutObject` the tag invariant means it IS the - // `event_loop_timer` field of a live `Box` whose - // refcount is ≥ 1 until the caller's release pass. + // SAFETY: `delete_min` returned a live node; the `TimeoutObject` + // it belongs to stays live until the caller's release pass. unsafe { - (*timer).state = EventLoopTimerState::CANCELLED; (*timer).in_heap = InHeap::None; + (*timer).state = EventLoopTimerState::CANCELLED; if (*timer).tag == EventLoopTimerTag::TimeoutObject { let parent = TimeoutObject::from_timer_ptr(timer); pinned.push(core::ptr::NonNull::new_unchecked( @@ -203,34 +156,15 @@ impl FakeTimers { } } - self.assert_locked(); pinned } - // noalias re-entrancy: `execute_*` / `fire` do NOT take - // `&mut self`. `EventLoopTimer::fire` dispatches into JS; a `setInterval` - // callback's reschedule (`timer::All::update` → `insert_lock_held` → - // `(*timer_all()).fake_timers.timers.insert`) writes back into *this - // same* `FakeTimers::timers` heap through a fresh raw pointer. With a - // live `&mut self` LLVM's `noalias` lets it cache `self.timers.root` - // across the (inlined) `fire` body — `peek()` on the next loop iteration - // then misses the re-inserted interval, so `advanceTimersByTime` / - // `runOnlyPendingTimers` fire each interval at most once per call. Same - // bug class as `TimerObjectInternals::fire` (see dc37f2018b34). Access - // the heap via the raw `timer_all()` pointer instead so every iteration - // reloads from memory. fn execute_next(global: &JSGlobalObject) -> bool { - let timers = timer_all(); - - let next = { - let _g = timers_lock_guard(); - // SAFETY: `timers` is the boxed per-thread `RuntimeState.timer`; - // single-threaded JS heap so no concurrent `&mut` to `.fake_timers`. - let n = unsafe { (*timers).fake_timers.timers.delete_min() }; - match n { - Some(n) => n, - None => return false, - } + // SAFETY: `timer_all()` is the live per-thread `All`; the borrow ends + // at this statement, before `fire` re-enters `All::insert`. + let next = match unsafe { (*timer_all()).fake_timers.timers.delete_min() } { + Some(n) => n, + None => return false, }; Self::fire(global, next); @@ -255,26 +189,22 @@ impl FakeTimers { } fn execute_until(global: &JSGlobalObject, until: Timespec) { - let timers = timer_all(); - + let all = timer_all(); 'outer: loop { let next = 'blk: { - let _g = timers_lock_guard(); - - // SAFETY: `timers` is the boxed per-thread `RuntimeState.timer`; - // single-threaded JS heap. Re-derive each iteration so the - // re-entrant `insert` from setInterval rescheduling is observed. - let Some(peek) = (unsafe { (*timers).fake_timers.timers.peek() }) else { + // SAFETY: `all` is the live per-thread `All`; each borrow + // lasts one statement and none spans `fire`. + let Some(peek) = (unsafe { (*all).fake_timers.timers.peek() }) else { break 'outer; }; - // SAFETY: `peek` is the heap root; live while locked. + // SAFETY: `peek` is the heap root; live while linked. if from_el_timespec(unsafe { &(*peek).next }).greater(&until) { break 'outer; } // bun.assert always evaluates its arg; debug_assert! does NOT in release. // Hoist the side-effecting delete_min() out so the timer is removed in all builds. // SAFETY: as above. - let min = unsafe { (*timers).fake_timers.timers.delete_min() }.expect("unreachable"); + let min = unsafe { (*all).fake_timers.timers.delete_min() }.expect("unreachable"); debug_assert!(core::ptr::eq(min, peek)); break 'blk min; }; @@ -283,20 +213,11 @@ impl FakeTimers { } fn execute_only_pending_timers(global: &JSGlobalObject) { - let timers = timer_all(); - - let until = { - let _g = timers_lock_guard(); - // SAFETY: `timers` is the boxed per-thread `RuntimeState.timer`. - let target = unsafe { (*timers).fake_timers.timers.find_max() }; - drop(_g); - match target { - Some(t) => { - // SAFETY: `t` was reachable in the heap under the lock. - from_el_timespec(unsafe { &(*t).next }) - } - None => return, - } + // SAFETY: `timer_all()` is the live per-thread `All`. + let until = match unsafe { (*timer_all()).fake_timers.timers.find_max() } { + // SAFETY: `t` is reachable in the heap and live while linked. + Some(t) => from_el_timespec(unsafe { &(*t).next }), + None => return, }; Self::execute_until(global, until); } @@ -311,17 +232,9 @@ impl FakeTimers { // === fn error_unless_fake_timers(global: &JSGlobalObject) -> JsResult<()> { - let timers = timer_all(); - // SAFETY: per-thread `timer::All`. - let this = unsafe { &(*timers).fake_timers }; - - { - let _g = timers_lock_guard(); - let active = this.is_active(); - drop(_g); - if active { - return Ok(()); - } + // SAFETY: per-thread `timer::All`, live for the VM lifetime. + if unsafe { (*timer_all()).fake_timers.is_active() } { + return Ok(()); } Err(global.throw(format_args!( "Fake timers are not active. Call useFakeTimers() first." @@ -353,10 +266,6 @@ fn set_fake_timer_marker(global: &JSGlobalObject, enabled: bool) { #[bun_jsc::host_fn] fn use_fake_timers(global: &JSGlobalObject, frame: &CallFrame) -> JsResult { - let timers = timer_all(); - // SAFETY: per-thread `timer::All`. - let this = unsafe { &mut (*timers).fake_timers }; - // SAFETY: FFI call into C++ JSMock let mut js_now = JSMock__getCurrentUnixTimeMs(); @@ -382,10 +291,8 @@ fn use_fake_timers(global: &JSGlobalObject, frame: &CallFrame) -> JsResult JsResult JsResult { - let timers = timer_all(); - - let pinned = { - // SAFETY: per-thread `timer::All`. - let this = unsafe { &mut (*timers).fake_timers }; - let _g = timers_lock_guard(); - this.deactivate(global) - }; + // SAFETY: per-thread `timer::All`; the borrow ends before `release_heap_pin`. + let pinned = unsafe { (*timer_all()).fake_timers.deactivate(global) }; let vm = global.bun_vm_ptr(); for p in pinned { TimerObjectInternals::release_heap_pin(p, vm); @@ -479,30 +380,20 @@ fn run_all_timers(global: &JSGlobalObject, frame: &CallFrame) -> JsResult JsResult { - let timers = timer_all(); - // SAFETY: per-thread `timer::All`. - let this = unsafe { &(*timers).fake_timers }; error_unless_fake_timers(global)?; - let count = { - let _g = timers_lock_guard(); - this.timers.count() - }; + // SAFETY: per-thread `timer::All`, live for the VM lifetime. + let count = unsafe { (*timer_all()).fake_timers.timers.count() }; Ok(JSValue::js_number(count as f64)) } #[bun_jsc::host_fn] fn clear_all_timers(global: &JSGlobalObject, frame: &CallFrame) -> JsResult { - let timers = timer_all(); error_unless_fake_timers(global)?; - let pinned = { - // SAFETY: per-thread `timer::All`. - let this = unsafe { &mut (*timers).fake_timers }; - let _g = timers_lock_guard(); - this.clear() - }; + // SAFETY: per-thread `timer::All`; the borrow ends before `release_heap_pin`. + let pinned = unsafe { (*timer_all()).fake_timers.clear() }; let vm = global.bun_vm_ptr(); for p in pinned { TimerObjectInternals::release_heap_pin(p, vm); @@ -513,14 +404,8 @@ fn clear_all_timers(global: &JSGlobalObject, frame: &CallFrame) -> JsResult JsResult { - let timers = timer_all(); - // SAFETY: per-thread `timer::All`. - let this = unsafe { &(*timers).fake_timers }; - - let is_active = { - let _g = timers_lock_guard(); - this.is_active() - }; + // SAFETY: per-thread `timer::All`, live for the VM lifetime. + let is_active = unsafe { (*timer_all()).fake_timers.is_active() }; Ok(JSValue::from(is_active)) } diff --git a/src/runtime/timer/Timer.rs b/src/runtime/timer/Timer.rs index 26c235420cc1..27a99009c83c 100644 --- a/src/runtime/timer/Timer.rs +++ b/src/runtime/timer/Timer.rs @@ -482,9 +482,9 @@ impl DateHeaderTimer { unsafe { (*(*vm).uws_loop()).update_date() }; let elt: *mut EventLoopTimer = &raw mut self.event_loop_timer; - // SAFETY: single JS thread; `All::update` only touches `lock`/`timers`/ - // `fake_timers`/`epoch`, disjoint from `date_header_timer` which `self` - // aliases (raw-ptr-per-field re-entry pattern, see jsc_hooks.rs). + // SAFETY: single JS thread; nothing `All::update` touches overlaps + // `date_header_timer`, which `self` aliases (raw-ptr-per-field + // re-entry pattern, see jsc_hooks.rs). unsafe { (*Self::timer_all()).update(elt, &now.add_ms(1000)) }; } else { // The date was updated recently, just reschedule for the next second diff --git a/src/runtime/timer/WTFTimer.rs b/src/runtime/timer/WTFTimer.rs index c0ee848c43c3..dc97f74e60cf 100644 --- a/src/runtime/timer/WTFTimer.rs +++ b/src/runtime/timer/WTFTimer.rs @@ -11,7 +11,6 @@ use core::ptr::{self, NonNull}; use core::sync::atomic::{AtomicPtr, Ordering}; use bun_core::{Timespec, TimespecMockMode}; -use bun_threading::Mutex; use crate::jsc::virtual_machine::{IS_BUNDLER_THREAD_FOR_BYTECODE_CACHE, VirtualMachine}; use crate::webcore::script_execution_context::Identifier as ScriptExecutionContextIdentifier; @@ -53,7 +52,6 @@ pub struct WTFTimer { // `*mut WTFTimer`). imminent: bun_ptr::BackRef>, repeat: bool, - lock: Mutex, script_execution_context_id: ScriptExecutionContextIdentifier, } @@ -82,17 +80,14 @@ impl WTFTimer { pub unsafe fn run(this: *mut Self, vm: *mut VirtualMachine) { // SAFETY: per fn contract — `this` is live; `ThisPtr` vends only fresh // short-lived `&Self` per Deref so no `&WTFTimer` spans the - // `All::remove` raw write to `event_loop_timer`. + // `All::wtf_disarm` raw write to `event_loop_timer`. let t = unsafe { bun_ptr::ThisPtr::new(this) }; - if t.event_loop_timer.state == EventLoopTimerState::ACTIVE { - // SAFETY: `vm` is the live VM that owns this timer's heap; - // `event_loop_timer` is an embedded field of a live allocation. - unsafe { - let state = crate::jsc_hooks::runtime_state_of(vm); - (*state) - .timer - .remove(ptr::addr_of_mut!((*this).event_loop_timer)); - } + // SAFETY: `vm` is the live VM that owns this timer's heap. + unsafe { + let state = crate::jsc_hooks::runtime_state_of(vm); + (*state) + .timer + .wtf_disarm(ptr::addr_of_mut!((*this).event_loop_timer)); } t.run_without_removing(); } @@ -115,7 +110,6 @@ impl WTFTimer { #[bun_uws::uws_callback(export = "WTFTimer__secondsUntilTimer", no_catch)] pub fn seconds_until_timer(&self) -> f64 { - let _g = self.lock.lock_guard(); if self.event_loop_timer.state == EventLoopTimerState::ACTIVE { let next = &self.event_loop_timer.next; // bun_event_loop carries a local `Timespec` stub; re-pack @@ -177,16 +171,12 @@ impl WTFTimer { interval.nsec -= NS_PER_S; } - // SAFETY: `t.vm` is the VM that owns this timer's heap (captured at - // `WTFTimer__create`); `event_loop_timer` is an embedded field of a - // live allocation. May be called off the JS thread — `All::update` - // takes its own lock. The `repeat` write is the only field write here; - // no `&Self` from `t` is live across it. + // SAFETY: `t.vm` owns this timer's heap; `wtf_arm` is safe from any thread. unsafe { let state = crate::jsc_hooks::runtime_state_of(t.vm.as_ptr()); (*state) .timer - .update(ptr::addr_of_mut!((*this).event_loop_timer), &interval); + .wtf_arm(ptr::addr_of_mut!((*this).event_loop_timer), &interval); (*this).repeat = repeat; } } @@ -195,11 +185,8 @@ impl WTFTimer { /// `this` must point at a live heap-allocated `WTFTimer`. pub unsafe fn cancel(this: *mut Self) { // SAFETY: per fn contract — `this` outlives this scope. `ThisPtr` vends - // only fresh short-lived `&Self` per Deref; `lock_guard` stores a - // `BackRef` (no `&Self` held in `_g`), so the `addr_of_mut!` - // below stays legal under Stacked Borrows. + // only fresh short-lived `&Self` per Deref. let t = unsafe { bun_ptr::ThisPtr::new(this) }; - let _g = t.lock.lock_guard(); if t.script_execution_context_id.valid() { // Only clear imminent if this timer was the one that set it. @@ -214,17 +201,13 @@ impl WTFTimer { Ordering::SeqCst, ); - if t.event_loop_timer.state == EventLoopTimerState::ACTIVE { - // SAFETY: `t.vm` is the VM that owns this timer's heap; may be - // called off the JS thread — `All::remove` locks. - // `addr_of_mut!` through the original `*mut` preserves write - // provenance for the heap-node mutation inside `remove`. - unsafe { - let state = crate::jsc_hooks::runtime_state_of(t.vm.as_ptr()); - (*state) - .timer - .remove(ptr::addr_of_mut!((*this).event_loop_timer)); - } + // SAFETY: `t.vm` owns this timer's heap; `wtf_disarm` is safe from + // any thread and is a no-op for a node that is no longer linked. + unsafe { + let state = crate::jsc_hooks::runtime_state_of(t.vm.as_ptr()); + (*state) + .timer + .wtf_disarm(ptr::addr_of_mut!((*this).event_loop_timer)); } } } @@ -233,12 +216,8 @@ impl WTFTimer { /// /// # Safety /// `this` is the container of an `EventLoopTimer` just popped from - /// `All.timers`; `_vm` is the live per-thread VM. + /// `All.wtf_timers`; `_vm` is the live per-thread VM. pub unsafe fn fire(this: *mut Self, _now: &ElTimespec, _vm: *mut VirtualMachine) { - // SAFETY: per fn contract — `this` is live. Single raw write to - // `event_loop_timer.state` precedes the `ThisPtr` borrow; subsequent - // field reads via `t` create fresh short-lived `&Self`. - unsafe { (*this).event_loop_timer.state = EventLoopTimerState::FIRED }; // SAFETY: per fn contract — `this` is live; `ThisPtr` vends only fresh // short-lived `&Self` per Deref. let t = unsafe { bun_ptr::ThisPtr::new(this) }; @@ -302,7 +281,6 @@ pub(crate) unsafe extern "C" fn WTFTimer__create(run_loop_timer: *mut RunLoopTim script_execution_context_id: ScriptExecutionContextIdentifier( vm_ref.initial_script_execution_context_identifier as u32, ), - lock: Mutex::default(), }) }; diff --git a/src/runtime/timer/mod.rs b/src/runtime/timer/mod.rs index b6926a22e19a..8a05dfdba08f 100644 --- a/src/runtime/timer/mod.rs +++ b/src/runtime/timer/mod.rs @@ -7,7 +7,7 @@ use bun_core::{Timespec, TimespecMockMode}; use bun_libuv_sys::UvHandle as _; #[cfg(windows)] use bun_sys::windows::libuv as uv; -use bun_threading::Mutex; +use bun_threading::Guarded; // Low-tier timer node + tag (per §Dispatch hot-path list, the `match tag` // dispatch lives in this crate; `bun_event_loop` only stores `(tag, ptr)`). @@ -419,8 +419,8 @@ impl DateHeaderTimer { nsec: next.nsec, }; let elt: *mut EventLoopTimer = &raw mut self.event_loop_timer; - // SAFETY: single JS thread; `All::insert` only touches `lock`/`timers`/ - // `fake_timers`, disjoint from `date_header_timer` which `self` aliases. + // SAFETY: single JS thread; nothing `All::insert` touches + // overlaps `date_header_timer`, which `self` aliases. unsafe { (*Self::timer_all()).insert(elt) }; } } @@ -474,8 +474,8 @@ impl EventLoopDelayMonitor { nsec: next.nsec, }; let elt: *mut EventLoopTimer = &raw mut self.event_loop_timer; - // SAFETY: single JS thread; `All::insert` only touches `lock`/`timers`/ - // `fake_timers`, disjoint from `event_loop_delay` which `self` aliases. + // SAFETY: single JS thread; nothing `All::insert` touches overlaps + // `event_loop_delay`, which `self` aliases. unsafe { (*Self::timer_all()).insert(elt) }; } @@ -560,7 +560,7 @@ pub use self::timeout_object::TimeoutObject; /// /// Returns a raw `NonNull` so the caller decides read vs. write: /// [`EventLoopTimer::less`] reads `.epoch()` on the heap-compare hot path; -/// [`All::update`] writes `.set_epoch()` under the timer lock. The two +/// [`All::update`] writes `.set_epoch()` on the JS thread. The two /// `internals.flags` arms store `Cell`; `Cell` is /// `#[repr(transparent)]` so the `addr_of!` → `.cast()` is layout-sound. /// @@ -606,7 +606,6 @@ pub use wtf_timer::WTFTimer; pub struct All { pub last_id: i32, - pub lock: Mutex, pub thread_id: std::thread::ThreadId, pub timers: TimerHeap, pub active_timer_count: i32, @@ -626,13 +625,13 @@ pub struct All { pub fake_timers: FakeTimers, pub maps: Maps, pub date_header_timer: DateHeaderTimer, + pub wtf_timers: Guarded, } impl All { pub fn init() -> Self { Self { last_id: 1, - lock: Mutex::default(), thread_id: std::thread::current().id(), timers: TimerHeap::default(), active_timer_count: 0, @@ -648,25 +647,25 @@ impl All { fake_timers: FakeTimers::default(), maps: Maps::default(), date_header_timer: DateHeaderTimer::default(), + wtf_timers: Guarded::init(TimerHeap::default()), } } - pub fn insert(&mut self, timer: *mut EventLoopTimer) { - self.lock.lock(); - // Note: bun_threading::Mutex is lock()/unlock(), not RAII. - let r = self.insert_lock_held(timer); - self.lock.unlock(); - r + #[inline] + fn assert_js_thread(&self) { + debug_assert!( + self.thread_id == std::thread::current().id(), + "timer::All: non-WTF timers may only be touched on the owning JS thread", + ); } - fn insert_lock_held(&mut self, timer: *mut EventLoopTimer) { + #[allow(clippy::not_unsafe_ptr_arg_deref)] + pub fn insert(&mut self, timer: *mut EventLoopTimer) { + self.assert_js_thread(); // SAFETY: caller guarantees `timer` is a valid live EventLoopTimer. - // Note (§Forbidden aliased-&mut): `TimerHeap::insert` forms a - // fresh `&mut EventLoopTimer` via `(*a).heap()` for the same - // allocation, so we must NOT hold a `&mut *timer` across that call. - // Read `tag` and write `state`/`in_heap` via raw deref instead. - let allow_fake = unsafe { (*timer).tag }.allow_fake_timers(); - if self.fake_timers.is_active() && allow_fake { + let tag = unsafe { (*timer).tag }; + debug_assert!(tag != EventLoopTimerTag::WTFTimer, "use wtf_arm"); + if self.fake_timers.is_active() && tag.allow_fake_timers() { // SAFETY: see fn contract unsafe { self.fake_timers.timers.insert(timer); @@ -686,25 +685,9 @@ impl All { } /// Lazily `uv_timer_init` the - /// per-`All` libuv timer, then (re)start it for the soonest heap deadline. - /// On Windows there is no epoll/kqueue fallback; this `uv_timer_t` is the - /// ONLY thing that wakes `uv_run` for JS timers. - /// - /// Note (jsc/runtime crate cycle): `All` is a field - /// of `RuntimeState` (not `VirtualMachine`) and `RuntimeState` carries no - /// back-pointer to the owning VM, so the lazy-init block falls back to the - /// calling thread's - /// TLS VM/loop. That equivalence holds **only** on the owning JS thread; - /// `All.lock` exists precisely because `insert`/`update` may be entered - /// cross-thread (WTFTimer), where TLS would resolve to the wrong loop or - /// panic. The `debug_assert!` below makes that precondition loud. Once - /// initialized, the re-arm path reads the loop back from the handle itself - /// (`uv_handle_get_loop`), so the hot path is TLS-free and always targets - /// the loop the timer was actually registered on. - /// - /// TODO: thread `vm: *mut VirtualMachine` through - /// `insert`/`insert_lock_held`/`update` once - /// the `RuntimeHooks::timer_insert` slot widens — see jsc_hooks.rs. + /// per-`All` libuv timer, then (re)start it for the soonest deadline + /// across both heaps. On Windows there is no epoll/kqueue fallback; this + /// `uv_timer_t` is the ONLY thing that wakes `uv_run` for JS timers. #[cfg(windows)] fn ensure_uv_timer(&mut self) { // `vm` here means the OWNING VM (the one this timer is embedded in), @@ -722,41 +705,53 @@ impl All { self.uv_timer.unref(); } - if let Some(timer) = self.timers.peek() { - // SAFETY: `uv_timer.data` is non-null past the lazy-init block, so - // `uv_timer_init` has run and the handle's `loop` field points at - // the owning VM's live `uv_loop_t` (== `vm.uvLoop()` per spec). - unsafe { uv::uv_update_time(self.uv_timer.get_loop()) }; - let now = Timespec::now(TimespecMockMode::ForceRealTime); + let reg_next = self.timers.peek().map(|timer| { // SAFETY: `peek` returns a live heap node. let next = unsafe { &(*timer).next }; - let next_ts = Timespec { + Timespec { sec: next.sec, nsec: next.nsec, - }; - let wait = if next_ts.greater(&now) { - next_ts.duration(&now) - } else { - Timespec { sec: 0, nsec: 0 } - }; + } + }); + let wtf_next = self.wtf_timers.lock().peek().map(|timer| { + // SAFETY: `peek` returns a live heap node. + let next = unsafe { &(*timer).next }; + Timespec { + sec: next.sec, + nsec: next.nsec, + } + }); + let Some(next_ts) = Self::soonest(reg_next, wtf_next) else { + return; + }; - // minimum 1ms - // https://github.com/nodejs/node/blob/f552c86fecd6c2ba9e832ea129b731dd63abdbe2/src/env.cc#L1512 - let wait_ms = core::cmp::max(1, wait.ms_unsigned()); + // SAFETY: `uv_timer.data` is non-null past the lazy-init block, so + // `uv_timer_init` has run and the handle's `loop` field points at + // the owning VM's live `uv_loop_t` (== `vm.uvLoop()` per spec). + unsafe { uv::uv_update_time(self.uv_timer.get_loop()) }; + let now = Timespec::now(TimespecMockMode::ForceRealTime); + let wait = if next_ts.greater(&now) { + next_ts.duration(&now) + } else { + Timespec { sec: 0, nsec: 0 } + }; - // SAFETY: `uv_timer_init` ran above; the handle is live. - let due_in = unsafe { uv::uv_timer_get_due_in(&self.uv_timer) }; - // Restarting an overdue handle shifts the wakeup out by 1ms. Done - // on every insert, the already-due callback never runs. - if !(self.uv_timer.is_active() && due_in <= wait_ms) { - self.uv_timer.start(wait_ms, 0, Some(Self::on_uv_timer)); - } + // minimum 1ms + // https://github.com/nodejs/node/blob/f552c86fecd6c2ba9e832ea129b731dd63abdbe2/src/env.cc#L1512 + let wait_ms = core::cmp::max(1, wait.ms_unsigned()); - if self.active_timer_count > 0 { - self.uv_timer.ref_(); - } else { - self.uv_timer.unref(); - } + // SAFETY: `uv_timer_init` ran above; the handle is live. + let due_in = unsafe { uv::uv_timer_get_due_in(&self.uv_timer) }; + // Restarting an overdue handle shifts the wakeup out by 1ms. Done + // on every insert, the already-due callback never runs. + if !(self.uv_timer.is_active() && due_in <= wait_ms) { + self.uv_timer.start(wait_ms, 0, Some(Self::on_uv_timer)); + } + + if self.active_timer_count > 0 { + self.uv_timer.ref_(); + } else { + self.uv_timer.unref(); } } @@ -781,13 +776,9 @@ impl All { unsafe { (*all).ensure_uv_timer() }; } + #[allow(clippy::not_unsafe_ptr_arg_deref)] pub fn remove(&mut self, timer: *mut EventLoopTimer) { - self.lock.lock(); - self.remove_lock_held(timer); - self.lock.unlock(); - } - - fn remove_lock_held(&mut self, timer: *mut EventLoopTimer) { + self.assert_js_thread(); // SAFETY: caller guarantees `timer` is a valid live EventLoopTimer. // Note (§Forbidden aliased-&mut): `TimerHeap::remove` forms a // fresh `&mut EventLoopTimer` via `(*v).heap()` for the same @@ -815,18 +806,15 @@ impl All { /// # Safety /// `timer` must point to a live `EventLoopTimer` with whole-container /// provenance for its tag (see [`js_timer_flags_ptr`]). - // `timer` must stay `*mut`: the body forms only short-lived `&mut *timer` - // so re-entrant `remove_lock_held` does not alias an outstanding `&mut` - // (see Notes below); contract is documented in `# Safety`. #[allow(clippy::not_unsafe_ptr_arg_deref)] pub fn update(&mut self, timer: *mut EventLoopTimer, time: &Timespec) { - self.lock.lock(); + self.assert_js_thread(); // SAFETY: caller guarantees `timer` is a valid live EventLoopTimer. // Read `state` via raw deref so we don't hold a `&mut *timer` across - // `remove_lock_held` (which also `&mut`-derefs the same pointer); - // overlapping `&mut` is UB under Stacked Borrows. + // `remove` (which also `&mut`-derefs the same pointer); overlapping + // `&mut` is UB under Stacked Borrows. if unsafe { (*timer).state } == EventLoopTimerState::ACTIVE { - self.remove_lock_held(timer); + self.remove(timer); } // SAFETY: `timer` is still a valid live EventLoopTimer; safe to derive @@ -835,7 +823,7 @@ impl All { // while `next` is `ElTimespec` — distinct types, so safe code cannot // construct the alias. Re-add a // `debug_assert!(!core::ptr::eq(time as *const _ as *const u8, &raw const (*timer).next as *const u8))` - // when the Timespec types unify (see the ElTimespec alias TODO at the + // when the Timespec types unify (see the ElTimespec alias note at the // top of this file). let timer_ref = unsafe { &mut *timer }; timer_ref.next.sec = time.sec; @@ -848,13 +836,93 @@ impl All { // is above so the raw `(*timer).tag` read inside is SB-clean. if let Some(flags) = unsafe { js_timer_flags_ptr(timer) } { self.epoch = self.epoch.wrapping_add(1) & ((1u32 << 25) - 1); - // SAFETY: exclusive under `self.lock`; `flags` points into the - // live container recovered above. + // SAFETY: `flags` points into the live container recovered above. unsafe { (*flags.as_ptr()).set_epoch(self.epoch) }; } - self.insert_lock_held(timer); - self.lock.unlock(); + self.insert(timer); + } + + #[allow(clippy::not_unsafe_ptr_arg_deref)] + pub(crate) fn wtf_arm(&mut self, timer: *mut EventLoopTimer, time: &Timespec) { + // SAFETY: caller guarantees `timer` is a valid live EventLoopTimer. + debug_assert!(unsafe { (*timer).tag } == EventLoopTimerTag::WTFTimer); + { + let mut wtf = self.wtf_timers.lock(); + // SAFETY: `timer` is live; its state and heap links only change under this guard. + unsafe { + if (*timer).state == EventLoopTimerState::ACTIVE { + wtf.remove(timer); + } + (*timer).next.sec = time.sec; + (*timer).next.nsec = time.nsec; + wtf.insert(timer); + (*timer).state = EventLoopTimerState::ACTIVE; + } + } + #[cfg(windows)] + if self.thread_id == std::thread::current().id() { + self.ensure_uv_timer(); + } + } + + #[allow(clippy::not_unsafe_ptr_arg_deref)] + pub(crate) fn wtf_disarm(&mut self, timer: *mut EventLoopTimer) { + // SAFETY: caller guarantees `timer` is a valid live EventLoopTimer. + debug_assert!(unsafe { (*timer).tag } == EventLoopTimerTag::WTFTimer); + let mut wtf = self.wtf_timers.lock(); + // SAFETY: `timer` is live; its state and heap links only change under this guard. + unsafe { + if (*timer).state == EventLoopTimerState::ACTIVE { + wtf.remove(timer); + (*timer).state = EventLoopTimerState::CANCELLED; + } + } + } + + unsafe fn drain_due_wtf_timers( + this: *mut Self, + maybe_now: &mut Option, + vm: *mut (), + ) -> Option { + loop { + let min = { + // SAFETY: `this` is live; the guard drops before `fire`. + let mut wtf = unsafe { &(*this).wtf_timers }.lock(); + let min = wtf.peek()?; + // SAFETY: `peek` returned a live heap node. + let min_next = unsafe { + Timespec { + sec: (*min).next.sec, + nsec: (*min).next.nsec, + } + }; + let now = *maybe_now + .get_or_insert_with(|| Timespec::now(TimespecMockMode::ForceRealTime)); + if min_next.greater(&now) { + return Some(min_next); + } + let min = wtf.delete_min().expect("peek succeeded"); + // SAFETY: `min` is the node `peek` returned above. + unsafe { (*min).state = EventLoopTimerState::FIRED }; + min + }; + let now = maybe_now.expect("set before the pop"); + let el_now = ElTimespec { + sec: now.sec, + nsec: now.nsec, + }; + // SAFETY: `min` is live; no guard or borrow of `All` is held here. + unsafe { EventLoopTimer::fire(min, &el_now, vm) }; + } + } + + #[inline] + fn soonest(a: Option, b: Option) -> Option { + match (a, b) { + (Some(a), Some(b)) => Some(if a.greater(&b) { b } else { a }), + (a, b) => a.or(b), + } } /// Called from `EventLoop::auto_tick` to compute the epoll/kqueue timeout. @@ -888,87 +956,49 @@ impl All { #[cfg(not(unix))] let _ = has_pending_immediate; - // Note (§Forbidden aliased-&mut): the WTFTimer arm below calls - // `(*min).fire(...)` → `WTFTimer__fire` → C++ may call back into - // `WTFTimer__update` → `(*runtime_state()).timer.update(...)`, minting - // a fresh `&mut All` to this same allocation while the outer - // `&mut self` is live → aliased-`&mut` UB. Mirror `drain_timers`: - // convert `self` to a raw pointer up-front and form *short-lived* - // `&mut *this` borrows only around `peek()`/`delete_min()`, dropping - // them before `fire()` so no `&mut All` is held across the re-entrant - // call. - // - // TODO: same caveat as `drain_timers` — the call-site auto-ref - // still creates a `&mut All` for the call frame; switch the signature - // to `this: *mut Self` (see the `get_timeout` call sites in jsc_hooks.rs). let this: *mut Self = self; let maybe_now: &mut Option = now_out; - loop { - // SAFETY: `this` derived from `&mut self`; short-lived exclusive - // borrow scoped to this `peek()` call only. - let Some(min) = (unsafe { &mut *this }).timers.peek() else { - break; - }; - // SAFETY: peek returns a live heap node. - // Note (§Forbidden aliased-&mut): `delete_min()` writes - // `(*min).heap` through a fresh `&mut EventLoopTimer`, so we must - // NOT hold a `&mut *min` across it. Read `next`/`tag` via raw - // deref and fire via raw deref (mirroring `drain_timers`). - let (min_next_sec, min_next_nsec, min_tag) = - unsafe { ((*min).next.sec, (*min).next.nsec, (*min).tag) }; - // Real clock: `self.timers` is the opt-out-of-fake-timers set, all - // armed in real-time units. Comparing against the mocked clock made - // internal pacing (GC, WTFTimer, test timeouts) spin on re-arm. - let now = - *maybe_now.get_or_insert_with(|| Timespec::now(TimespecMockMode::ForceRealTime)); - - // bun_event_loop carries its own Timespec stub; compare field-wise. - let min_next = Timespec { - sec: min_next_sec, - nsec: min_next_nsec, - }; - match now.order(&min_next) { - core::cmp::Ordering::Greater | core::cmp::Ordering::Equal => { - // Side-effect: potentially call the StopIfNecessary timer. - if min_tag == EventLoopTimerTag::WTFTimer { - // SAFETY: short-lived `&mut All` scoped to - // `delete_min()`; dropped before `fire()`. - let _ = unsafe { &mut *this }.timers.delete_min(); - let el_now = ElTimespec { - sec: now.sec, - nsec: now.nsec, - }; - // SAFETY: `min` was just popped and is live; no `&mut` - // to `All` or to `*min` is held across `fire()`, which - // may re-enter `(*runtime_state()).timer`. - unsafe { EventLoopTimer::fire(min, &el_now, vm) }; - continue; - } - *spec = Timespec { sec: 0, nsec: 0 }; - return true; - } - core::cmp::Ordering::Less => { - *spec = min_next.duration(&now); - if let Some(us) = quic_next_tick_us { - if us >= 0 { - Self::clamp_to_quic(spec, us); - } - } + + // SAFETY: `this` is the live per-thread `All`; `vm` per fn contract. + let wtf_next = unsafe { Self::drain_due_wtf_timers(this, maybe_now, vm) }; + + // SAFETY: `this` is live, and only this thread touches the regular heap. + let reg_next = (unsafe { &*this }).timers.peek().map(|min| { + // SAFETY: `peek` returns a live heap node. + let next = unsafe { &(*min).next }; + Timespec { + sec: next.sec, + nsec: next.nsec, + } + }); + + let Some(next) = Self::soonest(wtf_next, reg_next) else { + if let Some(us) = quic_next_tick_us { + if us >= 0 { + *spec = Timespec { + sec: us / US_PER_S, + nsec: (us % US_PER_S) * NS_PER_US, + }; return true; } } - } + return false; + }; - if let Some(us) = quic_next_tick_us { - if us >= 0 { - *spec = Timespec { - sec: us / US_PER_S, - nsec: (us % US_PER_S) * NS_PER_US, - }; - return true; + // Real clock: both heaps hold opt-out-of-fake-timers nodes armed in + // real-time units; the mocked clock made internal pacing spin on re-arm. + let now = *maybe_now.get_or_insert_with(|| Timespec::now(TimespecMockMode::ForceRealTime)); + if next.greater(&now) { + *spec = next.duration(&now); + if let Some(us) = quic_next_tick_us { + if us >= 0 { + Self::clamp_to_quic(spec, us); + } } + } else { + *spec = Timespec { sec: 0, nsec: 0 }; } - false + true } fn clamp_to_quic(spec: &mut Timespec, us: i64) { @@ -981,33 +1011,28 @@ impl All { } } - /// Pop the next due timer (under lock). `now` is filled lazily on first - /// call so we don't pay for `clock_gettime` when the heap is empty. + /// Pop the next due timer. `now` is filled lazily on first call so we + /// don't pay for `clock_gettime` when the heap is empty. fn next(&mut self, has_set_now: &mut bool, now: &mut Timespec) -> Option<*mut EventLoopTimer> { - self.lock.lock(); - let out = (|| { - let timer = self.timers.peek()?; - if !*has_set_now { - // Real clock: this heap is the opt-out-of-fake-timers set. - *now = Timespec::now(TimespecMockMode::ForceRealTime); - *has_set_now = true; - } - // SAFETY: peek returns a live heap node - let next = unsafe { &(*timer).next }; - if (Timespec { - sec: next.sec, - nsec: next.nsec, - }) - .greater(now) - { - return None; - } - let deleted = self.timers.delete_min().expect("peek succeeded"); - debug_assert!(core::ptr::eq(deleted, timer)); - Some(timer) - })(); - self.lock.unlock(); - out + let timer = self.timers.peek()?; + if !*has_set_now { + // Real clock: this heap is the opt-out-of-fake-timers set. + *now = Timespec::now(TimespecMockMode::ForceRealTime); + *has_set_now = true; + } + // SAFETY: peek returns a live heap node + let next = unsafe { &(*timer).next }; + if (Timespec { + sec: next.sec, + nsec: next.nsec, + }) + .greater(now) + { + return None; + } + let deleted = self.timers.delete_min().expect("peek succeeded"); + debug_assert!(core::ptr::eq(deleted, timer)); + Some(timer) } /// # Safety @@ -1033,6 +1058,11 @@ impl All { // itself; switch it to `All::drain_timers(core::ptr::addr_of_mut!( // (*state).timer), vm)` and change this signature to `this: *mut Self`. let this: *mut Self = self; + + let mut wtf_now: Option = None; + // SAFETY: `this` is the live per-thread `All`; `vm` per fn contract. + let _ = unsafe { Self::drain_due_wtf_timers(this, &mut wtf_now, vm) }; + let mut now = Timespec { sec: 0, nsec: 0 }; let mut has_set_now = false; loop { @@ -1142,11 +1172,6 @@ impl All { /// still linked in `timers` / `fake_timers.timers` so the in-heap `+1` ref /// and the JS pin (`this_value` Strong) are released before the GC sweep. /// - /// Snapshots the heap under `lock` (cross-thread `WTFTimer__update` from - /// the GC scheduler thread can race the DFS otherwise), then cancels each - /// node *outside* the lock — `cancel()` re-enters [`All::remove`] which - /// re-acquires `lock` (non-recursive `bun_threading::Mutex`). - /// /// # Safety /// JS thread only, with the TLS `RuntimeState` still installed and `vm` /// the live per-thread VM. Must run BEFORE JSC teardown @@ -1162,11 +1187,7 @@ impl All { let mut signal_timeouts: Vec<*mut AbortSignalTimeout> = Vec::new(); let mut stack: Vec<*mut EventLoopTimer> = Vec::new(); - // SAFETY: `this` is the live per-thread `All`; `lock` guards both heap - // roots against concurrent `WTFTimer` insert/remove from off-thread - // (GC scheduler thread). Lock/unlock is manual (non-RAII Mutex). - unsafe { (*this).lock.lock() }; - // SAFETY: `this` live; both roots are heap roots or null. + // SAFETY: `this` is the live per-thread `All` (JS thread only). let roots = unsafe { [(*this).timers.0.root, (*this).fake_timers.timers.0.root] }; for root in roots { if !root.is_null() { @@ -1208,9 +1229,6 @@ impl All { _ => {} } } - // SAFETY: paired with the `lock()` above. Must release before the - // cancel loop — `cancel()` re-enters `All::remove` which re-locks. - unsafe { (*this).lock.unlock() }; for internals in to_cancel { // SAFETY: each pointer was collected from the live heap; the diff --git a/src/runtime/timer/timer_object_internals.rs b/src/runtime/timer/timer_object_internals.rs index 1269f8028d66..9d3f0250d798 100644 --- a/src/runtime/timer/timer_object_internals.rs +++ b/src/runtime/timer/timer_object_internals.rs @@ -147,8 +147,9 @@ impl TimerObjectInternals { /// `cancel()` skips its own `remove`/`deref` because `state` is already /// `CANCELLED`, which is why the explicit `deref` follows. /// - /// `vm` is the live per-thread VM; `All.lock` must NOT be held (the - /// `set_enable_keeping_event_loop_alive` write reaches `&mut All`). + /// `vm` is the live per-thread VM; no borrow of `All` may be live across + /// this call (`cancel()` reaches `All::remove`, which forms its own + /// `&mut All`). pub(crate) fn release_heap_pin(this: core::ptr::NonNull, vm: *mut VirtualMachine) { // SAFETY: caller guarantees the parent box is live (refcount ≥ 1). let internals = unsafe { this.as_ref() }; @@ -290,8 +291,7 @@ impl TimerObjectInternals { flags: { let mut f = Flags::default(); f.set_kind(kind); - // SAFETY: `state` is the boxed per-thread `RuntimeState`; - // single-threaded JS heap so no concurrent `&mut` to `.timer`. + // SAFETY: `state` is the boxed per-thread `RuntimeState`. f.set_epoch(unsafe { (*state).timer.epoch }); Cell::new(f) }, diff --git a/test/js/web/timers/timer-heap-atomics-fixture.ts b/test/js/web/timers/timer-heap-atomics-fixture.ts new file mode 100644 index 000000000000..e5cf9baada76 --- /dev/null +++ b/test/js/web/timers/timer-heap-atomics-fixture.ts @@ -0,0 +1,68 @@ +declare var self: Worker; + +const DURATION_MS = Number(process.argv[2] ?? 3000); +const WORKERS = Number(process.argv[3] ?? 3); + +function noop() {} + +if (!Bun.isMainThread) { + self.onmessage = (e: MessageEvent) => { + const { sab, durationMs } = e.data as { sab: SharedArrayBuffer; durationMs: number }; + const i32 = new Int32Array(sab); + const deadline = Date.now() + durationMs; + + function pump() { + for (let i = 0; i < 48; i++) { + const w = Atomics.waitAsync(i32, 0, 0, 1 + (i % 7)); + if (w.async) w.value.then(noop); + } + for (let i = 0; i < 8; i++) setTimeout(noop, i % 4); + Atomics.notify(i32, 0, 8); + if (Date.now() < deadline) { + setTimeout(pump, 0); + } else { + postMessage("done"); + } + } + pump(); + }; +} else { + const sab = new SharedArrayBuffer(64); + const i32 = new Int32Array(sab); + const workers: Worker[] = []; + let done = 0; + + for (let w = 0; w < WORKERS; w++) { + const worker = new Worker(import.meta.url); + worker.onmessage = () => { + if (++done === WORKERS) { + for (const other of workers) other.terminate(); + console.log("OK"); + process.exit(0); + } + }; + worker.onerror = (e: ErrorEvent) => { + console.error("worker error:", e.message); + process.exit(3); + }; + worker.postMessage({ sab, durationMs: DURATION_MS }); + workers.push(worker); + } + + const deadline = Date.now() + DURATION_MS + 1000; + function hammer() { + for (let i = 0; i < 24; i++) { + const w = Atomics.waitAsync(i32, 0, 0, 1 + (i % 5)); + if (w.async) w.value.then(noop); + } + const spinUntil = Date.now() + 2; + while (Date.now() < spinUntil) { + Atomics.notify(i32, 0, 1); + Atomics.notify(i32, 0, 1); + Atomics.notify(i32, 0, 2); + } + for (let i = 0; i < 4; i++) setTimeout(noop, i % 3); + if (Date.now() < deadline && done < WORKERS) setTimeout(hammer, 0); + } + hammer(); +} diff --git a/test/js/web/timers/timer-heap-gc-fixture.ts b/test/js/web/timers/timer-heap-gc-fixture.ts new file mode 100644 index 000000000000..e5d31aeba199 --- /dev/null +++ b/test/js/web/timers/timer-heap-gc-fixture.ts @@ -0,0 +1,17 @@ +const TICKS = 30; + +let garbage: unknown[] = []; +let ticks = 0; + +function tick() { + garbage = []; + for (let i = 0; i < 6000; i++) garbage.push({ i, s: "x" + (i & 255) }); + Bun.gc(true); + if (++ticks < TICKS) { + setTimeout(tick, 16); + } else { + console.log("ok " + ticks); + } +} + +tick(); diff --git a/test/js/web/timers/timer-heap-race.test.ts b/test/js/web/timers/timer-heap-race.test.ts new file mode 100644 index 000000000000..e87b1d89ae0c --- /dev/null +++ b/test/js/web/timers/timer-heap-race.test.ts @@ -0,0 +1,43 @@ +import { expect, it } from "bun:test"; +import { bunEnv, bunExe, isDebug } from "harness"; +import path from "node:path"; + +async function runFixture(fixture: string) { + await using proc = Bun.spawn({ + cmd: [bunExe(), path.join(import.meta.dir, fixture)], + env: { + ...bunEnv, + // These make the debug build an order of magnitude slower; the fixtures need real wall time. + BUN_JSC_validateExceptionChecks: undefined, + BUN_JSC_dumpSimulatedThrows: undefined, + }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + return { stdout, stderr, signal: proc.signalCode, exitCode }; +} + +it("timer heap survives cross-thread Atomics.waitAsync timeout cancellation", async () => { + expect(await runFixture("timer-heap-atomics-fixture.ts")).toEqual({ + stdout: "OK\n", + stderr: expect.any(String), + signal: null, + exitCode: 0, + }); +}, 20_000); + +it.skipIf(!isDebug)( + "timer heap stays consistent while GC re-arms the RunLoop timer", + async () => { + expect(await runFixture("timer-heap-gc-fixture.ts")).toEqual({ + stdout: "ok 30\n", + stderr: expect.any(String), + signal: null, + exitCode: 0, + }); + }, + 20_000, +);