diff --git a/src/api/async_resource.cc b/src/api/async_resource.cc index 1d8224114e9511..5141c7c6b35abd 100644 --- a/src/api/async_resource.cc +++ b/src/api/async_resource.cc @@ -1,4 +1,5 @@ #include "node.h" +#include "env-inl.h" namespace node { @@ -15,21 +16,22 @@ AsyncResource::AsyncResource(Isolate* isolate, Local 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 AsyncResource::MakeCallback(Local callback, int argc, Local* argv) { - return node::MakeCallback(isolate_, get_resource(), + return node::MakeCallback(env_->isolate(), get_resource(), callback, argc, argv, async_context_); } @@ -37,7 +39,7 @@ MaybeLocal AsyncResource::MakeCallback(Local callback, MaybeLocal AsyncResource::MakeCallback(const char* method, int argc, Local* argv) { - return node::MakeCallback(isolate_, get_resource(), + return node::MakeCallback(env_->isolate(), get_resource(), method, argc, argv, async_context_); } @@ -45,13 +47,13 @@ MaybeLocal AsyncResource::MakeCallback(const char* method, MaybeLocal AsyncResource::MakeCallback(Local symbol, int argc, Local* argv) { - return node::MakeCallback(isolate_, get_resource(), + return node::MakeCallback(env_->isolate(), get_resource(), symbol, argc, argv, async_context_); } Local 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 diff --git a/src/api/hooks.cc b/src/api/hooks.cc index 3b1ee90a99ae1d..cec58cee00847c 100644 --- a/src/api/hooks.cc +++ b/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 diff --git a/src/node.h b/src/node.h index e638ff24182201..072fdd889eca12 100644 --- a/src/node.h +++ b/src/node.h @@ -685,9 +685,16 @@ NODE_EXTERN async_context EmitAsyncInit(v8::Isolate* isolate, v8::Local 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 resource_; async_context async_context_; }; diff --git a/src/node_api.cc b/src/node_api.cc index fa68323b3c1777..35f73e8f245694 100644 --- a/src/node_api.cc +++ b/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(async_context); - node::EmitAsyncDestroy(isolate, *node_async_context); + node::EmitAsyncDestroy( + reinterpret_cast(env)->node_env(), + *node_async_context); delete node_async_context; diff --git a/test/node-api/test_make_callback/binding.c b/test/node-api/test_make_callback/binding.c index 04e95e2570afe3..5731e28a5b2597 100644 --- a/test/node-api/test_make_callback/binding.c +++ b/test/node-api/test_make_callback/binding.c @@ -1,4 +1,6 @@ +#define NAPI_EXPERIMENTAL // napi_add_finalizer #include +#include #include "../../js-native-api/common.h" #define MAX_ARGUMENTS 10 @@ -44,6 +46,30 @@ 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; @@ -51,6 +77,11 @@ napi_value Init(napi_env env, napi_value exports) { // 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; } diff --git a/test/node-api/test_make_callback/test-async-hooks-gcable.js b/test/node-api/test_make_callback/test-async-hooks-gcable.js new file mode 100644 index 00000000000000..a9b4d3d75d6040 --- /dev/null +++ b/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); +});