Skip to content

node-api: enter env context for async callbacks - #65406

Open
codebytere wants to merge 1 commit into
nodejs:mainfrom
codebytere:napi-enter-env-context
Open

node-api: enter env context for async callbacks#65406
codebytere wants to merge 1 commit into
nodejs:mainfrom
codebytere:napi-enter-env-context

Conversation

@codebytere

Copy link
Copy Markdown
Member

AsyncResource::CallbackScope has to be opened with the resource's environment context entered; InternalCallbackScope otherwise asserts that Environment::GetCurrent(isolate) is that environment. uvimpl::Work::AfterThreadPoolWork() and the thread-safe function's DispatchOne() and Finalize() opened it with only a HandleScope, while CallFinalizer() beside them enters context() first. With one Environment per isolate that never shows, since the loop runs with that context entered. With two Environments on one isolate sharing a loop, an addon's napi_async_work completion or thread-safe function call aborts the process whenever the other Environment's context is the current one when the loop gets to it:

#  Node.js[216]: node::InternalCallbackScope::InternalCallbackScope(...) at ../../src/api/callback.cc:108
#  Assertion failed: (Environment::GetCurrent(isolate)) == (env)

This enters the node-api env's context at those three sites, the same way the zlib and WebCrypto thread pool callbacks do. In Finalize() the scope covers only the finalizer call rather than the whole function: context() returns a handle onto the env's persistent, and MaybeDelete() at the end can drop the last reference and free the env, so a function-wide scope would exit through a dead handle (test_threadsafe_function_shutdown covers that).

The node-api env's own context is used rather than node_env()->context(); they are the same today since addons only load in the principal realm.

Tests: new cctest NodeApiTest.AsyncCallbacksEnterOwnContext creates two Environments on one isolate, queues async work and a thread-safe function call from an addon in the first with the second's context entered, runs the loop, and checks that the complete callback, call_js and the thread-safe function's finalizer each ran in the addon's context; it hits the assertion above on main and passes here. cctest, node-api, js-native-api and async-hooks suites pass.


Disclosure: the code, test and this description were written by Claude Code, directed and reviewed by @codebytere.

`uvimpl::Work::AfterThreadPoolWork()` and the thread-safe function's
`DispatchOne()` and `Finalize()` opened an
`AsyncResource::CallbackScope` with only a `HandleScope`.
`InternalCallbackScope` expects the resource's environment context to
be entered and otherwise asserts that `Environment::GetCurrent(isolate)`
is that environment, so with two environments on one isolate an addon's
async work completion or thread-safe function call aborted the process
whenever the other environment's context was current when the loop ran
the callback.

Enter the node-api env's context first, as `CallFinalizer()` and the
zlib and WebCrypto thread pool callbacks already do. In `Finalize()` the
scope covers only the finalizer call, since `MaybeDelete()` can free
the env whose persistent handle `context()` returns.

Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/node-api

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. labels Aug 19, 2026
@codebytere codebytere added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 19, 2026
@codecov

codecov Bot commented Aug 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.12%. Comparing base (03fb384) to head (bea5dfb).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65406      +/-   ##
==========================================
+ Coverage   90.10%   90.12%   +0.01%     
==========================================
  Files         752      752              
  Lines      252209   252212       +3     
  Branches    47454    47449       -5     
==========================================
+ Hits       227260   227302      +42     
+ Misses      16264    16209      -55     
- Partials     8685     8701      +16     
Files with missing lines Coverage Δ
src/node_api.cc 75.33% <100.00%> (+0.09%) ⬆️

... and 39 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. request-ci Add this label to start a Jenkins CI on a PR.

Projects

Status: Need Triage

Development

Successfully merging this pull request may close these issues.

2 participants