src: use `Global` for storing resource in Node-API callback scope by addaleax · Pull Request #59828 · nodejs/node

addaleax · 2025-09-09T16:47:09Z

src: use `Global` for storing resource in Node-API callback scope

This is a follow-up to 234c26c. The Node-API interface does
not allow us to enforce that values are stored in a specific location,
e.g. on the stack or not; however, V8 requires Local<> handles
to be stored on the stack.

To circumvent this restriction, we add the ability to the async handle
stack to store either Local<>* pointers or Global<>* pointers, with
Node-API making use of the latter.

src: always use strong reference to `napi_async_context` resource

There already is an existing requirement to have matching calls of
napi_async_init() and napi_async_destroy(), so expecting users
of this API to manually hold onto the resource for the duration of
the napi_async_context's lifetime is unnecessary.

Weak callbacks are generally useful for when a corresponding C++
object should be cleaned up when a JS object is gargbage-collected,
but that is not the case here.

nodejs-github-bot · 2025-09-09T16:47:14Z

Review requested:

@nodejs/node-api

legendecas

The resource object used in the napi_callback_scope is already saved with a Global though:

node/src/node_api.cc

Line 636 in 234c26c

v8::Global<v8::Object> resource_;

It is a weak ref if user provided the object, and replace the ref-ed value with a plain object (strong-ref) if the user provided object is GC-ed. We can enforce the requirement that napi_async_context must be destroyed with napi_async_destroy so that this global can be a strong one.

addaleax · 2025-09-09T18:00:27Z

@legendecas Yeah, I was confused by this quite a bit and maybe you can help clarify things here. Why are we initially only storing the reference as a weak one, rather than a strong one? I didn't want to touch that logic since whoever put it there must have done so for a good reason (i.e. that this is an example of Chesterton's fence), but I couldn't think of this as anything other than it being a bug.

We can enforce the requirement that napi_async_context must be destroyed with napi_async_destroy so that this global can be a strong one.

And how else would the napi_async_context object be destroyed if not through napi_async_destroy? If there is indeed another way, wouldn't adding this requirement be considered API breakage?

codecov · 2025-09-10T17:32:11Z

Codecov Report

❌ Patch coverage is 73.58491% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.28%. Comparing base (f6ea5bf) to head (24cde9f).
⚠️ Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
src/env-inl.h	27.27%	5 Missing and 3 partials ⚠️
src/env.cc	57.14%	2 Missing and 1 partial ⚠️
src/node_api.cc	86.66%	0 Missing and 2 partials ⚠️
src/api/callback.cc	94.73%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #59828   +/-   ##
=======================================
  Coverage   88.28%   88.28%           
=======================================
  Files         702      702           
  Lines      206995   206987    -8     
  Branches    39833    39831    -2     
=======================================
- Hits       182740   182738    -2     
+ Misses      16265    16233   -32     
- Partials     7990     8016   +26

Files with missing lines	Coverage Δ
src/async_wrap.cc	`84.40% <100.00%> (ø)`
src/env.h	`98.14% <ø> (ø)`
src/node.h	`92.30% <ø> (ø)`
src/node_internals.h	`81.03% <ø> (ø)`
src/api/callback.cc	`83.17% <94.73%> (+0.84%)`	⬆️
src/node_api.cc	`75.25% <86.66%> (-0.92%)`	⬇️
src/env.cc	`80.85% <57.14%> (-0.07%)`	⬇️
src/env-inl.h	`92.45% <27.27%> (-1.17%)`	⬇️

... and 31 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.

legendecas · 2025-09-10T21:00:45Z

napi_async_init initially only allocated a structure of node::async_context, and did not save a strong reference of the resource object. And napi_async_init allowed nullptr as the resource object.

However, after the resource object been exposed as async_hooks.executionAsyncResource(), napi_open_callback_scope and napi_make_callback should use the same object as the resource object, rather than a maybe nullptr, or a potentially different receiver object, to ensure correct behavior in AsyncLocalStorage.

Back to this PR, napi_async_destroy is the only one way to release the napi_async_context. node::async_context was pretty trivial if it is not correctly released, but it doesn't mean that it is correct to not call napi_async_destroy. So "enforcing" napi_async_destroy should be the correct way to go.

addaleax · 2025-09-10T23:53:49Z

@legendecas Okay, that makes some sense, but yeah, callers have always been required to have matching calls of napi_async_init() and napi_async_destroy() to avoid memory leaks, so removing the weak callback just makes the code a bit more streamlined and removes the requirement for a separate strong Global reference 👍

doc/api/n-api.md

This is a follow-up to 234c26c. The Node-API interface does not allow us to enforce that values are stored in a specific location, e.g. on the stack or not; however, V8 requires `Local<>` handles to be stored on the stack. To circumvent this restriction, we add the ability to the async handle stack to store either `Local<>*` pointers or `Global<>*` pointers, with Node-API making use of the latter.

There already is an existing requirement to have matching calls of `napi_async_init()` and `napi_async_destroy()`, so expecting users of this API to manually hold onto the resource for the duration of the `napi_async_context`'s lifetime is unnecessary. Weak callbacks are generally useful for when a corresponding C++ object should be cleaned up when a JS object is gargbage-collected, but that is not the case here.

nodejs-github-bot · 2025-09-19T09:11:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69283/

nodejs-github-bot · 2025-09-22T09:46:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/69319/ (💚)

nodejs-github-bot · 2025-09-22T14:16:12Z

Landed in 3e79dba...0ec1d18

This is a follow-up to 234c26c. The Node-API interface does not allow us to enforce that values are stored in a specific location, e.g. on the stack or not; however, V8 requires `Local<>` handles to be stored on the stack. To circumvent this restriction, we add the ability to the async handle stack to store either `Local<>*` pointers or `Global<>*` pointers, with Node-API making use of the latter. PR-URL: #59828 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

There already is an existing requirement to have matching calls of `napi_async_init()` and `napi_async_destroy()`, so expecting users of this API to manually hold onto the resource for the duration of the `napi_async_context`'s lifetime is unnecessary. Weak callbacks are generally useful for when a corresponding C++ object should be cleaned up when a JS object is gargbage-collected, but that is not the case here. PR-URL: #59828 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

targos · 2025-09-23T08:42:14Z

Looks like it depends on #59705

targos · 2025-11-21T14:21:04Z

src/node.h

 private:
-  void* reserved_;
-  v8::Local<v8::Object> resource_storage_;
+  void* resource_storage_global_;


What is this field for? I'm getting a -Wunused-private-field warning with Clang 21.

I opened #60802 in case it's safe to remove.

Refs: nodejs#59828

Refs: #59828 PR-URL: #60802 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 9, 2025

legendecas reviewed Sep 9, 2025

View reviewed changes

addaleax added the node-api Issues and PRs related to the Node-API. label Sep 10, 2025

github-project-automation bot added this to Node-API Team Project Sep 10, 2025

github-project-automation bot moved this to Need Triage in Node-API Team Project Sep 10, 2025

addaleax force-pushed the callbackscope-global-ptr branch 2 times, most recently from 9a20f3d to 85eef92 Compare September 10, 2025 23:53

legendecas reviewed Sep 12, 2025

View reviewed changes

doc/api/n-api.md Outdated Show resolved Hide resolved

legendecas moved this from Need Triage to In Progress in Node-API Team Project Sep 12, 2025

addaleax added 2 commits September 17, 2025 15:03

addaleax force-pushed the callbackscope-global-ptr branch from 2e83988 to 24cde9f Compare September 17, 2025 13:03

addaleax requested a review from legendecas September 17, 2025 13:03

legendecas approved these changes Sep 19, 2025

View reviewed changes

legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 19, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 19, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 22, 2025

nodejs-github-bot closed this Sep 22, 2025

github-project-automation bot moved this from In Progress to Done in Node-API Team Project Sep 22, 2025

targos added the dont-land-on-v24.x PRs that should not land on the v24.x-staging branch and should not be released in v24.x. label Sep 23, 2025

RafaelGSS mentioned this pull request Oct 13, 2025

2025-10-15, Version 25.0.0 (Current) #59896

Merged

9 tasks

targos reviewed Nov 21, 2025

View reviewed changes

targos added a commit to targos/node that referenced this pull request Nov 21, 2025

src: remove unused private field

35cece7

Refs: nodejs#59828

targos mentioned this pull request Nov 21, 2025

src: mark unused private field as such #60802

Merged

targos added a commit to targos/node that referenced this pull request Nov 25, 2025

src: mark unused private field as such

93d241e

Refs: nodejs#59828

targos added a commit that referenced this pull request Nov 27, 2025

src: mark unused private field as such

0415f7f

Refs: #59828 PR-URL: #60802 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

aduh95 pushed a commit that referenced this pull request Jan 9, 2026

src: mark unused private field as such

5bedb09

Refs: #59828 PR-URL: #60802 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

RafaelGSS pushed a commit that referenced this pull request Jan 13, 2026

src: mark unused private field as such

2dfdc6f

Refs: #59828 PR-URL: #60802 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Uh oh!

Conversation

addaleax commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

src: use Global for storing resource in Node-API callback scope

src: always use strong reference to napi_async_context resource

Uh oh!

nodejs-github-bot commented Sep 9, 2025

Uh oh!

legendecas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 9, 2025

Uh oh!

codecov bot commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

legendecas commented Sep 10, 2025

Uh oh!

addaleax commented Sep 10, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 19, 2025

Uh oh!

nodejs-github-bot commented Sep 22, 2025 • edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 22, 2025

Uh oh!

targos commented Sep 23, 2025

Uh oh!

targos Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

targos Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

addaleax commented Sep 9, 2025 •

edited

Loading

src: use `Global` for storing resource in Node-API callback scope

src: always use strong reference to `napi_async_context` resource

legendecas left a comment •

edited

Loading

codecov bot commented Sep 10, 2025 •

edited

Loading

nodejs-github-bot commented Sep 22, 2025 •

edited by addaleax

Loading