From 7947f48584e85ce0e9fcff87628506aeb10b3d29 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 16 Apr 2019 13:18:45 +0200 Subject: [PATCH 1/4] src: add `Environment` overload of `EmitAsyncDestroy` This can be necessary for being able to call the function when no JS Context is on the stack, e.g. during GC. Refs: https://github.com/nodejs/node/issues/27218 --- src/api/hooks.cc | 7 +++++-- src/node.h | 9 ++++++++- 2 files changed, 13 insertions(+), 3 deletions(-) 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..8d9744d2244ec5 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; From 41283076665b76611c70fa987c2aa3dd5830fdcc Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 16 Apr 2019 13:19:40 +0200 Subject: [PATCH 2/4] n-api: do not require JS Context for `napi_async_destroy()` Allow the function to be called during GC, which is a common use case. Fixes: https://github.com/nodejs/node/issues/27218 --- src/node_api.cc | 5 ++- test/node-api/test_make_callback/binding.c | 29 ++++++++++++ .../test-async-hooks-gcable.js | 44 +++++++++++++++++++ 3 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 test/node-api/test_make_callback/test-async-hooks-gcable.js 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..18f83007da3a77 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,28 @@ 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); + 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 +75,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); +}); From 66f6944eb1902fc6c96ee0ea5a22707308d75abf Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 16 Apr 2019 13:31:44 +0200 Subject: [PATCH 3/4] src: do not require JS Context for `~AsyncResoure()` Allow the destructor to be called during GC, which is a common use case. --- src/api/async_resource.cc | 20 ++++++++++++-------- src/node.h | 2 +- 2 files changed, 13 insertions(+), 9 deletions(-) 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/node.h b/src/node.h index 8d9744d2244ec5..072fdd889eca12 100644 --- a/src/node.h +++ b/src/node.h @@ -803,7 +803,7 @@ class NODE_EXTERN AsyncResource { }; private: - v8::Isolate* isolate_; + Environment* env_; v8::Persistent resource_; async_context async_context_; }; From f880b4f9f187032f8c9da0bc4a31e90b5366b6a4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 16 Apr 2019 13:47:29 +0200 Subject: [PATCH 4/4] fixup! n-api: do not require JS Context for `napi_async_destroy()` --- test/node-api/test_make_callback/binding.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/node-api/test_make_callback/binding.c b/test/node-api/test_make_callback/binding.c index 18f83007da3a77..5731e28a5b2597 100644 --- a/test/node-api/test_make_callback/binding.c +++ b/test/node-api/test_make_callback/binding.c @@ -48,6 +48,8 @@ static napi_value MakeCallback(napi_env env, napi_callback_info info) { 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); }