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

[addaleax]

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]

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]

@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]

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]

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]

@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 👍

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 3e79dba...0ec1d18

[targos]

Looks like it depends on #59705