Skip to content

Commit

Permalink
node-api: allow napi_delete_reference in finalizers
Browse files Browse the repository at this point in the history
`napi_delete_reference` must be called immediately for a
`napi_reference` returned from `napi_wrap` in the corresponding
finalizer, in order to clean up the `napi_reference` timely.

`napi_delete_reference` is safe to be invoked during GC.

PR-URL: #55620
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Vladimir Morozov <[email protected]>
  • Loading branch information
legendecas authored and ruyadorno committed Dec 21, 2024
1 parent 2d06c8b commit 3742b75
Show file tree
Hide file tree
Showing 5 changed files with 71 additions and 5 deletions.
4 changes: 3 additions & 1 deletion src/js_native_api_v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2769,10 +2769,12 @@ napi_status NAPI_CDECL napi_create_reference(napi_env env,

// Deletes a reference. The referenced value is released, and may be GC'd unless
// there are other references to it.
// For a napi_reference returned from `napi_wrap`, this must be called in the
// finalizer.
napi_status NAPI_CDECL napi_delete_reference(napi_env env, napi_ref ref) {
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot throw
// JS exceptions.
CHECK_ENV_NOT_IN_GC(env);
CHECK_ENV(env);
CHECK_ARG(env, ref);

delete reinterpret_cast<v8impl::Reference*>(ref);
Expand Down
37 changes: 34 additions & 3 deletions test/js-native-api/6_object_wrap/6_object_wrap.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,25 @@
#include "assert.h"
#include "myobject.h"

typedef int32_t FinalizerData;

napi_ref MyObject::constructor;

MyObject::MyObject(double value)
: value_(value), env_(nullptr), wrapper_(nullptr) {}

MyObject::~MyObject() { napi_delete_reference(env_, wrapper_); }

void MyObject::Destructor(
napi_env env, void* nativeObject, void* /*finalize_hint*/) {
void MyObject::Destructor(node_api_basic_env env,
void* nativeObject,
void* /*finalize_hint*/) {
MyObject* obj = static_cast<MyObject*>(nativeObject);
delete obj;

FinalizerData* data;
NODE_API_BASIC_CALL_RETURN_VOID(
env, napi_get_instance_data(env, reinterpret_cast<void**>(&data)));
*data += 1;
}

void MyObject::Init(napi_env env, napi_value exports) {
Expand Down Expand Up @@ -154,7 +162,7 @@ napi_value MyObject::Multiply(napi_env env, napi_callback_info info) {
}

// This finalizer should never be invoked.
void ObjectWrapDanglingReferenceFinalizer(napi_env env,
void ObjectWrapDanglingReferenceFinalizer(node_api_basic_env env,
void* finalize_data,
void* finalize_hint) {
assert(0 && "unreachable");
Expand Down Expand Up @@ -198,15 +206,38 @@ napi_value ObjectWrapDanglingReferenceTest(napi_env env,
return ret;
}

static napi_value GetFinalizerCallCount(napi_env env, napi_callback_info info) {
size_t argc = 1;
napi_value argv[1];
FinalizerData* data;
napi_value result;

NODE_API_CALL(env,
napi_get_cb_info(env, info, &argc, argv, nullptr, nullptr));
NODE_API_CALL(env,
napi_get_instance_data(env, reinterpret_cast<void**>(&data)));
NODE_API_CALL(env, napi_create_int32(env, *data, &result));
return result;
}

static void finalizeData(napi_env env, void* data, void* hint) {
delete reinterpret_cast<FinalizerData*>(data);
}

EXTERN_C_START
napi_value Init(napi_env env, napi_value exports) {
FinalizerData* data = new FinalizerData;
*data = 0;
NODE_API_CALL(env, napi_set_instance_data(env, data, finalizeData, nullptr));

MyObject::Init(env, exports);

napi_property_descriptor descriptors[] = {
DECLARE_NODE_API_PROPERTY("objectWrapDanglingReference",
ObjectWrapDanglingReference),
DECLARE_NODE_API_PROPERTY("objectWrapDanglingReferenceTest",
ObjectWrapDanglingReferenceTest),
DECLARE_NODE_API_PROPERTY("getFinalizerCallCount", GetFinalizerCallCount),
};

NODE_API_CALL(
Expand Down
7 changes: 7 additions & 0 deletions test/js-native-api/6_object_wrap/binding.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,13 @@
"sources": [
"6_object_wrap.cc"
]
},
{
"target_name": "6_object_wrap_basic_finalizer",
"defines": [ "NAPI_EXPERIMENTAL" ],
"sources": [
"6_object_wrap.cc"
]
}
]
}
4 changes: 3 additions & 1 deletion test/js-native-api/6_object_wrap/myobject.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
class MyObject {
public:
static void Init(napi_env env, napi_value exports);
static void Destructor(napi_env env, void* nativeObject, void* finalize_hint);
static void Destructor(node_api_basic_env env,
void* nativeObject,
void* finalize_hint);

private:
explicit MyObject(double value_ = 0);
Expand Down
24 changes: 24 additions & 0 deletions test/js-native-api/6_object_wrap/test-basic-finalizer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Flags: --expose-gc

'use strict';
const common = require('../../common');
const assert = require('assert');
const addon = require(`./build/${common.buildType}/6_object_wrap_basic_finalizer`);

// This test verifies that ObjectWrap can be correctly finalized with a node_api_basic_finalizer
// in the current JS loop tick
(() => {
let obj = new addon.MyObject(9);
obj = null;
// Silent eslint about unused variables.
assert.strictEqual(obj, null);
})();

for (let i = 0; i < 10; ++i) {
global.gc();
if (addon.getFinalizerCallCount() === 1) {
break;
}
}

assert.strictEqual(addon.getFinalizerCallCount(), 1);

0 comments on commit 3742b75

Please sign in to comment.