src: use Global for storing resource in Node-API callback scope - #59828
src: use Global for storing resource in Node-API callback scope#59828addaleax wants to merge 2 commits into
Conversation
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
There was a problem hiding this comment.
The resource object used in the napi_callback_scope is already saved with a Global though:
Line 636 in 234c26c
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.
Sorry, something went wrong.
|
@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.
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? |
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 73.58491% with 14 lines in your changes missing coverage. Please review.
@@ 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
... and 31 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@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 👍 |
Sorry, something went wrong.
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.
Sorry, something went wrong.
Sorry, something went wrong.
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>
| private: | ||
| void* reserved_; | ||
| v8::Local<v8::Object> resource_storage_; | ||
| void* resource_storage_global_; |
There was a problem hiding this comment.
What is this field for? I'm getting a -Wunused-private-field warning with Clang 21.
Sorry, something went wrong.
There was a problem hiding this comment.
I opened #60802 in case it's safe to remove.
Sorry, something went wrong.
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.