diff --git a/src/event_loop/SpawnSyncEventLoop.rs b/src/event_loop/SpawnSyncEventLoop.rs index c8280d9f415b..5037f2d82133 100644 --- a/src/event_loop/SpawnSyncEventLoop.rs +++ b/src/event_loop/SpawnSyncEventLoop.rs @@ -283,6 +283,13 @@ impl Drop for SpawnSyncEventLoop { impl SpawnSyncEventLoop { /// Configure the event loop for a specific VM context pub fn prepare(&mut self, vm: *mut () /* SAFETY: erased *mut VirtualMachine */) { + #[cfg(unix)] + debug_assert!( + self.uws_loop().num_polls == 0 && self.uws_loop().active == 0, + "spawnSync isolated loop has num_polls={} active={} before a spawn", + self.uws_loop().num_polls, + self.uws_loop().active, + ); __bun_spawn_sync_event_loop_set_vm(self.event_loop, vm); self.did_timeout.set(false); self.vm = vm; diff --git a/src/io/ParentDeathWatchdog.rs b/src/io/ParentDeathWatchdog.rs index 43520bc43ac2..a172abe96f7f 100644 --- a/src/io/ParentDeathWatchdog.rs +++ b/src/io/ParentDeathWatchdog.rs @@ -297,13 +297,8 @@ pub fn install_on_event_loop(handle: EventLoopCtx) { Default::default(), Owner::new(poll_tag::PARENT_DEATH_WATCHDOG, instance_ptr.cast()), ); - // SAFETY: `poll` was just allocated by `FilePoll::init`; sole `&mut` - // borrow; `register` does not re-derive the loop. - match unsafe { &mut *poll }.register( - handle.loop_mut(), - crate::file_poll::Flags::Process, - true, - ) { + // SAFETY: `poll` was just allocated by `FilePoll::init`; sole `&mut` borrow. + match unsafe { &mut *poll }.register(handle, crate::file_poll::Flags::Process, true) { bun_sys::Result::Ok(()) => { // Do not keep the event loop alive on this poll's behalf — the // watchdog must never prevent Bun from exiting when there is no diff --git a/src/io/PipeReader.rs b/src/io/PipeReader.rs index 6373abeaf783..66f47bedf82a 100644 --- a/src/io/PipeReader.rs +++ b/src/io/PipeReader.rs @@ -110,6 +110,7 @@ impl BufferedReaderVTable { self.link().event_loop() } + #[cfg(windows)] fn loop_(&self) -> *mut Loop { self.link().loop_ptr() } @@ -360,7 +361,7 @@ impl PosixBufferedReader { // Unregister the FilePoll if it's registered if let PollOrFd::Poll(poll) = &mut self.handle { if poll.is_registered() { - let _ = poll.unregister(self.vtable.loop_().cast(), false); + let _ = poll.unregister(self.vtable.event_loop(), false); } } } @@ -521,7 +522,6 @@ impl PosixBufferedReader { // normalize self.handle to Poll before taking the single &mut borrow, // so no raw-pointer escape is needed. let ev = self.vtable.event_loop(); - let lp = self.vtable.loop_(); let owner_ptr = std::ptr::from_mut(self).cast::(); if let PollOrFd::Fd(fd) = self.handle { @@ -544,7 +544,7 @@ impl PosixBufferedReader { poll.enable_keeping_process_alive(ev); } - match poll.register_with_fd(lp.cast(), FilePollKind::Readable, poll.fd()) { + match poll.register_with_fd(ev, FilePollKind::Readable, poll.fd()) { sys::Result::Err(err) => Err(err), sys::Result::Ok(()) => Ok(()), } diff --git a/src/io/PipeWriter.rs b/src/io/PipeWriter.rs index 4b949b9ddcfd..228fc561ed65 100644 --- a/src/io/PipeWriter.rs +++ b/src/io/PipeWriter.rs @@ -469,9 +469,7 @@ impl PosixBufferedWriter { pub fn register_poll(&mut self) { let Some(poll) = self.get_poll() else { return }; - // Use the event loop from the parent, not the global one - let loop_ = self.parent_event_loop().loop_(); - match poll.register_with_fd(loop_, FilePollKind::Writable, poll.fd()) { + match poll.register_with_fd(self.parent_event_loop(), FilePollKind::Writable, poll.fd()) { sys::Result::Err(err) => { // Same report as a failed write (the streaming writer does the // same): parents expect every error to be followed by `on_close`. @@ -581,9 +579,7 @@ impl PosixBufferedWriter { p } }; - let loop_ = self.parent_event_loop().loop_(); - - match poll.register_with_fd(loop_, FilePollKind::Writable, fd) { + match poll.register_with_fd(self.parent_event_loop(), FilePollKind::Writable, fd) { sys::Result::Err(err) => { // A poll from an earlier start() still holds that start's fd. if existing_poll.is_none() { @@ -631,9 +627,6 @@ pub trait PosixStreamingWriterParent { /// # Safety /// `this` must point to a live `Self`. unsafe fn event_loop(this: *mut Self) -> EventLoopHandle; - /// # Safety - /// `this` must point to a live `Self`. - unsafe fn loop_(this: *mut Self) -> *mut bun_uws_sys::Loop; } pub struct PosixStreamingWriter { @@ -827,8 +820,8 @@ impl PosixStreamingWriter { fn register_poll(&mut self) { let Some(poll) = self.get_poll() else { return }; // SAFETY: parent BACKREF set via set_parent; outlives this writer. - let loop_ = unsafe { Parent::loop_(self.parent()) }.cast(); - match poll.register_with_fd(loop_, FilePollKind::Writable, poll.fd()) { + let event_loop = unsafe { Parent::event_loop(self.parent()) }; + match poll.register_with_fd(event_loop, FilePollKind::Writable, poll.fd()) { sys::Result::Err(err) => { // PORT_NOTES_PLAN R-2: `&mut self` carries LLVM `noalias`, but // `Parent::on_error` (e.g. `FileSink::on_error`) re-enters via @@ -1098,13 +1091,13 @@ impl PosixStreamingWriter { } // SAFETY: parent BACKREF set via set_parent; outlives this writer. - let loop_ = unsafe { Parent::event_loop(self.parent()) }; + let event_loop = unsafe { Parent::event_loop(self.parent()) }; let existing_poll = self.get_poll(); let poll = match existing_poll { Some(p) => p, None => { let p = FilePollRef::init( - loop_, + event_loop, fd, Owner::new(Parent::POLL_OWNER_TAG, std::ptr::from_mut(self).cast()), ); @@ -1113,7 +1106,7 @@ impl PosixStreamingWriter { } }; - match poll.register_with_fd(loop_.loop_(), FilePollKind::Writable, fd) { + match poll.register_with_fd(event_loop, FilePollKind::Writable, fd) { sys::Result::Err(err) => { // A poll from an earlier start() still holds that start's fd. if existing_poll.is_none() { @@ -2654,7 +2647,6 @@ pub mod __parent_macro { pub use ::bun_sys::Error as SysError; #[cfg(windows)] pub use ::bun_sys::windows::libuv::Loop as UvLoop; - pub use ::bun_uws_sys::Loop as UwsLoop; } /// Stamp `PosixStreamingWriterParent` + `WindowsWriterParent` + @@ -2676,7 +2668,6 @@ macro_rules! impl_streaming_writer_parent { on_ready = $on_ready:ident, on_close = $on_close:ident, event_loop = |$el_this:ident| $el:expr, - uws_loop = |$uws_this:ident| $uws:expr, uv_loop = |$uv_this:ident| $uv:expr, ref_ = |$ref_this:ident| $ref_:expr, deref = |$deref_this:ident| $deref:expr, @@ -2715,13 +2706,6 @@ macro_rules! impl_streaming_writer_parent { #[allow(unused_unsafe)] unsafe { $el } } - #[inline] - unsafe fn loop_(this: *mut Self) -> *mut $crate::pipe_writer::__parent_macro::UwsLoop { - // SAFETY: see on_write. Shared-only read. - let $uws_this = this; - #[allow(unused_unsafe)] - unsafe { $uws } - } } #[cfg(windows)] diff --git a/src/io/keep_alive.rs b/src/io/keep_alive.rs index b45fbb8a007b..f6c7dc000381 100644 --- a/src/io/keep_alive.rs +++ b/src/io/keep_alive.rs @@ -13,6 +13,8 @@ use crate::posix_event_loop::js_vm_ctx; #[derive(Default)] pub struct KeepAlive { status: Status, + /// `ref_` counted on `Bun.spawnSync`'s isolated loop, not the thread's loop. + spawn_sync_loop: bool, } #[derive(Copy, Clone, PartialEq, Eq, Default)] @@ -46,7 +48,7 @@ impl KeepAlive { } self.status = Status::Inactive; #[cfg(not(windows))] - event_loop_ctx.loop_unref(); + event_loop_ctx.loop_unref_for(self.spawn_sync_loop); #[cfg(windows)] event_loop_ctx.loop_sub_active(1); } @@ -56,6 +58,10 @@ impl KeepAlive { if self.status != Status::Active { return; } + debug_assert!( + !self.spawn_sync_loop, + "deferred unref of a spawnSync loop ref" + ); self.status = Status::Inactive; // vm.pending_unref_counter +|= 1; #[cfg(not(windows))] @@ -70,7 +76,7 @@ impl KeepAlive { return; } self.status = Status::Active; - event_loop_ctx.loop_ref(); + self.spawn_sync_loop = event_loop_ctx.loop_ref(); } /// Allow a poll to keep the process alive. diff --git a/src/io/lib.rs b/src/io/lib.rs index af271cd87741..278b23411502 100644 --- a/src/io/lib.rs +++ b/src/io/lib.rs @@ -331,33 +331,18 @@ bun_dispatch::link_interface! { } impl EventLoopCtx { - /// SAFETY: caller must not hold another live `&mut` to the same loop - /// across this borrow (resolver-style accessor; the loop is per-thread). - #[inline] - pub unsafe fn platform_event_loop(&self) -> &'static mut bun_uws_sys::Loop { - // Route through the single nonnull-asref accessor below; the `unsafe` - // on this fn's signature is the caller-side aliasing contract — the - // body itself needs no extra `unsafe`. - self.loop_mut() - } - // ── safe leaf wrappers (nonnull-asref) ────────────────────────────── // The platform loop / poll store are per-thread, set-once back-pointers - // (`BackRef`-shaped). [`platform_event_loop`] cannot be a safe fn because - // handing out `&mut Loop` from `&self` would let a caller alias two - // copies; these wrappers instead perform one counter adjustment and drop - // the borrow before returning, so no `&mut` escapes. Collapses N - // identical `ctx.platform_event_loop().op()` call sites into the single - // deref inside [`loop_mut`]. + // (`BackRef`-shaped). Handing out `&mut Loop` from `&self` would let a + // caller alias two copies; these wrappers instead perform one counter + // adjustment and drop the borrow before returning, so no `&mut` escapes. // - // `loop_mut` is the single nonnull-asref accessor: `pub(crate)`, + // `loop_mut` is the single nonnull-asref accessor: crate-private, // `&self → &mut` (so it must NOT be called twice with overlapping live // results). Every in-crate caller is a leaf op — counter bump, // `FilePoll::activate`/`deactivate`, `unregister` — that consumes the // borrow before returning and never re-enters `EventLoopCtx`, so no two - // `&mut Loop` ever coexist. Widened from impl-private to crate-private so - // `posix_event_loop`/`windows_event_loop` route their N identical - // `ctx.platform_event_loop()` derefs through this single accessor. + // `&mut Loop` ever coexist. #[inline] fn loop_mut(&self) -> &'static mut bun_uws_sys::Loop { // SAFETY: per-thread set-once pointer (the uws loop singleton); the @@ -387,14 +372,40 @@ impl EventLoopCtx { // SAFETY: per-thread scratch owned by the VM/Mini loop, which outlives every read. unsafe { (*self.pipe_read_scratch()).claim() } } + /// True while `Bun.spawnSync` has its isolated loop installed as the current loop. #[inline] - pub(crate) fn loop_ref(&self) { - self.loop_mut().ref_(); + pub(crate) fn is_spawn_sync_loop(&self) -> bool { + self.platform_event_loop_ptr() != bun_uws_sys::Loop::get() } + /// The loop a counter was taken on: the isolated `spawnSync` loop when its + /// owner recorded `spawn_sync_loop` from [`Self::is_spawn_sync_loop`], else + /// the thread's loop, even while `spawnSync` has swapped the current loop. #[inline] #[cfg(not(windows))] - pub(crate) fn loop_unref(&self) { - self.loop_mut().unref(); + fn loop_for(&self, spawn_sync_loop: bool) -> &'static mut bun_uws_sys::Loop { + if spawn_sync_loop { + debug_assert!( + self.is_spawn_sync_loop(), + "a spawnSync loop counter is released after spawnSync returned" + ); + return self.loop_mut(); + } + // SAFETY: the thread's loop; same contract as `loop_mut`. + unsafe { &mut *bun_uws_sys::Loop::get() } + } + /// Refs the current loop and returns the bit for [`Self::loop_unref_for`]. + #[inline(never)] // `KeepAlive::ref_` is inlined into several hundred callers. + #[must_use] + pub fn loop_ref(&self) -> bool { + let spawn_sync_loop = self.is_spawn_sync_loop(); + self.loop_mut().ref_(); + spawn_sync_loop + } + /// Releases a [`Self::loop_ref`] that returned `spawn_sync_loop`. + #[inline(never)] + #[cfg(not(windows))] + pub fn loop_unref_for(&self, spawn_sync_loop: bool) { + self.loop_for(spawn_sync_loop).unref(); } #[inline] #[cfg(windows)] @@ -402,10 +413,12 @@ impl EventLoopCtx { self.loop_mut().dec(); } #[inline] + #[cfg(windows)] pub(crate) fn loop_add_active(&self, n: u32) { self.loop_mut().add_active(n); } #[inline] + #[cfg(windows)] pub(crate) fn loop_sub_active(&self, n: u32) { self.loop_mut().sub_active(n); } @@ -1829,41 +1842,25 @@ impl FilePollRef { pub(crate) fn deinit_force_unregister(self) { self.inner().deinit_force_unregister(); } - /// Single nonnull-asref accessor for the process-global uWS loop pointer. - /// - /// Type invariant (encapsulated `unsafe`): every caller of - /// [`unregister`](Self::unregister) / [`register_with_fd`](Self::register_with_fd) - /// passes `Loop::get()` (the per-thread uWS loop singleton), which is - /// non-null after init and lives for the program. The event loop is - /// single-threaded so the returned `&mut` is the sole live borrow at the - /// point of use. Collapses the two identical `&mut *loop_` deref blocks in - /// those wrappers into one. - #[inline(always)] - fn uws_loop_mut<'a>(loop_: *mut bun_uws_sys::Loop) -> &'a mut bun_uws_sys::Loop { - debug_assert!(!loop_.is_null()); - // SAFETY: type invariant — see doc comment above. - unsafe { &mut *loop_ } - } #[inline] - pub(crate) fn unregister(self, loop_: *mut bun_uws_sys::Loop, force: bool) -> sys::Result<()> { - let loop_ = Self::uws_loop_mut(loop_); + pub(crate) fn unregister(self, ev: EventLoopHandle, force: bool) -> sys::Result<()> { #[cfg(not(windows))] { - self.inner().unregister(loop_, force) + self.inner().unregister(ev, force) } #[cfg(windows)] { let _ = force; // The windows `FilePoll::unregister` always returns true — the // bool is vestigial, so there is no error path to surface here. - let _ = self.inner().unregister(loop_); + let _ = self.inner().unregister(ev.loop_mut()); Ok(()) } } #[inline] pub(crate) fn register_with_fd( self, - loop_: *mut bun_uws_sys::Loop, + ev: EventLoopHandle, kind: FilePollKind, fd: Fd, ) -> sys::Result<()> { @@ -1873,16 +1870,12 @@ impl FilePollRef { }; #[cfg(not(windows))] { - self.inner().register_with_fd( - Self::uws_loop_mut(loop_), - flag, - OneShotFlag::Dispatch, - fd, - ) + self.inner() + .register_with_fd(ev, flag, OneShotFlag::Dispatch, fd) } #[cfg(windows)] { - let _ = (loop_, flag, fd); + let _ = (ev, flag, fd); unreachable!("FilePoll fd registration is POSIX-only"); } } diff --git a/src/io/posix_event_loop.rs b/src/io/posix_event_loop.rs index e802cd0154c7..6ce8f0eb1303 100644 --- a/src/io/posix_event_loop.rs +++ b/src/io/posix_event_loop.rs @@ -336,7 +336,7 @@ impl FilePoll { // Note: these handlers take no loop parameter: holding a // protected `&mut Loop` across `on_update` would alias the fresh `&mut Loop` // that downstream `__bun_run_file_poll` handlers conjure via - // `EventLoopCtx::platform_event_loop()` when they re-enter the loop + // `EventLoopCtx::loop_mut()` when they re-enter the loop // (`register_with_fd`/`unregister`/`deinit`). #[cfg(any(target_os = "macos", target_os = "freebsd"))] pub(crate) fn on_kqueue_event(&mut self, kqueue_event: &KQueueEvent) { @@ -388,10 +388,7 @@ impl FilePoll { } fn deinit_possibly_defer(&mut self, vm: EventLoopCtx, force_unregister: bool) { - // `loop_mut()` is the crate-private nonnull-asref accessor (single - // deref in `EventLoopCtx`); the `&mut Loop` is consumed by `unregister` - // and dropped before any `&mut Store` is materialised. - let _ = self.unregister(vm.loop_mut(), force_unregister); + let _ = self.unregister(vm, force_unregister); self.owner.clear(); let was_ever_registered = self.flags.contains(Flags::WasEverRegistered); @@ -450,11 +447,19 @@ impl FilePoll { || self.flags.contains(Flags::PollProcess)) } + /// The loop this poll is registered with, not the loop `vm` currently points at. + #[inline] + fn loop_mut(&self, vm: EventLoopCtx) -> &'static mut Loop { + vm.loop_for(self.flags.contains(Flags::SpawnSyncLoop)) + } + /// This decrements the active counter if it was previously incremented /// "active" controls whether or not the event loop should potentially idle pub fn disable_keeping_process_alive(&mut self, event_loop_ctx: EventLoopCtx) { - event_loop_ctx - .loop_sub_active(self.flags.contains(Flags::HasIncrementedActiveCount) as u32); + loop_sub_active( + self.loop_mut(event_loop_ctx), + self.flags.contains(Flags::HasIncrementedActiveCount) as u32, + ); self.flags.remove(Flags::KeepsEventLoopAlive); self.flags.remove(Flags::HasIncrementedActiveCount); @@ -465,8 +470,10 @@ impl FilePoll { return; } - event_loop_ctx - .loop_add_active((!self.flags.contains(Flags::HasIncrementedActiveCount)) as u32); + loop_add_active( + self.loop_mut(event_loop_ctx), + (!self.flags.contains(Flags::HasIncrementedActiveCount)) as u32, + ); self.flags.insert(Flags::KeepsEventLoopAlive); self.flags.insert(Flags::HasIncrementedActiveCount); @@ -514,7 +521,10 @@ impl FilePoll { /// non-macOS-debug builds and then read it in the `syslog!` below. Building /// the whole struct by value fixes both. #[inline] - fn new_value(vm: EventLoopCtx, fd: Fd, flags: FlagsSet, owner: Owner) -> FilePoll { + fn new_value(vm: EventLoopCtx, fd: Fd, mut flags: FlagsSet, owner: Owner) -> FilePoll { + if vm.is_spawn_sync_loop() { + flags.insert(Flags::SpawnSyncLoop); + } FilePoll { fd, flags, @@ -545,9 +555,9 @@ impl FilePoll { poll } - pub fn register(&mut self, loop_: &mut Loop, flag: Flags, one_shot: bool) -> sys::Result<()> { + pub fn register(&mut self, vm: EventLoopCtx, flag: Flags, one_shot: bool) -> sys::Result<()> { self.register_with_fd( - loop_, + vm, flag, if one_shot { OneShotFlag::OneShot @@ -560,7 +570,7 @@ impl FilePoll { pub fn register_with_fd( &mut self, - loop_: &mut Loop, + vm: EventLoopCtx, flag: Flags, one_shot: OneShotFlag, fd: Fd, @@ -571,7 +581,7 @@ impl FilePoll { target_os = "macos", target_os = "freebsd" ))] - return self.register_with_fd_impl(loop_, flag, one_shot, fd); + return self.register_with_fd_impl(self.loop_mut(vm), flag, one_shot, fd); #[cfg(not(any( target_os = "linux", target_os = "android", @@ -579,7 +589,7 @@ impl FilePoll { target_os = "freebsd" )))] { - let _ = (loop_, flag, one_shot, fd); + let _ = (vm, flag, one_shot, fd); sys::Result::Ok(()) } } @@ -861,16 +871,17 @@ impl FilePoll { sys::Result::Ok(()) } - pub fn unregister(&mut self, loop_: &mut Loop, force_unregister: bool) -> sys::Result<()> { - self.unregister_with_fd(loop_, self.fd, force_unregister) + pub fn unregister(&mut self, vm: EventLoopCtx, force_unregister: bool) -> sys::Result<()> { + self.unregister_with_fd(vm, self.fd, force_unregister) } pub(crate) fn unregister_with_fd( &mut self, - loop_: &mut Loop, + vm: EventLoopCtx, fd: Fd, force_unregister: bool, ) -> sys::Result<()> { + let loop_ = self.loop_mut(vm); // Note: compute the syscall result first, then unconditionally // deactivate. Avoids a raw-pointer scopeguard. #[cfg(any( @@ -1216,6 +1227,9 @@ pub enum Flags { Socket, Tty, + + /// Registered on `Bun.spawnSync`'s isolated loop, not the thread's loop. + SpawnSyncLoop, } pub type FlagsSet = enumset::EnumSet; @@ -1490,7 +1504,7 @@ unsafe extern "C" fn Bun__internal_dispatch_ready_poll( // SAFETY: `loop_` is the live uws loop. Do *not* materialize `&mut *loop_` // here — `on_update` (via `__bun_run_file_poll`) re-enters the loop and conjures - // a fresh `&mut Loop` through `EventLoopCtx::platform_event_loop()`; a + // a fresh `&mut Loop` through `EventLoopCtx::loop_mut()`; a // protected `&mut Loop` spanning that call would be SB-UB. Take a short-lived // `&*loop_` only to copy the POD event onto the stack (the `BackRef`-style // accessor returns by value), then drop the borrow before dispatching so the diff --git a/src/runtime/api/bun/Terminal.rs b/src/runtime/api/bun/Terminal.rs index a41db046700e..73489294820b 100644 --- a/src/runtime/api/bun/Terminal.rs +++ b/src/runtime/api/bun/Terminal.rs @@ -2072,9 +2072,6 @@ impl bun_io::pipe_writer::PosixStreamingWriterParent for Terminal { .event_loop_handle .as_event_loop_ctx() } - unsafe fn loop_(this: *mut Self) -> *mut bun_uws_sys::Loop { - Self::from_parent_ptr(this).event_loop_handle.r#loop() - } } #[cfg(windows)] diff --git a/src/runtime/api/bun/subprocess.rs b/src/runtime/api/bun/subprocess.rs index e4e6a456198b..2060d9b327cd 100644 --- a/src/runtime/api/bun/subprocess.rs +++ b/src/runtime/api/bun/subprocess.rs @@ -647,9 +647,8 @@ impl Subprocess<'_> { return; } self.event_loop_timer_refd.set(refd); - let uws_loop = self.global_this().bun_vm().uws_loop(); let delta: i32 = if refd { 1 } else { -1 }; - Self::timer_all().increment_timer_ref(delta, uws_loop); + Self::timer_all().increment_timer_ref(delta, bun_io::js_vm_ctx()); } #[inline] diff --git a/src/runtime/dns_jsc/dns.rs b/src/runtime/dns_jsc/dns.rs index 5634a548d18d..b33ab5afb13e 100644 --- a/src/runtime/dns_jsc/dns.rs +++ b/src/runtime/dns_jsc/dns.rs @@ -4100,7 +4100,6 @@ impl Resolver { sec: now.sec, nsec: now.nsec, }; - let uws_loop = vm.uws_loop(); // R-2: `&self` carries no `noalias`, and every field touched below is // UnsafeCell-backed, so the re-entrant `ares_process_fd` callbacks // (`request_completed`, `drain_pending_*`) may freely re-derive @@ -4111,7 +4110,7 @@ impl Resolver { // resolve via the high-tier `RuntimeState` hook. let state = crate::jsc_hooks::runtime_state(); // SAFETY: `state` is the boxed per-thread `RuntimeState`; single-threaded JS heap. - unsafe { (*state).timer.increment_timer_ref(-1, uws_loop) }; + unsafe { (*state).timer.increment_timer_ref(-1, js_event_loop_ctx()) }; // SAFETY: `deref_this` is the heap allocation from `init`; releases // `add_timer`'s ref. May be the final release; nothing touches // `*self` after this point. @@ -4203,11 +4202,10 @@ impl Resolver { nsec: next.nsec, } }); - let uws_loop = self.vm().uws_loop(); let state = crate::jsc_hooks::runtime_state(); // SAFETY: `state` is the boxed per-thread `RuntimeState`; single-threaded JS heap. unsafe { - (*state).timer.increment_timer_ref(1, uws_loop); + (*state).timer.increment_timer_ref(1, js_event_loop_ctx()); // whole-struct provenance: `from_field_ptr!` recovers the container on fire (*state).timer.insert( core::ptr::addr_of!(self.event_loop_timer) @@ -4232,9 +4230,8 @@ impl Resolver { // global-resolver permanent pin), so this // `deref` cannot reach 0 while `&self` is live. unsafe { - let uws_loop = (*this).vm().uws_loop(); let state = crate::jsc_hooks::runtime_state(); - (*state).timer.increment_timer_ref(-1, uws_loop); + (*state).timer.increment_timer_ref(-1, js_event_loop_ctx()); Self::deref(this); } } @@ -4956,8 +4953,6 @@ impl Resolver { Async::posix_event_loop::poll_tag::DNS_RESOLVER, self.as_ctx_ptr().cast::<()>(), ); - // SAFETY: `event_loop_handle` is set once VM is initialized; live for VM lifetime. - let loop_ = unsafe { &mut *self.vm().event_loop_handle.unwrap() }; // SAFETY: single-JS-thread; the `&mut PollsMap` borrow does not span // any re-entrant call (`FilePoll::register` is a syscall wrapper). let polls = unsafe { self.polls.get_mut() }; @@ -4987,12 +4982,12 @@ impl Resolver { // direction armed would busy-loop on level-triggered writable // once the socket connects. Full resync is the simplest // correct path and c-ares DNS fds are short-lived. - let _ = poll.unregister(loop_, false); + let _ = poll.unregister(ctx, false); if readable { - let _ = poll.register(loop_, Async::PollKind::Readable, false); + let _ = poll.register(ctx, Async::PollKind::Readable, false); } if writable { - let _ = poll.register(loop_, Async::PollKind::Writable, false); + let _ = poll.register(ctx, Async::PollKind::Writable, false); } } else { // Only adding directions (or no change). register() issues a @@ -5000,10 +4995,10 @@ impl Resolver { // on kqueue EV_ADD creates a separate (ident, filter) knote // without disturbing the existing one. if readable && !have_readable { - let _ = poll.register(loop_, Async::PollKind::Readable, false); + let _ = poll.register(ctx, Async::PollKind::Readable, false); } if writable && !have_writable { - let _ = poll.register(loop_, Async::PollKind::Writable, false); + let _ = poll.register(ctx, Async::PollKind::Writable, false); } } } diff --git a/src/runtime/dns_jsc/dns_sd.rs b/src/runtime/dns_jsc/dns_sd.rs index a2a7d844f6a4..bea668a93f7b 100644 --- a/src/runtime/dns_jsc/dns_sd.rs +++ b/src/runtime/dns_jsc/dns_sd.rs @@ -381,10 +381,8 @@ impl SharedConnection { ); // SAFETY: `FilePoll::init` returned a live pool slot; exclusive here. let poll = unsafe { &mut *poll_ptr }; - // SAFETY: the event loop outlives every lookup made on it. - let loop_ = unsafe { ctx.platform_event_loop() }; let rc = poll.register_with_fd( - loop_, + ctx, Async::PollKind::Readable, Async::posix_event_loop::OneShotFlag::None, fd, diff --git a/src/runtime/node/memory_pressure.rs b/src/runtime/node/memory_pressure.rs index f5c89fdd6c19..ea256fba73e6 100644 --- a/src/runtime/node/memory_pressure.rs +++ b/src/runtime/node/memory_pressure.rs @@ -246,10 +246,8 @@ mod posix { NonNull::<()>::dangling().as_ptr(), ), ); - // SAFETY: `poll` is the fresh hive slot; `platform_event_loop` is the live uws loop. - let result = unsafe { - (*poll).register(ctx.platform_event_loop(), Flags::MemoryPressure, false) - }; + // SAFETY: `poll` is the fresh hive slot. + let result = unsafe { (*poll).register(ctx, Flags::MemoryPressure, false) }; if result.is_err() { // SAFETY: fresh hive slot never handed out. deinit_poll(unsafe { &mut *poll }); diff --git a/src/runtime/timer/mod.rs b/src/runtime/timer/mod.rs index ec72b1beed2b..5aa336278acb 100644 --- a/src/runtime/timer/mod.rs +++ b/src/runtime/timer/mod.rs @@ -586,6 +586,9 @@ pub(crate) struct All { pub(crate) thread_id: std::thread::ThreadId, pub(crate) timers: TimerHeap, pub(crate) active_timer_count: i32, + /// `active_timer_count` went positive on `Bun.spawnSync`'s isolated loop. + #[cfg(not(windows))] + timer_refd_spawn_sync_loop: bool, #[cfg(windows)] pub(crate) uv_timer: bun_sys::windows::libuv::Timer, /// Whether we have emitted a warning for passing a negative timeout duration @@ -596,6 +599,9 @@ pub(crate) struct All { /// TimerObjectInternals.epoch. Masked to 25 bits on increment. pub(crate) epoch: u32, pub(crate) immediate_ref_count: i32, + /// `immediate_ref_count` went positive on `Bun.spawnSync`'s isolated loop. + #[cfg(not(windows))] + immediate_refd_spawn_sync_loop: bool, #[cfg(windows)] pub(crate) uv_idle: bun_sys::windows::libuv::uv_idle_t, pub(crate) event_loop_delay: EventLoopDelayMonitor, @@ -612,12 +618,16 @@ impl All { thread_id: std::thread::current().id(), timers: TimerHeap::default(), active_timer_count: 0, + #[cfg(not(windows))] + timer_refd_spawn_sync_loop: false, #[cfg(windows)] uv_timer: bun_core::ffi::zeroed(), warned_negative_number: false, warned_not_number: false, epoch: 0, immediate_ref_count: 0, + #[cfg(not(windows))] + immediate_refd_spawn_sync_loop: false, #[cfg(windows)] uv_idle: bun_core::ffi::zeroed(), event_loop_delay: EventLoopDelayMonitor::default(), @@ -1085,19 +1095,15 @@ impl All { } } - /// # Safety - /// `uws_loop` must point to the calling VM's live uws loop. - // `uws_loop` is an FFI handle held as `*mut` by every caller; contract is - // documented in `# Safety` above. Cannot be `&mut` without breaking the - // out-of-file call sites that hold raw pointers. - pub(crate) fn increment_immediate_ref(&mut self, delta: i32, uws_loop: *mut bun_uws_sys::Loop) { + pub(crate) fn increment_immediate_ref(&mut self, delta: i32, ctx: bun_io::EventLoopCtx) { let old = self.immediate_ref_count; let new = old + delta; self.immediate_ref_count = new; if old <= 0 && new > 0 { #[cfg(not(windows))] - // SAFETY: caller passes the VM's live uws loop - unsafe { &mut *uws_loop }.ref_(); + { + self.immediate_refd_spawn_sync_loop = ctx.loop_ref(); + } #[cfg(windows)] { // Lazy-init the idle handle and start @@ -1114,15 +1120,14 @@ impl All { } } else if old > 0 && new <= 0 { #[cfg(not(windows))] - // SAFETY: caller passes the VM's live uws loop - unsafe { &mut *uws_loop }.unref(); + ctx.loop_unref_for(self.immediate_refd_spawn_sync_loop); #[cfg(windows)] if !self.uv_idle.data.is_null() { self.uv_idle.stop(); } } #[cfg(windows)] - let _ = uws_loop; + let _ = ctx; } /// Empty `uv_idle` callback. Its presence alone @@ -1134,20 +1139,16 @@ impl All { // prevent libuv from polling forever } - /// # Safety - /// `uws_loop` must point to the calling VM's live uws loop. - // `uws_loop` is an FFI handle held as `*mut` by every caller; contract is - // documented in `# Safety` above. Cannot be `&mut` without breaking the - // out-of-file call sites that hold raw pointers. - pub(crate) fn increment_timer_ref(&mut self, delta: i32, uws_loop: *mut bun_uws_sys::Loop) { + pub(crate) fn increment_timer_ref(&mut self, delta: i32, ctx: bun_io::EventLoopCtx) { let old = self.active_timer_count; let new = old + delta; debug_assert!(new >= 0); self.active_timer_count = new; if old <= 0 && new > 0 { #[cfg(not(windows))] - // SAFETY: caller passes the VM's live uws loop - unsafe { &mut *uws_loop }.ref_(); + { + self.timer_refd_spawn_sync_loop = ctx.loop_ref(); + } // `uv_timer.ref()` is intentionally unconditional (no `data != // null` guard). Invariant: every path that reaches a positive // `active_timer_count` first inserts a timer, and `insert` @@ -1157,13 +1158,12 @@ impl All { self.uv_timer.ref_(); } else if old > 0 && new <= 0 { #[cfg(not(windows))] - // SAFETY: caller passes the VM's live uws loop - unsafe { &mut *uws_loop }.unref(); + ctx.loop_unref_for(self.timer_refd_spawn_sync_loop); #[cfg(windows)] self.uv_timer.unref(); } #[cfg(windows)] - let _ = uws_loop; + let _ = ctx; } /// VM teardown, after `cancel_all_timeout_objects`: unlink every timer still diff --git a/src/runtime/timer/timer_object_internals.rs b/src/runtime/timer/timer_object_internals.rs index 0a15698cf178..8be1e135bc70 100644 --- a/src/runtime/timer/timer_object_internals.rs +++ b/src/runtime/timer/timer_object_internals.rs @@ -192,20 +192,18 @@ impl TimerObjectInternals { let state = crate::jsc_hooks::runtime_state(); debug_assert!(!state.is_null(), "RuntimeState not installed"); - // SAFETY: `vm` is the live per-thread VM (hook contract); field read only. - let uws_loop = unsafe { (*vm).uws_loop() }; + // SAFETY: `vm` is the live per-thread VM (hook contract). + let ctx = unsafe { VirtualMachine::event_loop_ctx(vm) }; let delta = if enable { 1 } else { -1 }; match self.flags.get().kind() { // SAFETY: `state` points at the boxed per-thread `RuntimeState`; // single-threaded JS heap so no concurrent `&mut` to `.timer`. Kind::SetTimeout | Kind::SetInterval => unsafe { - (*state).timer.increment_timer_ref(delta, uws_loop) + (*state).timer.increment_timer_ref(delta, ctx) }, // setImmediate has slightly different event loop logic // SAFETY: as above. - Kind::SetImmediate => unsafe { - (*state).timer.increment_immediate_ref(delta, uws_loop) - }, + Kind::SetImmediate => unsafe { (*state).timer.increment_immediate_ref(delta, ctx) }, } } diff --git a/src/runtime/webcore/FileSink.rs b/src/runtime/webcore/FileSink.rs index e517c301d912..f7700ad07065 100644 --- a/src/runtime/webcore/FileSink.rs +++ b/src/runtime/webcore/FileSink.rs @@ -154,7 +154,6 @@ bun_io::impl_streaming_writer_parent! { on_ready = on_ready, on_close = on_close, event_loop = |this| (*this).io_evtloop(), - uws_loop = |this| (*this).event_loop_handle.r#loop(), uv_loop = |this| (*this).event_loop_handle.uv_loop(), ref_ = |this| (&*this).ref_(), deref = |this| FileSink::deref(this), diff --git a/src/spawn/process.rs b/src/spawn/process.rs index 1de6cc81d2c4..c4ac6674205f 100644 --- a/src/spawn/process.rs +++ b/src/spawn/process.rs @@ -473,15 +473,9 @@ impl Process { // Borrow scoped to the call. unsafe { (*poll).enable_keeping_process_alive(ctx) }; - // SAFETY: poll is live and `platform_event_loop` returns the live - // uws loop; both `&mut`s are scoped to the `register` call. - match unsafe { - (*poll).register( - &mut *self.event_loop.platform_event_loop(), - bun_io::PollKind::Process, - PROCESS_POLL_ONE_SHOT, - ) - } { + // SAFETY: poll is live; borrow scoped to the `register` call. + match unsafe { (*poll).register(ctx, bun_io::PollKind::Process, PROCESS_POLL_ONE_SHOT) } + { Ok(()) => { self.ref_(); Ok(()) @@ -511,13 +505,7 @@ impl Process { } if let Some(fd) = self.poller.fd_poll_mut() { - // SAFETY: `platform_event_loop` returns the live uws loop; borrow - // scoped to the `register` call. - let maybe = fd.register( - unsafe { &mut *self.event_loop.platform_event_loop() }, - bun_io::PollKind::Process, - PROCESS_POLL_ONE_SHOT, - ); + let maybe = fd.register(ctx, bun_io::PollKind::Process, PROCESS_POLL_ONE_SHOT); if maybe.is_ok() { self.ref_(); } diff --git a/test/js/bun/spawn/spawnSync.test.ts b/test/js/bun/spawn/spawnSync.test.ts index c5b7a3c9fc5d..ac5040c922f0 100644 --- a/test/js/bun/spawn/spawnSync.test.ts +++ b/test/js/bun/spawn/spawnSync.test.ts @@ -1,5 +1,5 @@ -import { describe, expect, it } from "bun:test"; -import { bunEnv, bunExe, bunRun, isLinux, isMusl, isPosix, isWindows } from "harness"; +import { describe, expect, it, test } from "bun:test"; +import { bunEnv, bunExe, bunRun, isLinux, isMusl, isPosix, isWindows, tempDir } from "harness"; import { totalmem } from "os"; import { join } from "path"; describe("spawnSync", () => { @@ -225,3 +225,67 @@ describe("uid/gid", () => { expect(thrown?.code).toBe("EPERM"); }); }); + +// spawnSync points the VM's loop handle at its private loop for the whole +// call. A FilePoll torn down inside that window (here: the GC finalizing a +// garbage `Bun.file(2).writer()` while spawnSync builds its result) must +// still unregister from the loop it was registered with. Before the fix the +// EPOLL_CTL_DEL went to the private loop, so the main loop kept a stale +// entry for the dup'd fd number. The next `Bun.file(2).writer()` dups to the +// same number and EPOLL_CTL_ADD fails with EEXIST. +test.skipIf(!isLinux)("a writer finalized during spawnSync does not break the next writer on the same fd", async () => { + // A file, not `-e`: run through `-e`, the collection that `Bun.gc(false)` + // requests does not finalize the writers inside spawnSync (their dups stay + // open for the whole wait), and the test no longer reaches the bug. + using dir = tempDir("spawnsync-writer-finalized", { + "fixture.js": ` + import { fstatSync, readdirSync } from "node:fs"; + const stderrInode = fstatSync(2).ino; + // Every writer holds a dup() of fd 2; those share stderr's inode. + const stderrDups = () => + readdirSync("/proc/self/fd") + .map(Number) + .filter(fd => { + if (fd === 2) return false; + try { + return fstatSync(fd).ino === stderrInode; + } catch { + return false; + } + }); + function leakWriter() { + const w = Bun.file(2).writer(); + w.write(""); + } + for (let i = 0; i < 4; i++) leakWriter(); + const dups = stderrDups().length; + // The collector finishes inside spawnSync: the first JS allocation after + // the wait (the result object) is where the mutator runs the sweep. + Bun.gc(false); + const r = Bun.spawnSync({ cmd: ["sleep", "0.2"] }); + // The finalized writers close their fds on the work pool; wait for that so + // dup(2) below hands out one of the same numbers. + const deadline = Date.now() + 5000; + while (Date.now() < deadline && stderrDups().length > 0) Bun.sleepSync(5); + const remaining = stderrDups().length; + const w = Bun.file(2).writer(); + w.write("second writer ok\\n"); + w.flush(); + console.log(JSON.stringify({ exitCode: r.exitCode, dups, remaining })); + `, + }); + await using proc = Bun.spawn({ + cmd: [bunExe(), "fixture.js"], + env: bunEnv, + cwd: String(dir), + stdout: "pipe", + // A pipe is pollable, so the writers register their dup of fd 2 with epoll. + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ + stdout: JSON.stringify({ exitCode: 0, dups: 4, remaining: 0 }) + "\n", + stderr: "second writer ok\n", + exitCode: 0, + }); +}); diff --git a/test/js/bun/spawn/spawnsync-isolated-event-loop.test.ts b/test/js/bun/spawn/spawnsync-isolated-event-loop.test.ts index 70619fddfcb0..8c7fdcdb94b5 100644 --- a/test/js/bun/spawn/spawnsync-isolated-event-loop.test.ts +++ b/test/js/bun/spawn/spawnsync-isolated-event-loop.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { bunEnv, bunExe } from "harness"; +import { bunEnv, bunExe, isWindows } from "harness"; import { join } from "node:path"; describe.concurrent("spawnSync isolated event loop", () => { @@ -188,4 +188,82 @@ describe.concurrent("spawnSync isolated event loop", () => { expect(stdout).not.toContain("FAIL"); expect(exitCode).toBe(0); }); + + // A GC that runs while spawnSync blocks finalizes objects whose polls live on + // the main loop. If those polls are released against the isolated loop, its + // poll count reaches zero while the next spawnSync still has live polls, and + // that spawnSync spins without polling until its timeout. + // BUN_JSC_slowPathAllocsBetweenGCs runs a full synchronous GC every few + // slow-path allocations. A warmed-up spawnSync allocates nothing before it + // installs the isolated loop, so the GC that frees the writers runs inside it. + // The fixture also reports how many writers were collected by the time the + // probe call returned. They are held by a global until the drop right before + // that call, so a change that keeps them alive past the call, or stops the + // GC from running in it, fails this test instead of passing it. + // (bun:internal-for-testing or node:fs would give fd and sink counts, but + // loading either module under this GC setting costs a debug build 5s to 15s.) + test.skipIf(isWindows)("finalizers that run inside spawnSync do not stall the next spawnSync", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + // Three writers on stderr (a pipe here), each with a poll registered + // on the main loop. They hang off globalThis: a binding that is only + // written after the await below is dead at the suspension, and the + // writers would already be collected in the warm-up call. + globalThis.writers = []; + const refs = []; + for (let i = 0; i < 3; i++) { + const writer = Bun.stderr.writer(); + writer.write(""); + writer.flush(); + globalThis.writers.push(writer); + refs.push(new WeakRef(writer)); + } + // A WeakRef keeps its target alive until the end of the job that + // created it. + await Bun.sleep(0); + const first = { cmd: ["echo", "first"], stdout: "pipe", stderr: "ignore", timeout: 2000 }; + // pidfd + stdout + stderr: three polls, the same count the three + // writers release. The child outlives the parent's poll registration, + // so the parent has to poll for its exit. + const second = { cmd: ["sh", "-c", "sleep 0.3; echo second"], stdout: "pipe", stderr: "pipe", timeout: 2000 }; + + // Warm up while the writers are still reachable: lazy structures and + // Bun.spawnSync itself are created here, on the main loop. + Bun.spawnSync(first); + globalThis.writers = null; + // The first allocations after the drop are this call's result, built + // while the isolated loop is installed: the GC they trigger frees the + // writers there. Nothing may run between the drop and the call. + Bun.spawnSync(first); + // Read the refs before anything allocates: a GC here would change the answer. + let collectedDuringCall = 0; + for (let i = 0; i < refs.length; i++) if (refs[i].deref() === undefined) collectedDuringCall++; + + const result = Bun.spawnSync(second); + console.log( + JSON.stringify({ + collectedDuringCall, + stdout: result.stdout.toString(), + exitedDueToTimeout: result.exitedDueToTimeout, + exitCode: result.exitCode, + }), + ); + `, + ], + env: { ...bunEnv, BUN_JSC_slowPathAllocsBetweenGCs: "5" }, + stdout: "pipe", + stderr: "pipe", + }); + + // stderr is drained, not asserted: the child's JSON and exit code carry the signal. + const [stdout, , exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stdout.trim()).toBe( + JSON.stringify({ collectedDuringCall: 3, stdout: "second\n", exitedDueToTimeout: false, exitCode: 0 }), + ); + expect(exitCode).toBe(0); + }); });