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.

PR-URL: #59828
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Author: addaleax

doc/api/n-api.md

@@ -5984,6 +5984,10 @@ the runtime.
 <!-- YAML
 added: v8.6.0
 napiVersion: 1
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/59828
+    description: The `async_resource` object will now be held as a strong reference.
 -->
 
 ```c
@@ -6003,23 +6007,19 @@ napi_status napi_async_init(napi_env env,
 
 Returns `napi_ok` if the API succeeded.
 
-The `async_resource` object needs to be kept alive until
-[`napi_async_destroy`][] to keep `async_hooks` related API acts correctly. In
-order to retain ABI compatibility with previous versions, `napi_async_context`s
-are not maintaining the strong reference to the `async_resource` objects to
-avoid introducing causing memory leaks. However, if the `async_resource` is
-garbage collected by JavaScript engine before the `napi_async_context` was
-destroyed by `napi_async_destroy`, calling `napi_async_context` related APIs
-like [`napi_open_callback_scope`][] and [`napi_make_callback`][] can cause
-problems like loss of async context when using the `AsyncLocalStorage` API.
-
 In order to retain ABI compatibility with previous versions, passing `NULL`
 for `async_resource` does not result in an error. However, this is not
 recommended as this will result in undesirable behavior with  `async_hooks`
 [`init` hooks][] and `async_hooks.executionAsyncResource()` as the resource is
 now required by the underlying `async_hooks` implementation in order to provide
 the linkage between async callbacks.
 
+Previous versions of this API were not maintaining a strong reference to
+`async_resource` while the `napi_async_context` object existed and instead
+expected the caller to hold a strong reference. This has been changed, as a
+corresponding call to [`napi_async_destroy`][] for every call to
+`napi_async_init()` is a requirement in any case to avoid memory leaks.
+
 ### `napi_async_destroy`
 
 <!-- YAML

src/node_api.cc

@@ -538,19 +538,13 @@ class AsyncContext {
  public:
   AsyncContext(node_napi_env env,
                v8::Local<v8::Object> resource_object,
-               const v8::Local<v8::String> resource_name,
-               bool externally_managed_resource)
+               v8::Local<v8::String> resource_name)
       : env_(env) {
     async_id_ = node_env()->new_async_id();
     trigger_async_id_ = node_env()->get_default_trigger_async_id();
     v8::Isolate* isolate = node_env()->isolate();
     resource_.Reset(isolate, resource_object);
     context_frame_.Reset(isolate, node::async_context_frame::current(isolate));
-    lost_reference_ = false;
-    if (externally_managed_resource) {
-      resource_.SetWeak(
-          this, AsyncContext::WeakCallback, v8::WeakCallbackType::kParameter);
-    }
 
     node::AsyncWrap::EmitAsyncInit(node_env(),
                                    resource_object,
@@ -559,18 +553,13 @@ class AsyncContext {
                                    trigger_async_id_);
   }
 
-  ~AsyncContext() {
-    resource_.Reset();
-    lost_reference_ = true;
-    node::AsyncWrap::EmitDestroy(node_env(), async_id_);
-  }
+  ~AsyncContext() { node::AsyncWrap::EmitDestroy(node_env(), async_id_); }
 
   inline v8::MaybeLocal<v8::Value> MakeCallback(
       v8::Local<v8::Object> recv,
       const v8::Local<v8::Function> callback,
       int argc,
       v8::Local<v8::Value> argv[]) {
-    EnsureReference();
     return node::InternalMakeCallback(
         node_env(),
         resource(),
@@ -583,21 +572,11 @@ class AsyncContext {
   }
 
   inline napi_callback_scope OpenCallbackScope() {
-    EnsureReference();
     auto scope = new HeapAllocatedCallbackScope(this);
     env_->open_callback_scopes++;
     return scope->to_opaque();
   }
 
-  inline void EnsureReference() {
-    if (lost_reference_) {
-      const v8::HandleScope handle_scope(node_env()->isolate());
-      resource_.Reset(node_env()->isolate(),
-                      v8::Object::New(node_env()->isolate()));
-      lost_reference_ = false;
-    }
-  }
-
   inline node::Environment* node_env() { return env_->node_env(); }
   inline v8::Local<v8::Object> resource() {
     return resource_.Get(node_env()->isolate());
@@ -609,15 +588,10 @@ class AsyncContext {
   static inline void CloseCallbackScope(node_napi_env env,
                                         napi_callback_scope s) {
     delete HeapAllocatedCallbackScope::FromOpaque(s);
+    CHECK_GT(env->open_callback_scopes, 0);
     env->open_callback_scopes--;
   }
 
-  static void WeakCallback(const v8::WeakCallbackInfo<AsyncContext>& data) {
-    AsyncContext* async_context = data.GetParameter();
-    async_context->resource_.Reset();
-    async_context->lost_reference_ = true;
-  }
-
  private:
   class HeapAllocatedCallbackScope final {
    public:
@@ -629,23 +603,18 @@ class AsyncContext {
     }
 
     explicit HeapAllocatedCallbackScope(AsyncContext* async_context)
-        : resource_storage_(async_context->node_env()->isolate(),
-                            async_context->resource_.Get(
-                                async_context->node_env()->isolate())),
-          cs_(async_context->node_env(),
-              &resource_storage_,
+        : cs_(async_context->node_env(),
+              &async_context->resource_,
               async_context->async_context()) {}
 
    private:
-    v8::Global<v8::Object> resource_storage_;
     node::CallbackScope cs_;
   };
 
   node_napi_env env_;
   double async_id_;
   double trigger_async_id_;
   v8::Global<v8::Object> resource_;
-  bool lost_reference_;
   v8::Global<v8::Value> context_frame_;
 };
 
@@ -943,23 +912,17 @@ napi_status NAPI_CDECL napi_async_init(napi_env env,
   v8::Local<v8::Context> context = env->context();
 
   v8::Local<v8::Object> v8_resource;
-  bool externally_managed_resource;
   if (async_resource != nullptr) {
     CHECK_TO_OBJECT(env, context, v8_resource, async_resource);
-    externally_managed_resource = true;
   } else {
     v8_resource = v8::Object::New(isolate);
-    externally_managed_resource = false;
   }
 
   v8::Local<v8::String> v8_resource_name;
   CHECK_TO_STRING(env, context, v8_resource_name, async_resource_name);
 
-  v8impl::AsyncContext* async_context =
-      new v8impl::AsyncContext(reinterpret_cast<node_napi_env>(env),
-                               v8_resource,
-                               v8_resource_name,
-                               externally_managed_resource);
+  v8impl::AsyncContext* async_context = new v8impl::AsyncContext(
+      reinterpret_cast<node_napi_env>(env), v8_resource, v8_resource_name);
 
   *result = reinterpret_cast<napi_async_context>(async_context);
 

test/node-api/test_async_context/test-gcable-callback.js

@@ -50,13 +50,10 @@ setImmediate(() => {
   assert.strictEqual(hook_result.destroy_called, false);
   makeCallback(asyncResource, process, () => {
     const executionAsyncResource = async_hooks.executionAsyncResource();
-    // Assuming the executionAsyncResource was created for the absence of the
-    // initial `{ foo: 'bar' }`.
-    // This is the worst path of `napi_async_context` related API of
-    // recovering from the condition and not break the executionAsyncResource
-    // shape, although the executionAsyncResource might not be correct.
+    // Previous versions of Node-API would have gargbage-collected
+    // the `asyncResource` object, now we can just assert that it is intact.
     assert.strictEqual(typeof executionAsyncResource, 'object');
-    assert.strictEqual(executionAsyncResource.foo, undefined);
+    assert.strictEqual(executionAsyncResource.foo, 'bar');
     destroyAsyncResource(asyncResource);
     setImmediate(() => {
       assert.strictEqual(hook_result.destroy_called, true);