napi: implement uv_async_t on POSIX by bridging to Bun's event loop - #35475
napi: implement uv_async_t on POSIX by bridging to Bun's event loop#35475robobun wants to merge 5 commits into
Conversation
On Linux/macOS Bun does not run a libuv loop; every uv_* symbol except a
handful of pthread/time wrappers is a panic stub. N-API addons that use
uv_async_t to call back onto the JS thread (wrtc, zeromq, serialport, ...)
die with 'unsupported uv function: uv_async_init'.
The uv_loop_t* that napi_get_uv_event_loop returns on POSIX is already
Bun's *mut EventLoop, and uv_async_send's contract ('run this callback on
the loop thread, coalesced') is exactly EventLoop::enqueue_task_concurrent.
Three small extern "C" shims in napi_body.rs expose that plus the
concurrent keep-alive refcount and the main-thread EventLoop pointer for
uv_default_loop(); uv-posix-polyfills.c owns the uv_handle_t bookkeeping
(flags, pending, close_cb) so addons see the same bits libuv sets.
Handle lifecycle matches libuv: init sets REF|ACTIVE and refs the loop;
send CAS's pending 0->1 with libuv's busy-counter guard so a concurrent
uv_close can spin it out; close is deferred and holds the loop open until
close_cb runs; dispatch runs on the loop thread via ManagedTask. uv_close /
uv_ref / uv_unref keep panicking for handle types Bun has not initialised.
Fixes #5019.
|
Warning Review limit reached
Next review available in: 16 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
|
Updated 12:10 AM PT - Jul 25th, 2026
❌ @robobun, your commit eb2d513 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35475That installs a local version of the PR into your bun-35475 --bun |
Clearing pending to 0 on the close-path dispatch reopened the window for a racing uv_async_send to schedule a second task behind close_cb, which may free the handle. Check the CLOSING flag first and leave pending non-zero on that path so at most one task ever references the handle.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
The setImmediate chain itself kept the loop alive, so a no-op Bun__uv_handle_ref would still pass. Use an unref'd timer so the uv_async_t handle is the only thing holding the process open.
- uv_close: check CLOSING before writing close_cb, matching libuv's assert-first ordering so a second close does not silently clobber the first caller's callback - uv_async_init: return UV_EINVAL when loop is NULL instead of silently succeeding with a dead handle - uv_default_loop: fall back to this thread's VM when there is no main-thread VM (the bun build macro VM is created with is_main_thread: false) - test: cover uv_close with a send already queued so the prev!=0 branch runs; byte-identical output under Node - document the Worker-teardown caveat pending env-tracked handle cleanup
There was a problem hiding this comment.
The three earlier findings are all addressed in eb2d513 and predecessors (CLOSING check now precedes the pending reset in Bun__uv_handle_dispatch; keepalive test uses an unref'd timer; subprocess tests are test.concurrent), and I found nothing new this pass. Deferring to a human because this is hand-rolled concurrent native code — atomics + spin-wait coordinating uv_async_send (any thread) with uv_close/dispatch, plus event-loop keep-alive refcount balancing across an FFI boundary — and the acknowledged Worker-teardown gap is a design tradeoff worth a maintainer's sign-off.
What was reviewed:
- Re-traced the send/close/dispatch interleaving after the CLOSING-first fix —
pendingstays non-zero throughclose_cb, so the racing-send UAF window is closed. - Ref/unref bookkeeping: init (+1), unref (−1), ref (+1), close-of-unref'd (+1), dispatch-close (−1) balance on every path I could enumerate.
uv_stub.test.tsstill consistent: the now-implemented symbols were removed from bothsymbols[]andplugin.c, and the deadsymbols_to_testinuv.test.tswas unused.
Extended reasoning...
Overview
Implements uv_async_t on POSIX by bridging to Bun's EventLoop::enqueue_task_concurrent, replacing panic stubs for uv_async_init/uv_async_send/uv_close/uv_ref/uv_unref/uv_default_loop and the handle accessors. ~200 lines of new C in uv-posix-polyfills.c (handle flags, atomic pending/u.fd busy-counter, spin-wait in uv_close), three extern "C" Rust shims in napi_body.rs (Bun__uv_handle_schedule/_ref/_default_loop), plus a native test addon and 6 subprocess tests. Generated stub files updated to drop the now-implemented symbols.
Security risks
None user-facing. The concern here is memory safety: raw uv_handle_t* stored in a concurrent task queue, cross-thread atomics coordinating with a spin-wait, and an FFI boundary where the C side treats a *mut EventLoop as an opaque uv_loop_t*. My earlier review found a UAF race (dispatch clearing pending before checking CLOSING); that's fixed. The PR description explicitly documents a remaining Worker-teardown gap (#18546) where uv_async_send after Worker::terminate() can reach a freed loop — not a regression since these calls previously panicked at init, but it's a known hole a maintainer should acknowledge.
Level of scrutiny
High. This is exactly the category REVIEW.md flags hardest: hand-written atomics with weakly-justified orderings (relaxed load in uv_async_send's fast path, seq_cst elsewhere), a busy-spin (bun__uv_async_spin) copied from libuv semantics, refcount balancing across four entry points, and native callbacks (async_cb/close_cb) that re-enter JS from a ManagedTask. Previously this whole surface was an immediate panic, so any bug here is net new. The test coverage is good (both loop sources, both send origins, close-while-pending, keepalive) but can't exercise the tight races the spin-wait guards.
Other factors
All three of my prior findings were fixed in follow-up commits and verified against the current diff. The symbols_to_test deletion from uv.test.ts was dead code (only uv_stub.test.ts consumes it). CI build #79860 was still running when I reviewed. Given the concurrency subtlety and the design decision to ship with the Worker-teardown limitation, this warrants a maintainer's eyes rather than a bot approval.
|
CI build #79860:
None touch the napi/uv surface this diff changes. |
|
#39652 implements uv_async_t together with uv_queue_work, uv_default_loop and the header-only helpers, with a per-VM loop object in place of the EventLoop pointer. It covers what this PR does, so this one can be closed once that lands. |
|
Closing in favor of #39652, which now carries this work. #39652 implements the same uv_async_t functions (all 13 symbols this PR un-stubs are in its list), and also uv_queue_work, uv_cancel and the header-only helpers. It uses a per-VM loop object in place of the EventLoop pointer. Its tests cover every case in this PR: both loops, a send from another thread, close with a send pending, the ref and unref keep-alive behavior, and the handle accessors. This branch is also in conflict with main. Nothing here needs to move over. |
1 participant
What
On Linux/macOS Bun does not run a libuv loop; every
uv_*symbol except a handful of pthread/time wrappers is a panic stub. N-API addons that useuv_async_tto call back onto the JS thread die with:This is the
wrtc,zeromq,@serialport/bindings-cpppattern: work happens on an addon thread anduv_async_sendbounces the completion back to JS. It is also the one loop-coupled libuv primitive that maps cleanly onto machinery Bun already has.How
The
uv_loop_t*thatnapi_get_uv_event_loophands out on POSIX is already Bun's*mut EventLoop(seenapi_get_uv_event_loopinsrc/runtime/napi/napi_body.rs), anduv_async_send's contract ("run this callback on the loop thread, coalesced, thread-safe") is exactlyEventLoop::enqueue_task_concurrent, the same pathnapi_threadsafe_functionuses.Three
extern "C"shims innapi_body.rsexpose what C cannot reach: enqueue aManagedTaskonto the loop, bump the loop's concurrent keep-alive refcount, and return the main-threadEventLoop*foruv_default_loop(). Everything the addon can observe on theuv_handle_titself (flags,type,pending,close_cb, theu.fdbusy counter) lives inuv-posix-polyfills.cso the struct bits match what libuv would have written.Handle lifecycle matches libuv's (semantics taken from
src/unix/async.c/src/uv-common.hat the header commit insrc/jsc/bindings/libuv/README.md):uv_async_initsetsREF|ACTIVEand refs the loop;datais left untouched. ReturnsUV_EINVALwhenloopis NULL.uv_async_sendcoalesces via atomic exchange onpendingand carries libuv'su.fdbusy counter so a concurrentuv_closecan spin out in-flight sends.CLOSINGfirst (so a racing send cannot schedule behindclose_cb), then resetspendingbefore callingasync_cbso a send inside the callback schedules again.uv_closeasserts!uv__is_closingbefore any field write, is deferred, and keeps the loop alive untilclose_cbreturns; if the handle was unref'd it takes a ref back for the duration so the process cannot exit with a close callback outstanding.uv_ref/uv_unref/uv_closekeep panicking for handle types Bun has not initialised, so the failure mode for everything else is unchanged.Also implemented:
uv_default_loop,uv_has_ref,uv_is_active,uv_is_closing,uv_handle_get_data/set_data/get_loop/get_type(pure field accessors addons reach for alongside async handles).Worker teardown
A
uv_async_tinitialised inside a Worker stores that Worker's*mut EventLoop. UnlikeThreadSafeFunctionthere is no env-teardown hook yet to neutralise it when the Worker terminates (#18546 tracking the broader handle surface), so an addon thread that keeps callinguv_async_sendpastWorker::terminate()without the usualuv_close+ join in a cleanup hook can reach a freed loop. Before this PR such code panicked atuv_async_initinstead, so this is not a regression for any working addon; handles on the main thread's loop are unaffected.Tests
test/napi/uv-stub-stuff/uv_impl.cgainstestUvAsync/testUvAsyncClosePendingexports that driveuv_async_init/uv_async_send/uv_ref/uv_unref/uv_close/uv_is_closing/ the handle accessors, with sends both from the loop thread and from apthread, with the loop obtained from bothnapi_get_uv_event_loopanduv_default_loop(), and with a close issued while a send is still queued so the "queued task takes the close path" branch runs. A separate keep-alive test initialises a ref'd handle and waits on an unref'd timer so the handle's loop ref is the only thing keeping the process alive.Every case produces byte-identical output under Node.js.
USE_SYSTEM_BUN=1 bun test test/napi/uv.test.ts: 6 pass, 6 fail (panic: unsupported uv function: uv_async_init/uv_default_loop)bun bd test test/napi/uv.test.ts: 12 passFixes #5019. Towards #18546.
Related open PRs #31696 / #34155 / #29261 implement the loop-free libuv subset and explicitly leave
uv_async_*for a loop-integrated change; this is that change for the async handle family and does not overlap those diffs beyond the shared generated-stubs files.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/napi/uv.test.ts