napi: keep dispatching threadsafe function calls when a callback blocks in a nested event loop - #36831
napi: keep dispatching threadsafe function calls when a callback blocks in a nested event loop#36831robobun wants to merge 17 commits into
Conversation
…ks in a nested event loop A threadsafe function's dispatch loop coalesces pushes: while the loop is marked Running, a push only flips the state to Pending and relies on the running loop to pick the item up. That breaks when the dispatched callback (or a microtask drained between two batched callbacks) re-enters the event loop and blocks until a promise settles, as bun:test's expect(promise).resolves / .rejects does via wait_for_promise: the promise can only settle through further threadsafe function calls, but those calls are coalesced into the blocked loop and never dispatched, deadlocking the process. Fix: - schedule_dispatch also enqueues a dispatch task when the previous state was Running; if the running loop is live it drains the item first and the extra task is a cheap no-op, and if it is blocked in a nested event loop the task is what keeps the function draining. - dispatch_one schedules one backup dispatch per on_dispatch when items remain queued behind the one being called, so queued-behind items cannot be stranded either. - on_dispatch tracks in-flight dispatch tasks and its own re-entrancy depth, and only frees a Closed threadsafe function from the last task at the outermost frame, since more than one task can now reference it. Fixes #36828
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe runtime now tracks nested ThreadSafeFunction nested dispatch
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/napi/napi_body.rs (1)
2653-2711: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCall
schedule_dispatch()while holdingself.lock.
dispatch_onedrops the lock before the backup call, butschedule_dispatch()requires the lock while readingevent_loop. This can race withenv_teardown, which setsevent_looptoNoneunder the same lock. Move the backup-dispatch decision into the locked block.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/napi/napi_body.rs` around lines 2653 - 2711, Move the backup-dispatch decision and self.schedule_dispatch() call into the self.lock guard block in dispatch_one, after determining that remaining > 0 and scheduled_backup is false. Update scheduled_backup while holding the lock, and remove the unlocked scheduling block so event_loop cannot race with env_teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/napi/napi_body.rs`:
- Around line 2634-2643: In the finalizer-task scheduling block, move the
inflight_dispatch_tasks fetch_add before
loop_.enqueue_task(Task::init(self_ptr)). Preserve the existing loop teardown
guard and task initialization, ensuring the task is counted before it is
published to the event loop.
---
Outside diff comments:
In `@src/runtime/napi/napi_body.rs`:
- Around line 2653-2711: Move the backup-dispatch decision and
self.schedule_dispatch() call into the self.lock guard block in dispatch_one,
after determining that remaining > 0 and scheduled_backup is false. Update
scheduled_backup while holding the lock, and remove the unlocked scheduling
block so event_loop cannot race with env_teardown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 461fe26e-ac08-4cfe-b108-242227639c05
📒 Files selected for processing (5)
src/runtime/napi/napi_body.rstest/napi/napi-app/binding.gyptest/napi/napi-app/tsfn-nested-wait.fixture.tstest/napi/napi-app/tsfn_nested_wait_addon.cpptest/napi/tsfn-nested-wait.test.ts
…ion lock schedule_dispatch documents that callers must hold the lock (it reads event_loop, which env_teardown clears under the same lock), and every other caller does. Move the backup-dispatch decision in dispatch_one inside the guard instead of after it drops.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Addressed the review in 969b7eb: the backup dispatch in dispatch_one is now scheduled inside the lock guard, honoring schedule_dispatch's documented lock contract (event_loop is cleared by env_teardown under the same lock). The finalizer-task counter ordering is unchanged; both statements run on the JS thread and the queue is only drained by that thread, so the task cannot be observed before it is counted (replied on the thread). |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/napi/napi_body.rs`:
- Around line 2685-2698: Remove the pre-callback backup scheduling from the
dispatch path around on_dispatch, and avoid holding or materializing &mut
ThreadSafeFunction across self.call() or any JavaScript entry. Retain only a raw
*mut Self while invoking the callback, then reacquire a short-lived mutable
reference after it returns, revalidate the function state, and perform any
required backup scheduling without permitting on_dispatch re-entry during the
original mutable borrow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ad9aab57-7572-4c97-9ff2-a70c7aee80de
📒 Files selected for processing (1)
src/runtime/napi/napi_body.rs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/runtime/napi/napi_body.rs:2696-2697— Nit:schedule_dispatch's doc comment (line 2824) still says "Caller must holdlock", but this new backup-dispatch call runs after_gdrops at the end of the'brkblock. It's safe —dispatch_oneandenv_teardownboth run on the JS thread so theevent_loopread can't race — but the comment now states an invariant a caller violates, which will send future readers hunting for a bug here. Consider updating the comment to note JS-thread callers are exempt.Extended reasoning...
What
schedule_dispatchatnapi_body.rs:2824is documented as:Caller must hold
lock. Reached from addon threads (enqueue,release_locked), so it may only take a shared&EventLoop: the JS thread can be insidetick()with its own&mutat the same time.This PR adds a new caller in
dispatch_oneat line 2697 — the backup dispatch for the nested-event-loop case — which invokesself.schedule_dispatch()after the lock guard has been dropped.Step-by-step
dispatch_oneenters the'brkblock and takes the lock at line 2658:let _g = self.lock.lock_guard();.- The block computes
(task, remaining)andbreak 'brks at line 2685. - The
'brkblock closes at line 2686, dropping_gand releasing the lock. - Control reaches line 2688–2697, where
remaining > 0 && !*scheduled_backupcallsself.schedule_dispatch()— with the lock not held.
So the doc comment's stated precondition is violated by a caller this PR introduces.
Why the code is still correct
The lock requirement exists to serialize
schedule_dispatch's read ofself.event_loopagainstenv_teardownclearing it (per the field comment at ~line 2427: "Written underlockbyenv_teardownon the JS thread"). Addon-thread callers (enqueue,release_locked) genuinely need the lock for that.dispatch_one, however, runs only on the JS thread (viaon_dispatch), andenv_teardownalso runs only on the JS thread, so they cannot execute concurrently and no race onevent_loopis possible. The other fieldsschedule_dispatchtouches (dispatch_state,inflight_dispatch_tasks) are atomic. So there's no functional bug.Why it's worth flagging
Per the repo review guidelines ("Only comment what the code cannot say"), an invariant comment that a caller visibly violates is a maintenance hazard in two directions: a future reader will either (a) flag line 2697 as a locking bug and "fix" it by moving the call back under
_g(harmless but pointless), or worse (b) conclude the lock requirement is vestigial and drop it for the addon-thread callers too — which would introduce a real race withenv_teardown.Suggested fix
Update the doc comment on
schedule_dispatchto something like:Addon-thread callers (
enqueue,release_locked) must holdlockso theevent_loopread is serialized againstenv_teardownclearing it. JS-thread callers (dispatch_one's backup dispatch) don't need the lock —env_teardownis JS-thread-only and cannot race.Nit severity — no functional bug, just a doc/code mismatch introduced by the PR.
dispatch_one and call now take *mut Self and form only scoped reborrows that end before any user JS runs (the callback, the microtask drain), since a nested event loop can re-enter on_dispatch for the same function. No &mut ThreadSafeFunction is live across a JS entry anymore.
|
source-lints failing |
|
Fixed in the latest push: the fn-long |
…item The between-callbacks microtask drain ran after the next item was already popped, so a drain that blocked in a nested event loop stranded that item: later items were delivered out of push order, and with nothing left in the queue the nested wait could never settle. Move the drain ahead of the dequeue in dispatch_one so a blocked drain leaves the item reachable for a nested dispatch, in FIFO order. call no longer enters JS before the callback and loses its is_first/Result plumbing. Covers the ordering with a new fixture case that blocks in a microtask between two queued calls.
A push landing in the unbacked Pending window between drain-loop iterations coalesced with no task queued; if the next iteration's microtask drain then blocked, the item was unreachable. Enqueue whenever no dispatch task is in flight, regardless of dispatch state, so every push is backed by a task or by one already queued. Also drop the stale caller list on loop_mut.
There was a problem hiding this comment.
Beyond the inline nit: re-traced my prior Pending-window scenario against 69712df's inflight-only gate in schedule_dispatch — the push at step 4 now sees inflight==0 under lock and enqueues, so that ordering is closed. Also checked that maybe_queue_finalizer's new inflight increment is balanced on the env-torn-down early return and by on_dispatch's unconditional decrement. The gate evidence showing the test passing on release-without-fix was examined — the coalescing is timing-dependent and only the debug/ASAN lane hits it deterministically.
Extended reasoning...
d7cb10b is a doc-only follow-up to my last nit; the substantive logic hasn't changed since 69712df. This is still a concurrency-heavy change to TSFN dispatch (atomics across addon/JS threads, re-entrant on_dispatch, deferred-destroy accounting via dispatch_depth/pending_destroy/inflight_dispatch_tasks) that has needed three correction rounds to converge — a maintainer should be the one to sign off. Recording what this pass covered so it isn't re-derived.
There was a problem hiding this comment.
No new findings this pass — all prior inline comments (aliasing UB in the re-entrant dispatch, FIFO reordering across the microtask drain, the unbacked-Pending window, stale doc comments, and the mtime rebuild gate) have been addressed. Deferring to a human because this rewrites the core TSFN dispatch loop for re-entrancy: raw-pointer access across nested on_dispatch frames, deferred destroy gated on dispatch_depth + inflight_dispatch_tasks, and the schedule_dispatch gate now keyed solely on the atomic in-flight count under lock.
Checked this round: schedule_dispatch is only reached while lock is held (enqueue, release_locked, take_one_locked), so the load-then-fetch_add on inflight_dispatch_tasks cannot race another scheduler; the env_dead early return in on_dispatch skips the deferred-destroy path but env_teardown owns the free there; and maybe_queue_finalizer's enqueue_task is now counted in inflight_dispatch_tasks so a Closed check in a later task defers correctly.
Extended reasoning...
Overview
The PR reworks ThreadSafeFunction dispatch in src/runtime/napi/napi_body.rs so queued calls keep draining when a callback (or the between-callbacks microtask drain) blocks in a nested event loop — the #36828 deadlock. It adds three fields (dispatch_depth, pending_destroy, inflight_dispatch_tasks), converts dispatch_one/call to raw-*mut Self so no &mut spans re-entrant user JS, moves the node#38506 microtask drain ahead of the dequeue, extracts the locked dequeue into take_one_locked (which schedules a backup dispatch when items remain and none is in flight), and rewrites schedule_dispatch to gate purely on inflight_dispatch_tasks under lock. A new native addon target plus a spawned bun test fixture cover the callback-blocks, queued-behind, and microtask-blocks orderings.
Security risks
None user-facing. The risk surface is memory safety and cross-thread lifetime: multiple queued concurrent tasks now reference the same heap allocation, and destroy is deferred until the outermost frame with zero in-flight tasks. The PR description notes ASAN caught a UAF in an earlier revision without the deferral; the current diff's pending_destroy + dispatch_depth == 0 && inflight == 0 gate is the mechanism that closes it. The env_dead early-return path intentionally does not free — env_teardown owns that.
Level of scrutiny
High. This is exactly the REVIEW.md "most-blocked category": raw-pointer re-entrancy, refcount-like balancing across every terminal path, and an atomic shared with addon threads. The change went through several rounds here already (aliasing UB, dropped-item across nested finalize, FIFO reorder, unbacked-Pending race), each of which the author fixed. That iteration history and the concurrency subtlety are why a maintainer should read the final shape rather than relying on automated review alone.
Other factors
All prior inline findings from earlier passes are resolved in the current diff. The evidence block shows the new test deadlocks on an ASAN debug build without the fix and passes with it (release-without-fix passing is expected — the bug is timing-dependent). The existing test/napi/napi.test.ts -t threadsafe suite was re-run per the description. The test harness follows the sibling napi.test.ts conventions after the mtime-gate follow-up. Nothing outstanding from human reviewers beyond Jarred's "plz fix" requests, all of which were actioned.
There was a problem hiding this comment.
No bugs found this pass, but this reworks the TSFN dispatch/destroy state machine (raw *mut Self re-entrancy, three new lifetime-tracking fields, atomics gating destroy) — worth a maintainer's eyes before merge.
Checked: inflight_dispatch_tasks is balanced on every enqueue path (schedule_dispatch, maybe_queue_finalizer's add/sub-on-None, on_dispatch's fetch_sub); destroy is deferred correctly when a nested frame observes Closed (traced the finalize-task-during-drain ordering); the schedule_dispatch gate change closes the unbacked-Pending window without unbounded task growth (extra tasks drain as no-ops); AsyncTaskTracker is Copy so the copied-out (*this).tracker in call() is equivalent to the old self.tracker. Earlier findings (FIFO reorder, drain-before-backup gap, stale caller lists, mtime rebuild gate) are all addressed in the current diff.
Extended reasoning...
Overview
The PR changes ~180 lines in src/runtime/napi/napi_body.rs to fix #36828: threadsafe function calls deadlock when a callback (or the between-callbacks microtask drain) blocks in a nested event loop under bun test. It adds three fields to ThreadSafeFunction (dispatch_depth, pending_destroy, inflight_dispatch_tasks), converts dispatch_one/call from &mut self to raw-*mut Self associated fns to permit re-entrancy, moves the microtask drain ahead of the dequeue, rewrites schedule_dispatch to gate solely on the in-flight task count, and defers destroy until the outermost frame with zero in-flight tasks. New addon target + fixture cover three orderings (concurrent push, queued-behind, microtask-between).
Security risks
None identified. This is internal event-loop scheduling; no user input parsing, no auth/crypto/permissions surface.
Level of scrutiny
High. Per REVIEW.md this is squarely in the most-blocked category: native memory safety with manual free (destroy/free_orphaned), reference-count-like balancing on every terminal path (inflight_dispatch_tasks), thread-affinity rules (addon threads vs JS thread, lock-held vs not), and re-entrancy through user JS. The correctness of "whoever is last out frees it" now depends on the interaction of five pieces of state (dispatch_state, closing, dispatch_depth, pending_destroy, inflight_dispatch_tasks) across nested frames and concurrent addon-thread pushes. Three earlier review passes on this PR each found real issues (FIFO ordering violation, unbacked-Pending race after the drain move, stale doc invariants), which is a signal the state space is subtle enough to warrant a human owner's read of the final shape.
Other factors
- I traced the destroy-deferral through several orderings (nested frame sees Closed with depth>0 → sets
pending_destroy; outer frame frees at depth→0 with inflight==0; multiple queued tasks after Closed each defer until the last) and did not find a UAF or double-free. schedule_dispatchis serialized underlockandinflight_dispatch_tasksis atomic; the load-then-add there is not a TOCTOU because all callers holdlock. The unlockedfetch_subat the top ofon_dispatchcan race with the load, but the outcome is at most one extra no-op task, not a lost wakeup.maybe_queue_finalizernow incrementsinflightbeforeloop_mut()and compensates with afetch_subon the env-torn-down path — balanced.- The
call()refactor copies the callback target into a localTargetenum before invoking JS so no&*thisborrow spans the user call;trackeris#[derive(Copy)](just a u64 id) so the by-value copy is behavior-preserving. - Tests: the new fixture is spawned as a subprocess with pipes drained concurrently, uses
it.skipIf(!canBuildNodeAddons()), and the mtime-gated rebuild matches the siblingnapi.test.tsconvention. All prior inline findings from this reviewer are resolved in the current head.
Given the complexity and that Jarred has been actively steering this PR, deferring rather than auto-approving.
3 participants
Fixes #36828
Problem
A promise that can only settle through napi threadsafe function calls never settles under
bun teston 1.4 canary (works on 1.3.13 and underbun run). Reported with@temporalio/worker:native.workerPollWorkflowActivation()/native.workerPollActivityTask()promises never resolve, the worker never leaves DRAINING, and teardown then fails witherror: Channel has been shut down.Cause
The threadsafe function dispatch loop coalesces pushes: while
dispatch_stateis Running,napi_call_threadsafe_functiononly flips it to Pending and relies on the runningon_dispatchloop to pick the item up, without enqueueing a new event-loop task.bun:test's
expect(promise).resolves/.rejectswaits for the promise synchronously by running a nested event loop (Expect::process_promise->wait_for_promise). When that wait runs inside a threadsafe function dispatch (directly in the callback, or in a microtask drained between two batched callbacks, which is where the temporal repro blocks), the dispatch loop is parked underneath the wait. Every subsequent push coalesces into that blocked loop and is never dispatched, so the promise the nested loop is waiting on can never settle: deadlock. The main thread parks inepoll_waitforever, which is why the hung process also ignores SIGTERM.Captured stack of the hang (test runner -> TSFN dispatch -> microtask ->
expect().rejects.toThrow()->wait_for_promise->auto_tickparked in epoll):The coalescing design predates 1.4; the regression window only shifted timing so the two neon channel sends land in one dispatch batch deterministically. The deadlock is latent in any ordering where a callback blocks in a nested wait.
Fix
In
src/runtime/napi/napi_body.rs:schedule_dispatchalso enqueues a dispatch task when the previous state was Running. If the running loop is live it drains the item first and the extra task is a cheap no-op; if it is blocked in a nested event loop, the task is what keeps the function draining.dispatch_oneschedules one backup dispatch peron_dispatchwhen items remain queued behind the one being called, so queued-behind items cannot be stranded when that call blocks.on_dispatchtracks in-flight dispatch tasks and its own re-entrancy depth, and only frees a Closed threadsafe function from the last task at the outermost frame, since more than one task can now reference it. (Without this, a second queued task dereferences the freed function; ASAN catches it in the existingtest_create_tsfn_with_async_contexttest.)Test
New addon target
tsfn_nested_wait_addonplus abun testfixture covering both orders: a call pushed while the callback is already blocked in a nested wait, and a call already queued behind the blocking one. Both deadlock and time out without the fix, and pass in under a second with it.Verification
bun bd test test/napi/tsfn-nested-wait.test.ts: pass with fix, times out without (git stash push -- src/).worker.runUntil+expect().rejects.toThrow()) now passes underbun bd test:1 pass, 0 failin ~90s on a debug build; unfixed it hangs past a 300s test timeout.bun bd test test/napi/napi.test.ts -t threadsafe: same results as unmodified main in this environment (one pre-existing 5s timeout on a slow debug container, identical on main).[review] gate passed · iteration 3 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 9 passed · 0 rejected · iteration 3
evidence per changed file
root cause · written by the author bot
When JavaScript re-entered the event loop from inside a ThreadSafeFunction callback, such as by synchronously waiting on a promise during a nested dispatch, the dispatcher's single in-flight task was blocked and newly queued callbacks were never scheduled, so the queue stalled and NAPI promises never settled. The fix makes the ThreadSafeFunction track dispatch depth and in-flight dispatch tasks, scheduling backup dispatches so work queued during a nested wait still drains in FIFO order, and it defers destruction until all nested dispatches complete to keep teardown safe. Regression coverage…