fix(ext/napi): run async work execute callback on a worker thread#32560
Conversation
Per the NAPI spec, `napi_queue_async_work`'s `execute` callback must run on a worker thread while `complete` runs on the main thread. Previously both ran on the main thread via `env.add_async_work()`, which caused deadlocks when `execute` called threadsafe functions (e.g. lmdb-js async transactions). - Spawn `execute` on `std::thread::spawn`, dispatch `complete` back to main thread via `async_work_sender.spawn()` - Use `ExternalOpsTracker` to keep event loop alive during execution - Fix `uv_async_send` to dispatch directly to main thread instead of going through `napi_queue_async_work` - Add test exercising threadsafe function call from execute callback Closes denoland#23268 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note to self: after this gets released open a PR to lmdb repo to update README to note that transactions work correctly in Deno now |
kajukitli
left a comment
There was a problem hiding this comment.
lgtm — this is a critical fix for NAPI compatibility
the change is correct per NAPI spec:
executeruns on worker thread viastd::thread::spawncompletedispatched to main thread viaasync_work_sender.spawn()ExternalOpsTrackerref/unref keeps event loop alive during worker execution
the uv_async_send fix is also correct — those callbacks need V8 access so they must run on main thread directly
the test case exercising tsfn-from-execute is exactly the pattern that was deadlocking. closes the lmdb-js issue
kajukitli
left a comment
There was a problem hiding this comment.
The main change looks good - moving execute to a worker thread fixes the deadlock when calling threadsafe functions from within execute.
One observation: uv_async_send now spawns directly via async_work_sender.spawn() without the ref_op()/unref_op() pattern that napi_queue_async_work uses. This means if uv_async_send is called when nothing else keeps the event loop alive, the callback could theoretically be dropped.
However, this is likely a pre-existing limitation (the old add_async_work also didn't ref-count), and the typical usage pattern has other things keeping the loop alive. The PR correctly keeps uv_async_send callbacks on the main thread since they need V8 access.
The deadlock fix is the important part and the tests validate it well.
Summary
napi_queue_async_workto run theexecutecallback on a workerthread (via
std::thread::spawn) instead of the V8 main threadcompleteback to the main thread viaasync_work_sender.spawn()where it can safely access V8ExternalOpsTracker(ref_op/unref_op) to keep the event loopalive while the worker thread is running
uv_async_sendto dispatch directly to the main thread instead ofgoing through
napi_queue_async_work(since uv_async callbacks need V8access)
executecallback —the exact pattern that caused the deadlock
Background
Per the NAPI spec,
napi_queue_async_work'sexecutecallback must run on aworker thread, while
completeruns on the main thread. Previously both ran onthe main thread via
env.add_async_work()→async_work_sender.spawn(). Thiscaused deadlocks when native addons (like lmdb-js) called
napi_call_threadsafe_functionfromexecute— the threadsafe function queueswork to the main thread and waits, but the main thread is blocked running
execute.Closes #23268
Test plan
cargo test napi— all NAPI tests pass (49 passed, 0 failed, 3 ignored)test_async_work_with_tsfnexercises the tsfn-from-executepattern
transaction()all pass (previously segfaulted with exit code 139)🤖 Generated with Claude Code