Skip to content

Commit

Permalink
n-api: do not require JS Context for napi_async_destroy()
Browse files Browse the repository at this point in the history
Allow the function to be called during GC, which is a common use case.

Fixes: nodejs#27218
  • Loading branch information
addaleax committed Apr 16, 2019
1 parent 7947f48 commit 4128307
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 2 deletions.
5 changes: 3 additions & 2 deletions src/node_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
29 changes: 29 additions & 0 deletions test/node-api/test_make_callback/binding.c
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -44,13 +46,40 @@ 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;
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;
}

Expand Down
44 changes: 44 additions & 0 deletions test/node-api/test_make_callback/test-async-hooks-gcable.js
Original file line number Diff line number Diff line change
@@ -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);
});

0 comments on commit 4128307

Please sign in to comment.