From 8b5d73867fe3c4be967af3d47fa801a79afca4f8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 16 Apr 2019 13:19:40 +0200 Subject: [PATCH] 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 PR-URL: https://github.com/nodejs/node/pull/27255 Reviewed-By: Ben Noordhuis Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- src/node_api.cc | 5 ++- test/node-api/test_make_callback/binding.c | 31 +++++++++++++ .../test-async-hooks-gcable.js | 44 +++++++++++++++++++ 3 files changed, 78 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 ecacd7013f66fe..ab48caa2374338 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); +});