Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 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
1 change: 0 additions & 1 deletion src/event_loop/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ bun_dispatch::link_interface! {
fn exit();
fn enqueue_task(task: Task);
fn enqueue_task_concurrent(task: core::ptr::NonNull<ConcurrentTask::ConcurrentTask>);
fn concurrent_poster_end();
fn env() -> *mut bun_dotenv::Loader;
fn top_level_dir() -> *const [u8];
fn create_null_delimited_env_map() -> Result<bun_dotenv::NullDelimitedEnvMap, bun_core::AllocError>;
Expand Down
17 changes: 13 additions & 4 deletions src/jsc/VirtualMachine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,14 @@ impl VirtualMachine {
unsafe { &*self.event_loop }
}

/// Close both loops' [`crate::event_loop::ConcurrentPosterGate`]s (macro mode can have
/// pointed tasks at either loop) ahead of the final queue drain: no work-pool completion can
/// post after this returns. See the gate type for why this never waits on a blocked syscall.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn close_concurrent_posters(&mut self) {
self.regular_event_loop.close_concurrent_posters();
self.macro_event_loop.close_concurrent_posters();
}

/// Alias for [`Self::event_loop_mut`]. Kept for callers migrated on the
/// `runtime-hostfn-safe` branch; both names funnel into the single audited
/// `unsafe` deref above.
Expand Down Expand Up @@ -1638,10 +1646,11 @@ impl VirtualMachine {
bun_http::shutdown_for_exit();

// Release tasks the HTTP daemon posted before observing `is_shutting_down` (else the
// tasklet ⇄ `Box<AsyncHTTP>` cycle leaks); must precede `destructOnExit`. Wait for
// work-pool fs completions first — they post without a shutdown check, so a post
// landing after the drain leaks.
self.event_loop_mut().wait_for_concurrent_posters();
// tasklet ⇄ `Box<AsyncHTTP>` cycle leaks); must precede `destructOnExit`. Close the
// poster gates first so no work-pool fs completion can post after the drain; a
// completion still blocked in its syscall is refused at its gate later and frees
// itself without touching the VM.
Comment thread
robobun marked this conversation as resolved.
Outdated
self.close_concurrent_posters();
self.event_loop_mut().release_queued_tasks_for_shutdown();

if let Some(rare) = self.rare_data.as_deref_mut() {
Expand Down
114 changes: 90 additions & 24 deletions src/jsc/event_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,12 @@

pub entered_event_loop_count: isize,
pub concurrent_ref: AtomicI32,
/// Work-pool tasks that will post to `concurrent_tasks` when they finish (counted on the JS
/// thread before hand-off, decremented on the pool thread after the post). Shutdown waits for
/// zero before the final queue drain so a completion can't land after it and leak.
pub concurrent_posters: AtomicU32,
/// Gate shared (via `Arc` clones) with every scheduled work-pool task whose completion posts
/// back through [`Self::enqueue_task_concurrent`]. Shutdown closes it before the final queue
/// drain so a completion can't land after the drain; a poster that arrives later observes the
/// closed gate and frees its task without touching this (possibly already freed) loop. `None`
/// only after [`Self::deinit`].
Comment thread
robobun marked this conversation as resolved.
Outdated
pub concurrent_poster_gate: Option<std::sync::Arc<ConcurrentPosterGate>>,
/// Atomic nullable pointer to the next-due `WTFTimer`.
///
/// Note (§Dispatch): payload is `*mut ()` — the real
Expand Down Expand Up @@ -132,7 +134,7 @@
uws_loop: (),
entered_event_loop_count: 0,
concurrent_ref: AtomicI32::new(0),
concurrent_posters: AtomicU32::new(0),
concurrent_poster_gate: Some(std::sync::Arc::new(ConcurrentPosterGate::default())),
imminent_gc_timer: AtomicPtr::new(core::ptr::null_mut()),
#[cfg(unix)]
signal_handler: None,
Expand All @@ -142,6 +144,69 @@
}
}

/// Gate between work-pool completion posts and event-loop teardown.
///
/// A scheduled fs operation can block indefinitely (a `read()` from a FIFO or pipe with no
/// writer), so shutdown must not wait for it — the old whole-operation poster count turned
/// `worker.terminate()` into an unbounded spin. Instead, each poster wraps ONLY its completion
/// post in [`Self::begin_post`]/[`Self::end_post`], and [`Self::close`] refuses everything that
/// comes later. The gate is `Arc`-shared with every in-flight task precisely so a poster that
/// outlives the VM can still load the closed state from live memory.
///
/// Protocol guarantees, in terms of the single `state` word's modification order:
/// - `begin_post() == true` ⇒ the increment preceded `close()`'s closed-bit store, so `close()`
/// cannot return before the matching [`Self::end_post`] — the event loop and its VM stay live
/// for the whole post.
/// - `begin_post() == false` ⇒ the loop may already be freed; the caller still owns its task and
/// must dispose of it without touching the VM or the JS heap (`dispose_without_post` in
/// `node_fs.rs`).
Comment thread
robobun marked this conversation as resolved.
Outdated
#[derive(Default)]
pub struct ConcurrentPosterGate {
/// Bit 31: closed. Bits 0..31: posters currently between `begin_post` and `end_post`.
state: AtomicU32,
}

impl ConcurrentPosterGate {
const CLOSED: u32 = 1 << 31;

/// Work-pool thread: begin a completion post. On `true`, the loop is guaranteed live until
/// the matching [`Self::end_post`].
Comment thread
robobun marked this conversation as resolved.
Outdated
#[must_use]
pub fn begin_post(&self) -> bool {
let mut state = self.state.load(Ordering::Relaxed);
loop {
if state & Self::CLOSED != 0 {
return false;
}
match self.state.compare_exchange_weak(
state,
state + 1,
Ordering::Acquire,
Ordering::Relaxed,
) {
Ok(_) => return true,
Err(actual) => state = actual,
}
}
}

/// Work-pool thread: the post finished. Last touch of the event loop; after this the JS
/// thread may drain the queue and tear the VM down.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn end_post(&self) {
let prev = self.state.fetch_sub(1, Ordering::Release);
debug_assert!(prev & !Self::CLOSED > 0);
}

/// JS thread, shutdown: no post begun after this returns can succeed, and every post that
/// already began has finished. Bounded by an enqueue, not by the underlying fs operation.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn close(&self) {
self.state.fetch_or(Self::CLOSED, Ordering::AcqRel);
while self.state.load(Ordering::Acquire) & !Self::CLOSED != 0 {
std::thread::yield_now();
}
}
}

mod drain_result {
pub(super) const SUCCESS: u8 = 0;
pub(super) const JS_TERMINATED: u8 = 1;
Expand Down Expand Up @@ -770,6 +835,11 @@
}

pub fn deinit(&mut self) {
// Drop this loop's ref to the poster gate (closed by now on every path that deinits a
// loop with posters). The VM box is raw-dealloc'd without field `Drop`s, so the `Arc`
// must be released here; stranded posters hold their own clones and free the allocation
// when the last one finishes.
Comment thread
robobun marked this conversation as resolved.
Outdated
drop(self.concurrent_poster_gate.take());
// Free (don't run — running could re-enter the dying VM) queued
// ManagedTask boxes. Other tags are left in place: they were re-queued
// by `release_queued_tasks_for_shutdown` because their callback can't
Expand Down Expand Up @@ -1010,26 +1080,23 @@
self.wakeup();
}

/// See `concurrent_posters`. Call on the JS thread before scheduling a
/// work-pool task whose completion posts via `enqueue_task_concurrent`.
pub fn concurrent_poster_begin(&self) {
let _ = self.concurrent_posters.fetch_add(1, Ordering::SeqCst);
/// Clone of [`Self::concurrent_poster_gate`] for a work-pool task scheduled on the JS
/// thread. Panics after [`Self::deinit`] — tasks are only created while the VM is live.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn poster_gate(&self) -> std::sync::Arc<ConcurrentPosterGate> {
self.concurrent_poster_gate

Check failure on line 1086 in src/jsc/event_loop.rs

View workflow job for this annotation

GitHub Actions / cargo clippy

using `.clone()` on a ref-counted pointer
.as_ref()
.expect("poster_gate() after EventLoop::deinit()")
.clone()
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/// Pool-thread pair of [`Self::concurrent_poster_begin`]. Must be the
/// poster's last touch of this event loop: once the count hits zero the
/// shutdown thread may drain the queue and tear the VM down.
pub fn concurrent_poster_end(&self) {
let prev = self.concurrent_posters.fetch_sub(1, Ordering::Release);
debug_assert!(prev > 0);
}

/// Spin until every counted poster has finished its post. Called on the JS thread during
/// shutdown, after the last JS has run and before the final queue drain. Each pending fs
/// operation is finite, so the wait is bounded by syscall latency.
pub fn wait_for_concurrent_posters(&self) {
while self.concurrent_posters.load(Ordering::Acquire) > 0 {
std::thread::yield_now();
/// Close [`Self::concurrent_poster_gate`]: refuse all future work-pool completion posts and
/// wait out the ones already mid-enqueue. Called on the JS thread during shutdown, after the
/// last JS has run and before the final queue drain, so the drain sees every post that will
/// ever land. Unlike the fs operations themselves (which can block forever on a pipe/FIFO),
/// the wait here is bounded by an enqueue + wakeup.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn close_concurrent_posters(&self) {
if let Some(gate) = self.concurrent_poster_gate.as_ref() {
gate.close();
}
}

Expand Down Expand Up @@ -1343,7 +1410,6 @@
exit() => (*this).exit(),
enqueue_task(task) => (*this).enqueue_task(task),
enqueue_task_concurrent(task) => (*this).enqueue_task_concurrent(task),
concurrent_poster_end() => (*this).concurrent_poster_end(),
env() => (*this).vm_ref().transpiler.env,
top_level_dir() => core::ptr::from_ref::<[u8]>((*this).vm_ref().top_level_dir()),
create_null_delimited_env_map() =>
Expand Down
9 changes: 5 additions & 4 deletions src/jsc/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1359,10 +1359,11 @@ pub use self::event_loop as EventLoop;
pub mod any_task_job;
pub use self::any_task_job::{AnyTaskJob, AnyTaskJobCtx};
pub use self::event_loop::{
AnyEventLoop, AnyTaskWithExtraContext, ConcurrentCppTask, ConcurrentPromiseTask,
ConcurrentTask, CppTask, DeferredTaskQueue, EventLoopHandle, EventLoopTask, EventLoopTaskPtr,
GarbageCollectionController, JsTerminated, JsTerminatedResult, ManagedTask, MiniEventLoop,
PosixSignalHandle, PosixSignalTask, Task, WorkPool, WorkPoolTask, WorkTask, WorkTaskContext,
AnyEventLoop, AnyTaskWithExtraContext, ConcurrentCppTask, ConcurrentPosterGate,
ConcurrentPromiseTask, ConcurrentTask, CppTask, DeferredTaskQueue, EventLoopHandle,
EventLoopTask, EventLoopTaskPtr, GarbageCollectionController, JsTerminated, JsTerminatedResult,
ManagedTask, MiniEventLoop, PosixSignalHandle, PosixSignalTask, Task, WorkPool, WorkPoolTask,
WorkTask, WorkTaskContext,
};
#[cfg(unix)]
pub type PlatformEventLoop = bun_uws::Loop;
Expand Down
14 changes: 14 additions & 0 deletions src/jsc/node_path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,20 @@
pub fn adopt(value: T) -> Self {
Self(value)
}

/// Drop the inner `T` (freeing its owned Rust payloads) WITHOUT running
/// [`Unprotect::unprotect`]. Only for teardown paths where the VM that
/// held the protect counts is already gone — the GC roots died with the
/// heap, and touching them would be a use-after-free. `to_thread_safe()`
/// already converted any thread-affine strings, so the inner drop is safe
/// off the JS thread.
Comment thread
robobun marked this conversation as resolved.
Outdated
pub fn dispose_skip_unprotect(self) {
let this = core::mem::ManuallyDrop::new(self);
// SAFETY: `this` is `ManuallyDrop`, so `ThreadSafe::drop` (the
// unprotect) never runs and the inner value is read out exactly once;
// its own `Drop` frees the owned payloads.
drop(unsafe { core::ptr::read(&this.0) });

Check failure on line 66 in src/jsc/node_path.rs

View workflow job for this annotation

GitHub Actions / cargo clippy

implicit borrow as raw pointer
}
Comment thread
robobun marked this conversation as resolved.
Outdated
}

impl<T: Unprotect> core::ops::Deref for ThreadSafe<T> {
Expand Down
9 changes: 5 additions & 4 deletions src/jsc/web_worker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1308,10 +1308,11 @@ impl WebWorker {
// teardownJSCVM sets it again.
Bun__JSCTaskScheduler__markShuttingDown(vm.global());
// Reclaim queued CppTasks while JSC is still live (after teardownJSCVM the worker VM is
// dealloc'd-without-Drop so anything still in self.tasks leaks). Work-pool fs completions
// post without a shutdown check; wait for in-flight ones so the drain below sees every
// post.
vm.event_loop_mut().wait_for_concurrent_posters();
// dealloc'd-without-Drop so anything still in self.tasks leaks). Close the poster gates
// first so the drain below sees every work-pool fs completion that will ever land; a
// completion whose syscall is still blocked (pipe/FIFO with no writer) is refused at
// its gate later and frees itself without touching this VM.
Comment thread
robobun marked this conversation as resolved.
Outdated
vm.close_concurrent_posters();
vm.event_loop_mut().release_queued_tasks_for_shutdown();
if let Some(rare) = vm.rare_data.as_deref_mut() {
rare.release_js_handles();
Expand Down
Loading
Loading