n-api: do not require JS Context for napi_async_destroy()

src/api/async_resource.cc

@@ -1,4 +1,5 @@
 #include "node.h"
+#include "env-inl.h"
 
 namespace node {
 
@@ -15,43 +16,44 @@ AsyncResource::AsyncResource(Isolate* isolate,
                              Local<Object> resource,
                              const char* name,
                              async_id trigger_async_id)
-    : isolate_(isolate),
+    : env_(Environment::GetCurrent(isolate)),
       resource_(isolate, resource) {
+  CHECK_NOT_NULL(env_);
   async_context_ = EmitAsyncInit(isolate, resource, name,
                                  trigger_async_id);
 }
 
 AsyncResource::~AsyncResource() {
-  EmitAsyncDestroy(isolate_, async_context_);
+  EmitAsyncDestroy(env_, async_context_);
   resource_.Reset();
 }
 
 MaybeLocal<Value> AsyncResource::MakeCallback(Local<Function> callback,
                                               int argc,
                                               Local<Value>* argv) {
-  return node::MakeCallback(isolate_, get_resource(),
+  return node::MakeCallback(env_->isolate(), get_resource(),
                             callback, argc, argv,
                             async_context_);
 }
 
 MaybeLocal<Value> AsyncResource::MakeCallback(const char* method,
                                               int argc,
                                               Local<Value>* argv) {
-  return node::MakeCallback(isolate_, get_resource(),
+  return node::MakeCallback(env_->isolate(), get_resource(),
                             method, argc, argv,
                             async_context_);
 }
 
 MaybeLocal<Value> AsyncResource::MakeCallback(Local<String> symbol,
                                               int argc,
                                               Local<Value>* argv) {
-  return node::MakeCallback(isolate_, get_resource(),
+  return node::MakeCallback(env_->isolate(), get_resource(),
                             symbol, argc, argv,
                             async_context_);
 }
 
 Local<Object> AsyncResource::get_resource() {
-  return resource_.Get(isolate_);
+  return resource_.Get(env_->isolate());
 }
 
 async_id AsyncResource::get_async_id() const {
@@ -62,9 +64,11 @@ async_id AsyncResource::get_trigger_async_id() const {
   return async_context_.trigger_async_id;
 }
 
+// TODO(addaleax): We shouldn’t need to use env_->isolate() if we’re just going
+// to end up using the Isolate* to figure out the Environment* again.
 AsyncResource::CallbackScope::CallbackScope(AsyncResource* res)
-    : node::CallbackScope(res->isolate_,
-                          res->resource_.Get(res->isolate_),
+    : node::CallbackScope(res->env_->isolate(),
+                          res->resource_.Get(res->env_->isolate()),
                           res->async_context_) {}
 
 }  // namespace node

src/api/hooks.cc

@@ -130,8 +130,11 @@ async_context EmitAsyncInit(Isolate* isolate,
 }
 
 void EmitAsyncDestroy(Isolate* isolate, async_context asyncContext) {
-  AsyncWrap::EmitDestroy(
-      Environment::GetCurrent(isolate), asyncContext.async_id);
+  EmitAsyncDestroy(Environment::GetCurrent(isolate), asyncContext);
+}
+
+void EmitAsyncDestroy(Environment* env, async_context asyncContext) {
+  AsyncWrap::EmitDestroy(env, asyncContext.async_id);
 }
 
 }  // namespace node

src/node.h

@@ -685,9 +685,16 @@ NODE_EXTERN async_context EmitAsyncInit(v8::Isolate* isolate,
                                         v8::Local<v8::String> name,
                                         async_id trigger_async_id = -1);
 
-/* Emit the destroy() callback. */
+/* Emit the destroy() callback. The overload taking an `Environment*` argument
+ * should be used when the Isolate’s current Context is not associated with
+ * a Node.js Environment, or when there is no current Context, for example
+ * when calling this function during garbage collection. In that case, the
+ * `Environment*` value should have been acquired previously, e.g. through
+ * `GetCurrentEnvironment()`. */
 NODE_EXTERN void EmitAsyncDestroy(v8::Isolate* isolate,
                                   async_context asyncContext);
+NODE_EXTERN void EmitAsyncDestroy(Environment* env,
+                                  async_context asyncContext);
 
 class InternalCallbackScope;
 
@@ -796,7 +803,7 @@ class NODE_EXTERN AsyncResource {
   };
 
  private:
-  v8::Isolate* isolate_;
+  Environment* env_;
   v8::Persistent<v8::Object> resource_;
   async_context async_context_;
 };

src/node_api.cc

@@ -632,10 +632,11 @@ napi_status napi_async_destroy(napi_env env,
   CHECK_ENV(env);
   CHECK_ARG(env, async_context);
 
-  v8::Isolate* isolate = env->isolate;
   node::async_context* node_async_context =
       reinterpret_cast<node::async_context*>(async_context);
-  node::EmitAsyncDestroy(isolate, *node_async_context);
+  node::EmitAsyncDestroy(
+      reinterpret_cast<node_napi_env>(env)->node_env(),
+      *node_async_context);
 
   delete node_async_context;
 

test/node-api/test_make_callback/binding.c

@@ -1,4 +1,6 @@
+#define NAPI_EXPERIMENTAL  // napi_add_finalizer
 #include <node_api.h>
+#include <assert.h>
 #include "../../js-native-api/common.h"
 
 #define MAX_ARGUMENTS 10
@@ -44,13 +46,42 @@ static napi_value MakeCallback(napi_env env, napi_callback_info info) {
   return result;
 }
 
+static void AsyncDestroyCb(napi_env env, void* data, void* hint) {
+  napi_status status = napi_async_destroy(env, (napi_async_context)data);
+  // We cannot use NAPI_ASSERT_RETURN_VOID because we need to have a JS stack
+  // below in order to use exceptions.
+  assert(status == napi_ok);
+}
+
+static napi_value CreateAsyncResource(napi_env env, napi_callback_info info) {
+  napi_value object;
+  NAPI_CALL(env, napi_create_object(env, &object));
+
+  napi_value resource_name;
+  NAPI_CALL(env, napi_create_string_utf8(
+      env, "test_gcable", NAPI_AUTO_LENGTH, &resource_name));
+
+  napi_async_context context;
+  NAPI_CALL(env, napi_async_init(env, object, resource_name, &context));
+
+  NAPI_CALL(env, napi_add_finalizer(
+      env, object, (void*)context, AsyncDestroyCb, NULL, NULL));
+
+  return object;
+}
+
 static
 napi_value Init(napi_env env, napi_value exports) {
   napi_value fn;
   NAPI_CALL(env, napi_create_function(
       // NOLINTNEXTLINE (readability/null_usage)
       env, NULL, NAPI_AUTO_LENGTH, MakeCallback, NULL, &fn));
   NAPI_CALL(env, napi_set_named_property(env, exports, "makeCallback", fn));
+  NAPI_CALL(env, napi_create_function(
+      // NOLINTNEXTLINE (readability/null_usage)
+      env, NULL, NAPI_AUTO_LENGTH, CreateAsyncResource, NULL, &fn));
+  NAPI_CALL(env, napi_set_named_property(
+      env, exports, "createAsyncResource", fn));
   return exports;
 }
 

test/node-api/test_make_callback/test-async-hooks-gcable.js

@@ -0,0 +1,44 @@
+'use strict';
+// Flags: --gc-interval=100 --gc-global
+
+const common = require('../../common');
+const assert = require('assert');
+const async_hooks = require('async_hooks');
+const { createAsyncResource } = require(`./build/${common.buildType}/binding`);
+
+// Test for https://github.com/nodejs/node/issues/27218:
+// napi_async_destroy() can be called during a regular garbage collection run.
+
+const hook_result = {
+  id: null,
+  init_called: false,
+  destroy_called: false,
+};
+
+const test_hook = async_hooks.createHook({
+  init: (id, type) => {
+    if (type === 'test_gcable') {
+      hook_result.id = id;
+      hook_result.init_called = true;
+    }
+  },
+  destroy: (id) => {
+    if (id === hook_result.id) hook_result.destroy_called = true;
+  },
+});
+
+test_hook.enable();
+createAsyncResource();
+
+// Trigger GC. This does *not* use global.gc(), because what we want to verify
+// is that `napi_async_destroy()` can be called when there is no JS context
+// on the stack at the time of GC.
+// Currently, using --gc-interval=100 + 1M elements seems to work fine for this.
+const arr = new Array(1024 * 1024);
+for (let i = 0; i < arr.length; i++)
+  arr[i] = {};
+
+assert.strictEqual(hook_result.destroy_called, false);
+setImmediate(() => {
+  assert.strictEqual(hook_result.destroy_called, true);
+});