Skip to content
27 changes: 23 additions & 4 deletions src/jsc/VmHandle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,14 @@ impl VmHandle {
self.0.state() == State::Open
}

/// Is the wait over? Past it nothing started for the VM runs any more:
/// [`VirtualMachine::ticket`] panics and a queued task is only released.
/// Any thread; meaningful on the JS thread (a final collection's finalizer).
Comment thread
robobun marked this conversation as resolved.
#[inline]
pub fn closed(&self) -> bool {
self.0.state() == State::Closed
}

pub(crate) fn tickets_outstanding(&self) -> u32 {
self.0.tickets.load(Ordering::SeqCst)
}
Expand Down Expand Up @@ -503,19 +511,30 @@ impl VmHandle {
impl VirtualMachine {
/// JS thread: a ticket for work about to leave this thread — this VM, and
/// the loop it is currently ticking. Hold it in the in-flight operation
/// and drop it after the completion is posted. Infallible until the wait
/// has finished (after which nothing on this thread starts off-thread work).
/// and drop it after the completion is posted.
///
/// Panics, in release too, once the VM is [`closed`](Self::closed): the
/// wait is over, so nothing would wait for this ticket and its completion
/// would land on a loop teardown is freeing. Callers that can run that late
/// (finalizers) check `closed()` and refuse the work; the panic names one
/// that did not.
Comment thread
robobun marked this conversation as resolved.
#[track_caller]
#[inline]
pub fn ticket(&self) -> Ticket {
let h = self.handle_ref();
h.assert_js_thread();
debug_assert!(
h.0.state() != State::Closed,
assert!(
!h.closed(),
"off-thread work started after the VM finished draining"
);
Ticket::issue(&h.0, self.current_loop_kind())
}

/// [`VmHandle::closed`] for this VM.
#[inline]
pub fn closed(&self) -> bool {
self.handle_ref().closed()
}
}

// ── Test suite only: deterministic late completions ───────────────────────
Expand Down
7 changes: 4 additions & 3 deletions src/runtime/api/JSTranspiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -651,7 +651,8 @@ impl Config {
// This is going to be hard to not leak
/// `transpiler.transform()` off the JS thread. The parse/print state points
/// into the owning `JSTranspiler`'s config (its `Transpiler` is bit-copied),
/// which the job's Js side keeps alive and the pool borrow keeps valid.
/// which the `Strong` on the job's Js side keeps alive until the job is back
/// on the JS thread; `run` reads it while the job's ticket keeps the VM alive.
Comment thread
robobun marked this conversation as resolved.
pub(crate) struct TransformTask {
pub input_code: bun_jsc::ThreadSafe<StringOrBuffer>,
pub output_code: BunString,
Expand All @@ -663,8 +664,8 @@ pub(crate) struct TransformTask {
pub loader: Loader,
pub replace_exports: bun_ast::runtime::ReplaceableExportMap,
}
// SAFETY: see the type doc — VM-owned config is read only under the pool
// borrow; everything else is owned.
// SAFETY: see the type doc — the wrapper's config (behind `transpiler` and
// `tsconfig`) is read only in `run`, under the job's ticket; the rest is owned.
unsafe impl Send for TransformTask {}

#[derive(bun_jsc::JsAffine)]
Expand Down
6 changes: 4 additions & 2 deletions src/runtime/image/Image.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1398,8 +1398,10 @@ pub struct PipelineTask {
auto_orient: bool,
result: TaskResult,
}
// SAFETY: `input` borrows bytes that are pinned (`Pin`) or owned by the Image
// the job's Js side keeps alive; read only under the pool borrow. The rest is owned.
// SAFETY: `input` borrows bytes pinned by `Pin` or, like its path, owned by the
// Image `PendingTask` holds; both sit on the job's Js side, dropped there only
// after the pool has posted this task back, and the pool reads them in `run`,
// under the job's ticket. The rest is owned.
unsafe impl Send for PipelineTask {}

/// The JS-thread half of a scheduled `PipelineTask`.
Expand Down
41 changes: 34 additions & 7 deletions src/runtime/napi/napi_body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,10 @@
// `Taskable` impls for the napi heap tasks dispatched through the JS event loop.
impl Taskable for napi_async_work {
const TAG: TaskTag = task_tag::NapiAsyncWork;
/// Work the pool handed back during teardown: its `complete` callback is
/// how the addon learns the outcome and frees the work (Node calls it from
/// environment cleanup too); script it tries to run is refused at the boundary.
/// Work handed back during teardown (by the pool, or by `schedule` itself
/// once script was forbidden): its `complete` callback is how the addon
/// learns the outcome and frees the work (Node calls it from environment
/// cleanup too); script it tries to run is refused at the boundary.
Comment thread
robobun marked this conversation as resolved.
unsafe fn release_unrun(this: *mut Self) {
let vm = VirtualMachine::get().as_mut();
let global = vm.global();
Expand Down Expand Up @@ -143,6 +144,12 @@
Self::set_last_error(Some(self), NapiStatus::pending_exception)
}

/// Node's status for a call the environment can no longer serve because it
/// is shutting down.
Comment thread
robobun marked this conversation as resolved.
pub(crate) fn cannot_run_js(&self) -> napi_status {
Self::set_last_error(Some(self), NapiStatus::cannot_run_js)
}

/// Checks both `env->m_pendingException` (set by `napi_throw*`) and the JSC
/// VM exception slot. This is the gate Node.js's `NAPI_PREAMBLE` enforces.
pub(crate) fn has_pending_exception(&self) -> bool {
Expand Down Expand Up @@ -1851,17 +1858,35 @@
drop(unsafe { bun_core::heap::take(this) });
}

pub(crate) fn schedule(&mut self) {
/// `false`: refused untouched, the work stays the addon's to delete.
pub(crate) fn schedule(&mut self) -> bool {
if self.scheduled {
return;
return true;
}
let vm = VirtualMachine::get();
if vm.closed() {
// A finalizer of the collection destroying the heap: `vm.ticket()`
// would panic, and the closed queue would run `complete` right
// here, mid-collection.
Comment thread
robobun marked this conversation as resolved.
Outdated
return false;
}
self.scheduled = true;
if !vm.script_allowed() {
// The pool would only hand it back cancelled; skip the pool and the
// ticket. The queue runs or releases it, so `complete` still gets
// `napi_cancelled`, possibly inside this call (a queue teardown has
// already drained releases on the spot): last use of `self`.
Comment thread
robobun marked this conversation as resolved.
Outdated
let _ = self.cancel();
vm.event_loop_mut().enqueue_task(Task::init(self));
return true;

Check failure on line 1881 in src/runtime/napi/napi_body.rs

View check run for this annotation

Claude / Claude Code Review

schedule() holds &mut self across enqueue_task, which can synchronously free the work

`schedule(&mut self)` holds a live `&mut self` function parameter across `enqueue_task(Task::init(self))`, which — once `closed_for_tasks` is set — synchronously runs `release_unrun` → the addon's `complete` → `napi_delete_async_work` → `heap::take` → dealloc, freeing `*self` before `schedule()` returns. Deallocating the referent of a function-argument `&mut` is UB under Stacked/Tree Borrows (the argument carries a protector for the whole call; this backs rustc's `dereferenceable`/`noalias`), an
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
self.poll_ref.ref_(bun_io::js_vm_ctx());
// The work object belongs to the addon and `execute` receives this
// env, so the VM waits for it (Node likewise settles its threadpool
// requests before an environment is freed).
self.ticket = Some(self.global.bun_vm().ticket());
self.ticket = Some(vm.ticket());
WorkPool::schedule(&raw mut self.task);
true
}

pub(crate) unsafe fn run_from_thread_pool(task: *mut WorkPoolTask) {
Expand Down Expand Up @@ -2227,7 +2252,9 @@
return env.invalid_arg();
};
debug_assert!(core::ptr::eq(env.to_js(), work.global.as_ptr()));
work.schedule();
if !work.schedule() {
return env.cannot_run_js();
}
env.ok()
}

Expand Down
3 changes: 2 additions & 1 deletion src/runtime/webcore/CompressionStreamCoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,8 @@ pub(crate) enum AsyncInput {
Owned(Vec<u8>),
}
// SAFETY: `Pinned.ptr` is a backing store pinned + protected by the paired
// `PinnedChunk` for as long as the job lives; read only under the pool borrow.
// `PinnedChunk` on the job's Js side, dropped there only after the pool has
// posted the job back; the pool reads it in `run`, under the job's ticket.
Comment thread
robobun marked this conversation as resolved.
unsafe impl Send for AsyncInput {}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/// The pin + GC protection on a chunk whose bytes went to the pool; released
Expand Down
110 changes: 110 additions & 0 deletions test/js/web/workers/worker-late-completion-napi-fixture.c

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

Loading
Loading