From bbb0fb55d7976fbb3ecd34737cccb2b9317b06dd Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 7 Apr 2023 07:40:16 -0700 Subject: [PATCH 01/15] node-api: get Node API version used by addon --- src/api/environment.cc | 3 +- src/js_native_api_v8.cc | 8 +- src/js_native_api_v8.h | 10 +- src/node_api.cc | 19 +- src/node_api.h | 10 + src/node_api_internals.h | 3 +- src/node_binding.cc | 13 +- src/node_binding.h | 5 +- src/node_version.h | 6 +- test/cctest/test_node_api.cc | 3 +- .../test_reference_all_types/binding.gyp | 9 + .../node-api/test_reference_all_types/test.js | 90 +++++++++ .../test_reference_all_types.c | 175 ++++++++++++++++++ .../test_reference_obj_only/binding.gyp | 9 + test/node-api/test_reference_obj_only/test.js | 106 +++++++++++ .../test_reference_obj_only.c | 175 ++++++++++++++++++ 16 files changed, 625 insertions(+), 19 deletions(-) create mode 100644 test/node-api/test_reference_all_types/binding.gyp create mode 100644 test/node-api/test_reference_all_types/test.js create mode 100644 test/node-api/test_reference_all_types/test_reference_all_types.c create mode 100644 test/node-api/test_reference_obj_only/binding.gyp create mode 100644 test/node-api/test_reference_obj_only/test.js create mode 100644 test/node-api/test_reference_obj_only/test_reference_obj_only.c diff --git a/src/api/environment.cc b/src/api/environment.cc index cfc3516d61f78a..0aa5f78a00f4a8 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -887,7 +887,8 @@ void AddLinkedBinding(Environment* env, exports, module, context, - reinterpret_cast(priv)); + reinterpret_cast(priv), + NAPI_DEFAULT_MODULE_API_VERSION); }, name, reinterpret_cast(fn), diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index f4df74299781c8..4186ec02f5231c 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -2419,9 +2419,11 @@ napi_status NAPI_CDECL napi_create_reference(napi_env env, CHECK_ARG(env, result); v8::Local v8_value = v8impl::V8LocalValueFromJsValue(value); - if (!(v8_value->IsObject() || v8_value->IsFunction() || - v8_value->IsSymbol())) { - return napi_set_last_error(env, napi_invalid_arg); + if (env->module_api_version <= 8) { + if (!(v8_value->IsObject() || v8_value->IsFunction() || + v8_value->IsSymbol())) { + return napi_set_last_error(env, napi_invalid_arg); + } } v8impl::Reference* reference = v8impl::Reference::New( diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 6378a055208ef9..0b4482b94549b8 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -51,8 +51,13 @@ class Finalizer; } // end of namespace v8impl struct napi_env__ { - explicit napi_env__(v8::Local context) - : isolate(context->GetIsolate()), context_persistent(isolate, context) { + explicit napi_env__(v8::Local context, + int32_t module_api_version) + : isolate(context->GetIsolate()), + context_persistent(isolate, context), + module_api_version(module_api_version != 0 + ? module_api_version + : NAPI_DEFAULT_MODULE_API_VERSION) { napi_clear_last_error(this); } @@ -144,6 +149,7 @@ struct napi_env__ { int open_callback_scopes = 0; int refs = 1; void* instance_data = nullptr; + int32_t module_api_version = NAPI_DEFAULT_MODULE_API_VERSION; protected: // Should not be deleted directly. Delete with `napi_env__::DeleteMe()` diff --git a/src/node_api.cc b/src/node_api.cc index 096a6e580fb9e8..f79905330202df 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -20,8 +20,9 @@ #include node_napi_env__::node_napi_env__(v8::Local context, - const std::string& module_filename) - : napi_env__(context), filename(module_filename) { + const std::string& module_filename, + int32_t module_api_version) + : napi_env__(context, module_api_version), filename(module_filename) { CHECK_NOT_NULL(node_env()); } @@ -152,10 +153,11 @@ class BufferFinalizer : private Finalizer { }; inline napi_env NewEnv(v8::Local context, - const std::string& module_filename) { + const std::string& module_filename, + int32_t module_api_version) { node_napi_env result; - result = new node_napi_env__(context, module_filename); + result = new node_napi_env__(context, module_filename, module_api_version); // TODO(addaleax): There was previously code that tried to delete the // napi_env when its v8::Context was garbage collected; // However, as long as N-API addons using this napi_env are in place, @@ -616,17 +618,20 @@ static void napi_module_register_cb(v8::Local exports, v8::Local module, v8::Local context, void* priv) { + const napi_module* napi_mod = static_cast(priv); napi_module_register_by_symbol( exports, module, context, - static_cast(priv)->nm_register_func); + napi_mod->nm_register_func, + NAPI_DEFAULT_MODULE_API_VERSION); } void napi_module_register_by_symbol(v8::Local exports, v8::Local module, v8::Local context, - napi_addon_register_func init) { + napi_addon_register_func init, + int32_t module_api_version) { node::Environment* node_env = node::Environment::GetCurrent(context); std::string module_filename = ""; if (init == nullptr) { @@ -654,7 +659,7 @@ void napi_module_register_by_symbol(v8::Local exports, } // Create a new napi_env for this specific module. - napi_env env = v8impl::NewEnv(context, module_filename); + napi_env env = v8impl::NewEnv(context, module_filename, module_api_version); napi_value _exports = nullptr; env->CallIntoModule([&](napi_env env) { diff --git a/src/node_api.h b/src/node_api.h index 4d1b50414e2e02..b5355f457dae75 100644 --- a/src/node_api.h +++ b/src/node_api.h @@ -30,6 +30,7 @@ struct uv_loop_s; // Forward declaration. typedef napi_value(NAPI_CDECL* napi_addon_register_func)(napi_env env, napi_value exports); +typedef int32_t(NAPI_CDECL* napi_addon_get_api_version_func)(); // Used by deprecated registration method napi_module_register. typedef struct napi_module { @@ -54,11 +55,20 @@ typedef struct napi_module { #define NAPI_MODULE_INITIALIZER_BASE napi_register_module_v #endif +#define NAPI_MODULE_GET_API_VERSION_BASE napi_module_get_api_version_v + #define NAPI_MODULE_INITIALIZER \ NAPI_MODULE_INITIALIZER_X(NAPI_MODULE_INITIALIZER_BASE, NAPI_MODULE_VERSION) +#define NAPI_MODULE_GET_API_VERSION \ + NAPI_MODULE_INITIALIZER_X(NAPI_MODULE_GET_API_VERSION_BASE, \ + NAPI_MODULE_VERSION) + #define NAPI_MODULE_INIT() \ EXTERN_C_START \ + NAPI_MODULE_EXPORT int32_t NAPI_MODULE_GET_API_VERSION() { \ + return NAPI_VERSION; \ + } \ NAPI_MODULE_EXPORT napi_value NAPI_MODULE_INITIALIZER(napi_env env, \ napi_value exports); \ EXTERN_C_END \ diff --git a/src/node_api_internals.h b/src/node_api_internals.h index 5201966b779508..25f6b291902024 100644 --- a/src/node_api_internals.h +++ b/src/node_api_internals.h @@ -10,7 +10,8 @@ struct node_napi_env__ : public napi_env__ { node_napi_env__(v8::Local context, - const std::string& module_filename); + const std::string& module_filename, + int32_t module_api_version); bool can_call_into_js() const override; void CallFinalizer(napi_finalize cb, void* data, void* hint) override; diff --git a/src/node_binding.cc b/src/node_binding.cc index 90855aada5dab9..dfcf59736e33fe 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -408,6 +408,12 @@ inline napi_addon_register_func GetNapiInitializerCallback(DLib* dlib) { dlib->GetSymbolAddress(name)); } +inline napi_addon_get_api_version_func GetNapiAddonGetApiVersionCallback( + DLib* dlib) { + return reinterpret_cast( + dlib->GetSymbolAddress(STRINGIFY(NAPI_MODULE_GET_API_VERSION))); +} + // DLOpen is process.dlopen(module, filename, flags). // Used to load 'module.node' dynamically shared objects. // @@ -484,7 +490,12 @@ void DLOpen(const FunctionCallbackInfo& args) { callback(exports, module, context); return true; } else if (auto napi_callback = GetNapiInitializerCallback(dlib)) { - napi_module_register_by_symbol(exports, module, context, napi_callback); + int32_t module_api_version = 0; + if (auto get_version = GetNapiAddonGetApiVersionCallback(dlib)) { + module_api_version = get_version(); + } + napi_module_register_by_symbol( + exports, module, context, napi_callback, module_api_version); return true; } else { mp = dlib->GetSavedModuleFromGlobalHandleMap(); diff --git a/src/node_binding.h b/src/node_binding.h index 1b024774e120a9..805064b36ebd4b 100644 --- a/src/node_binding.h +++ b/src/node_binding.h @@ -21,7 +21,7 @@ enum { // Make sure our internal values match the public API's values. static_assert(static_cast(NM_F_LINKED) == - static_cast(node::ModuleFlags::kLinked), + static_cast(node::ModuleFlags::kLinked), "NM_F_LINKED != node::ModuleFlags::kLinked"); #if NODE_HAVE_I18N_SUPPORT @@ -52,7 +52,8 @@ static_assert(static_cast(NM_F_LINKED) == void napi_module_register_by_symbol(v8::Local exports, v8::Local module, v8::Local context, - napi_addon_register_func init); + napi_addon_register_func init, + int32_t module_api_version); namespace node { diff --git a/src/node_version.h b/src/node_version.h index f24b9387937880..3f93a18ef09525 100644 --- a/src/node_version.h +++ b/src/node_version.h @@ -93,6 +93,10 @@ // The NAPI_VERSION provided by this version of the runtime. This is the version // which the Node binary being built supports. -#define NAPI_VERSION 8 +#define NAPI_VERSION 8 + +// The NAPI_VERSION used by default by Node API modules if it is not explicitly +// specified. It must be always 8. +#define NAPI_DEFAULT_MODULE_API_VERSION 8 #endif // SRC_NODE_VERSION_H_ diff --git a/test/cctest/test_node_api.cc b/test/cctest/test_node_api.cc index ad5be52fc8ffcc..261e3e368c1012 100644 --- a/test/cctest/test_node_api.cc +++ b/test/cctest/test_node_api.cc @@ -34,7 +34,8 @@ TEST_F(NodeApiTest, CreateNodeApiEnv) { }; Local module_obj = Object::New(isolate_); Local exports_obj = Object::New(isolate_); - napi_module_register_by_symbol(exports_obj, module_obj, env->context(), init); + napi_module_register_by_symbol( + exports_obj, module_obj, env->context(), init, 0); ASSERT_NE(addon_env, nullptr); node_napi_env internal_env = reinterpret_cast(addon_env); EXPECT_EQ(internal_env->node_env(), env); diff --git a/test/node-api/test_reference_all_types/binding.gyp b/test/node-api/test_reference_all_types/binding.gyp new file mode 100644 index 00000000000000..f58807b0e5baf1 --- /dev/null +++ b/test/node-api/test_reference_all_types/binding.gyp @@ -0,0 +1,9 @@ +{ + "targets": [ + { + "target_name": "test_reference_all_types", + "sources": [ "test_reference_all_types.c" ], + "defines": [ 'NAPI_EXPERIMENTAL' ] + } + ] +} diff --git a/test/node-api/test_reference_all_types/test.js b/test/node-api/test_reference_all_types/test.js new file mode 100644 index 00000000000000..caee3f4d803125 --- /dev/null +++ b/test/node-api/test_reference_all_types/test.js @@ -0,0 +1,90 @@ +'use strict'; +// Flags: --expose-gc +// +// Testing API calls for references to all value types. +// This test uses NAPI_MODULE_INIT macro to initialize module. +// +const { gcUntil, buildType } = require('../../common'); +const assert = require('assert'); +const addon = require(`./build/${buildType}/test_reference_all_types`); + +async function runTests() { + let allEntries = []; + + (() => { + // Create values of all napi_valuetype types. + const undefinedValue = undefined; + const nullValue = null; + const booleanValue = false; + const numberValue = 42; + const stringValue = 'test_string'; + const symbolValue = Symbol.for('test_symbol'); + const objectValue = { x: 1, y: 2 }; + const functionValue = (x, y) => x + y; + const externalValue = addon.createExternal(); + const bigintValue = 9007199254740991n; + + allEntries = [ + { value: undefinedValue, canBeWeak: false }, + { value: nullValue, canBeWeak: false }, + { value: booleanValue, canBeWeak: false }, + { value: numberValue, canBeWeak: false }, + { value: stringValue, canBeWeak: false }, + { value: symbolValue, canBeWeak: false }, + { value: objectValue, canBeWeak: true }, + { value: functionValue, canBeWeak: true }, + { value: externalValue, canBeWeak: true }, + { value: bigintValue, canBeWeak: false }, + ]; + + // Go over all values of different types, create strong ref values for + // them, read the stored values, and check how the ref count works. + for (const entry of allEntries) { + const index = addon.createRef(entry.value); + const refValue = addon.getRefValue(index); + assert.strictEqual(entry.value, refValue); + assert.strictEqual(addon.ref(index), 2); + assert.strictEqual(addon.unref(index), 1); + assert.strictEqual(addon.unref(index), 0); + } + + // The references become weak pointers when the ref count is 0. + // Here we know that the GC is not run yet because the values are + // still in the allEntries array. + allEntries.forEach((entry, index) => { + assert.strictEqual(addon.getRefValue(index), entry.value); + // Set to undefined to allow GC collect the value. + entry.value = undefined; + }); + + // To check that GC pass is done. + const objWithFinalizer = {}; + addon.addFinalizer(objWithFinalizer); + })(); + + assert.strictEqual(addon.getFinalizeCount(), 0); + await gcUntil('Wait until a finalizer is called', + () => (addon.getFinalizeCount() === 1)); + + // Create and call finalizer again to make sure that we had another GC pass. + (() => { + const objWithFinalizer = {}; + addon.addFinalizer(objWithFinalizer); + })(); + await gcUntil('Wait until a finalizer is called again', + () => (addon.getFinalizeCount() === 2)); + + // After GC and finalizers run, all values that support weak reference + // semantic must return undefined value. + // It also includes the value at index 0 because it is the undefined value. + // Other value types are not collected by GC. + allEntries.forEach((entry, index) => { + if (entry.canBeWeak || index === 0) { + assert.strictEqual(addon.getRefValue(index), undefined); + } else { + assert.notStrictEqual(addon.getRefValue(index), undefined); + } + addon.deleteRef(index); + }); +} +runTests(); diff --git a/test/node-api/test_reference_all_types/test_reference_all_types.c b/test/node-api/test_reference_all_types/test_reference_all_types.c new file mode 100644 index 00000000000000..332baa7e8433d2 --- /dev/null +++ b/test/node-api/test_reference_all_types/test_reference_all_types.c @@ -0,0 +1,175 @@ +#define NAPI_EXPERIMENTAL +#include +#include "../../js-native-api/common.h" +#include "stdlib.h" + +#define NODE_API_ASSERT_STATUS(env, assertion, message) \ + NODE_API_ASSERT_BASE(env, assertion, message, napi_generic_failure) + +#define NODE_API_CHECK_STATUS(env, the_call) \ + do { \ + napi_status status = (the_call); \ + if (status != napi_ok) { \ + return status; \ + } \ + } while (0) + +static uint32_t finalizeCount = 0; + +static void FreeData(napi_env env, void* data, void* hint) { + NODE_API_ASSERT_RETURN_VOID(env, data != NULL, "Expects non-NULL data."); + free(data); +} + +static void Finalize(napi_env env, void* data, void* hint) { + ++finalizeCount; +} + +static napi_status GetArgValue(napi_env env, + napi_callback_info info, + napi_value* argValue) { + size_t argc = 1; + NODE_API_CHECK_STATUS( + env, napi_get_cb_info(env, info, &argc, argValue, NULL, NULL)); + + NODE_API_ASSERT_STATUS(env, argc == 1, "Expects one arg."); + return napi_ok; +} + +static napi_status GetArgValueAsIndex(napi_env env, + napi_callback_info info, + uint32_t* index) { + napi_value argValue; + NODE_API_CHECK_STATUS(env, GetArgValue(env, info, &argValue)); + + napi_valuetype valueType; + NODE_API_CHECK_STATUS(env, napi_typeof(env, argValue, &valueType)); + NODE_API_ASSERT_STATUS( + env, valueType == napi_number, "Argument must be a number."); + + return napi_get_value_uint32(env, argValue, index); +} + +static napi_status GetRef(napi_env env, + napi_callback_info info, + napi_ref* ref) { + uint32_t index; + NODE_API_CHECK_STATUS(env, GetArgValueAsIndex(env, info, &index)); + + napi_ref* refValues; + NODE_API_CHECK_STATUS(env, napi_get_instance_data(env, (void**)&refValues)); + NODE_API_ASSERT_STATUS(env, refValues != NULL, "Cannot get instance data."); + + *ref = refValues[index]; + return napi_ok; +} + +static napi_value ToUInt32Value(napi_env env, uint32_t value) { + napi_value result; + NODE_API_CALL(env, napi_create_uint32(env, value, &result)); + return result; +} + +static napi_status InitRefArray(napi_env env) { + // valueRefs array has one entry per napi_valuetype + napi_ref* valueRefs = malloc(sizeof(napi_ref) * ((int)napi_bigint + 1)); + return napi_set_instance_data(env, valueRefs, &FreeData, NULL); +} + +static napi_value CreateExternal(napi_env env, napi_callback_info info) { + napi_value result; + int* data = (int*)malloc(sizeof(int)); + *data = 42; + NODE_API_CALL(env, napi_create_external(env, data, &FreeData, NULL, &result)); + return result; +} + +static napi_value CreateRef(napi_env env, napi_callback_info info) { + napi_value argValue; + NODE_API_CALL(env, GetArgValue(env, info, &argValue)); + + napi_valuetype valueType; + NODE_API_CALL(env, napi_typeof(env, argValue, &valueType)); + uint32_t index = (uint32_t)valueType; + + napi_ref* valueRefs; + NODE_API_CALL(env, napi_get_instance_data(env, (void**)&valueRefs)); + NODE_API_CALL(env, + napi_create_reference(env, argValue, 1, valueRefs + index)); + + return ToUInt32Value(env, index); +} + +static napi_value GetRefValue(napi_env env, napi_callback_info info) { + napi_ref refValue; + NODE_API_CALL(env, GetRef(env, info, &refValue)); + napi_value value; + NODE_API_CALL(env, napi_get_reference_value(env, refValue, &value)); + return value; +} + +static napi_value Ref(napi_env env, napi_callback_info info) { + napi_ref refValue; + NODE_API_CALL(env, GetRef(env, info, &refValue)); + uint32_t refCount; + NODE_API_CALL(env, napi_reference_ref(env, refValue, &refCount)); + return ToUInt32Value(env, refCount); +} + +static napi_value Unref(napi_env env, napi_callback_info info) { + napi_ref refValue; + NODE_API_CALL(env, GetRef(env, info, &refValue)); + uint32_t refCount; + NODE_API_CALL(env, napi_reference_unref(env, refValue, &refCount)); + return ToUInt32Value(env, refCount); +} + +static napi_value DeleteRef(napi_env env, napi_callback_info info) { + napi_ref refValue; + NODE_API_CALL(env, GetRef(env, info, &refValue)); + NODE_API_CALL(env, napi_delete_reference(env, refValue)); + return NULL; +} + +static napi_value AddFinalizer(napi_env env, napi_callback_info info) { + napi_value obj; + NODE_API_CALL(env, GetArgValue(env, info, &obj)); + + napi_valuetype valueType; + NODE_API_CALL(env, napi_typeof(env, obj, &valueType)); + NODE_API_ASSERT(env, valueType == napi_object, "Argument must be an object."); + + NODE_API_CALL(env, napi_add_finalizer(env, obj, NULL, &Finalize, NULL, NULL)); + return NULL; +} + +static napi_value GetFinalizeCount(napi_env env, napi_callback_info info) { + return ToUInt32Value(env, finalizeCount); +} + +EXTERN_C_START + +NAPI_MODULE_INIT() { + finalizeCount = 0; + NODE_API_CALL(env, InitRefArray(env)); + + napi_property_descriptor properties[] = { + DECLARE_NODE_API_PROPERTY("createExternal", CreateExternal), + DECLARE_NODE_API_PROPERTY("createRef", CreateRef), + DECLARE_NODE_API_PROPERTY("getRefValue", GetRefValue), + DECLARE_NODE_API_PROPERTY("ref", Ref), + DECLARE_NODE_API_PROPERTY("unref", Unref), + DECLARE_NODE_API_PROPERTY("deleteRef", DeleteRef), + DECLARE_NODE_API_PROPERTY("addFinalizer", AddFinalizer), + DECLARE_NODE_API_PROPERTY("getFinalizeCount", GetFinalizeCount), + }; + + NODE_API_CALL( + env, + napi_define_properties( + env, exports, sizeof(properties) / sizeof(*properties), properties)); + + return exports; +} + +EXTERN_C_END diff --git a/test/node-api/test_reference_obj_only/binding.gyp b/test/node-api/test_reference_obj_only/binding.gyp new file mode 100644 index 00000000000000..8165ac9092d594 --- /dev/null +++ b/test/node-api/test_reference_obj_only/binding.gyp @@ -0,0 +1,9 @@ +{ + "targets": [ + { + "target_name": "test_reference_obj_only", + "sources": [ "test_reference_obj_only.c" ], + "defines": [ "NAPI_VERSION=8" ] + } + ] +} diff --git a/test/node-api/test_reference_obj_only/test.js b/test/node-api/test_reference_obj_only/test.js new file mode 100644 index 00000000000000..a1f34c8ad0a7df --- /dev/null +++ b/test/node-api/test_reference_obj_only/test.js @@ -0,0 +1,106 @@ +'use strict'; +// Flags: --expose-gc +// +// Testing API calls for references to only object, function, and symbol types. +// This is the reference behavior without the napi_feature_reference_all_types +// feature enabled. +// This test uses NAPI_MODULE_INIT macro to initialize module. +// +const { gcUntil, buildType } = require('../../common'); +const assert = require('assert'); +const addon = require(`./build/${buildType}/test_reference_obj_only`); + +async function runTests() { + let allEntries = []; + + (() => { + // Create values of all napi_valuetype types. + const undefinedValue = undefined; + const nullValue = null; + const booleanValue = false; + const numberValue = 42; + const stringValue = 'test_string'; + const symbolValue = Symbol.for('test_symbol'); + const objectValue = { x: 1, y: 2 }; + const functionValue = (x, y) => x + y; + const externalValue = addon.createExternal(); + const bigintValue = 9007199254740991n; + + allEntries = [ + { value: undefinedValue, canBeWeak: false, canBeRef: false }, + { value: nullValue, canBeWeak: false, canBeRef: false }, + { value: booleanValue, canBeWeak: false, canBeRef: false }, + { value: numberValue, canBeWeak: false, canBeRef: false }, + { value: stringValue, canBeWeak: false, canBeRef: false }, + { value: symbolValue, canBeWeak: false, canBeRef: true }, + { value: objectValue, canBeWeak: true, canBeRef: true }, + { value: functionValue, canBeWeak: true, canBeRef: true }, + { value: externalValue, canBeWeak: true, canBeRef: true }, + { value: bigintValue, canBeWeak: false, canBeRef: false }, + ]; + + // Go over all values of different types, create strong ref values for + // them, read the stored values, and check how the ref count works. + for (const entry of allEntries) { + if (entry.canBeRef) { + const index = addon.createRef(entry.value); + const refValue = addon.getRefValue(index); + assert.strictEqual(entry.value, refValue); + assert.strictEqual(addon.ref(index), 2); + assert.strictEqual(addon.unref(index), 1); + assert.strictEqual(addon.unref(index), 0); + } else { + assert.throws(() => { addon.createRef(entry.value); }, + { + name: 'Error', + message: 'Invalid argument', + }); + } + } + + // The references become weak pointers when the ref count is 0. + // The old reference were supported for objects, external objects, + // functions, and symbols. + // Here we know that the GC is not run yet because the values are + // still in the allEntries array. + allEntries.forEach((entry, index) => { + if (entry.canBeRef) { + assert.strictEqual(addon.getRefValue(index), entry.value); + } + // Set to undefined to allow GC collect the value. + entry.value = undefined; + }); + + // To check that GC pass is done. + const objWithFinalizer = {}; + addon.addFinalizer(objWithFinalizer); + })(); + + assert.strictEqual(addon.getFinalizeCount(), 0); + await gcUntil('Wait until a finalizer is called', + () => (addon.getFinalizeCount() === 1)); + + // Create and call finalizer again to make sure that we had another GC pass. + (() => { + const objWithFinalizer = {}; + addon.addFinalizer(objWithFinalizer); + })(); + await gcUntil('Wait until a finalizer is called again', + () => (addon.getFinalizeCount() === 2)); + + // After GC and finalizers run, all values that support weak reference + // semantic must return undefined value. + // It also includes the value at index 0 because it is the undefined value. + // Other value types are not collected by GC. + allEntries.forEach((entry, index) => { + if (entry.canBeRef) { + if (entry.canBeWeak || index === 0) { + assert.strictEqual(addon.getRefValue(index), undefined); + } else { + assert.notStrictEqual(addon.getRefValue(index), undefined); + } + addon.deleteRef(index); + } + }); +} +runTests(); diff --git a/test/node-api/test_reference_obj_only/test_reference_obj_only.c b/test/node-api/test_reference_obj_only/test_reference_obj_only.c new file mode 100644 index 00000000000000..332baa7e8433d2 --- /dev/null +++ b/test/node-api/test_reference_obj_only/test_reference_obj_only.c @@ -0,0 +1,175 @@ +#define NAPI_EXPERIMENTAL +#include +#include "../../js-native-api/common.h" +#include "stdlib.h" + +#define NODE_API_ASSERT_STATUS(env, assertion, message) \ + NODE_API_ASSERT_BASE(env, assertion, message, napi_generic_failure) + +#define NODE_API_CHECK_STATUS(env, the_call) \ + do { \ + napi_status status = (the_call); \ + if (status != napi_ok) { \ + return status; \ + } \ + } while (0) + +static uint32_t finalizeCount = 0; + +static void FreeData(napi_env env, void* data, void* hint) { + NODE_API_ASSERT_RETURN_VOID(env, data != NULL, "Expects non-NULL data."); + free(data); +} + +static void Finalize(napi_env env, void* data, void* hint) { + ++finalizeCount; +} + +static napi_status GetArgValue(napi_env env, + napi_callback_info info, + napi_value* argValue) { + size_t argc = 1; + NODE_API_CHECK_STATUS( + env, napi_get_cb_info(env, info, &argc, argValue, NULL, NULL)); + + NODE_API_ASSERT_STATUS(env, argc == 1, "Expects one arg."); + return napi_ok; +} + +static napi_status GetArgValueAsIndex(napi_env env, + napi_callback_info info, + uint32_t* index) { + napi_value argValue; + NODE_API_CHECK_STATUS(env, GetArgValue(env, info, &argValue)); + + napi_valuetype valueType; + NODE_API_CHECK_STATUS(env, napi_typeof(env, argValue, &valueType)); + NODE_API_ASSERT_STATUS( + env, valueType == napi_number, "Argument must be a number."); + + return napi_get_value_uint32(env, argValue, index); +} + +static napi_status GetRef(napi_env env, + napi_callback_info info, + napi_ref* ref) { + uint32_t index; + NODE_API_CHECK_STATUS(env, GetArgValueAsIndex(env, info, &index)); + + napi_ref* refValues; + NODE_API_CHECK_STATUS(env, napi_get_instance_data(env, (void**)&refValues)); + NODE_API_ASSERT_STATUS(env, refValues != NULL, "Cannot get instance data."); + + *ref = refValues[index]; + return napi_ok; +} + +static napi_value ToUInt32Value(napi_env env, uint32_t value) { + napi_value result; + NODE_API_CALL(env, napi_create_uint32(env, value, &result)); + return result; +} + +static napi_status InitRefArray(napi_env env) { + // valueRefs array has one entry per napi_valuetype + napi_ref* valueRefs = malloc(sizeof(napi_ref) * ((int)napi_bigint + 1)); + return napi_set_instance_data(env, valueRefs, &FreeData, NULL); +} + +static napi_value CreateExternal(napi_env env, napi_callback_info info) { + napi_value result; + int* data = (int*)malloc(sizeof(int)); + *data = 42; + NODE_API_CALL(env, napi_create_external(env, data, &FreeData, NULL, &result)); + return result; +} + +static napi_value CreateRef(napi_env env, napi_callback_info info) { + napi_value argValue; + NODE_API_CALL(env, GetArgValue(env, info, &argValue)); + + napi_valuetype valueType; + NODE_API_CALL(env, napi_typeof(env, argValue, &valueType)); + uint32_t index = (uint32_t)valueType; + + napi_ref* valueRefs; + NODE_API_CALL(env, napi_get_instance_data(env, (void**)&valueRefs)); + NODE_API_CALL(env, + napi_create_reference(env, argValue, 1, valueRefs + index)); + + return ToUInt32Value(env, index); +} + +static napi_value GetRefValue(napi_env env, napi_callback_info info) { + napi_ref refValue; + NODE_API_CALL(env, GetRef(env, info, &refValue)); + napi_value value; + NODE_API_CALL(env, napi_get_reference_value(env, refValue, &value)); + return value; +} + +static napi_value Ref(napi_env env, napi_callback_info info) { + napi_ref refValue; + NODE_API_CALL(env, GetRef(env, info, &refValue)); + uint32_t refCount; + NODE_API_CALL(env, napi_reference_ref(env, refValue, &refCount)); + return ToUInt32Value(env, refCount); +} + +static napi_value Unref(napi_env env, napi_callback_info info) { + napi_ref refValue; + NODE_API_CALL(env, GetRef(env, info, &refValue)); + uint32_t refCount; + NODE_API_CALL(env, napi_reference_unref(env, refValue, &refCount)); + return ToUInt32Value(env, refCount); +} + +static napi_value DeleteRef(napi_env env, napi_callback_info info) { + napi_ref refValue; + NODE_API_CALL(env, GetRef(env, info, &refValue)); + NODE_API_CALL(env, napi_delete_reference(env, refValue)); + return NULL; +} + +static napi_value AddFinalizer(napi_env env, napi_callback_info info) { + napi_value obj; + NODE_API_CALL(env, GetArgValue(env, info, &obj)); + + napi_valuetype valueType; + NODE_API_CALL(env, napi_typeof(env, obj, &valueType)); + NODE_API_ASSERT(env, valueType == napi_object, "Argument must be an object."); + + NODE_API_CALL(env, napi_add_finalizer(env, obj, NULL, &Finalize, NULL, NULL)); + return NULL; +} + +static napi_value GetFinalizeCount(napi_env env, napi_callback_info info) { + return ToUInt32Value(env, finalizeCount); +} + +EXTERN_C_START + +NAPI_MODULE_INIT() { + finalizeCount = 0; + NODE_API_CALL(env, InitRefArray(env)); + + napi_property_descriptor properties[] = { + DECLARE_NODE_API_PROPERTY("createExternal", CreateExternal), + DECLARE_NODE_API_PROPERTY("createRef", CreateRef), + DECLARE_NODE_API_PROPERTY("getRefValue", GetRefValue), + DECLARE_NODE_API_PROPERTY("ref", Ref), + DECLARE_NODE_API_PROPERTY("unref", Unref), + DECLARE_NODE_API_PROPERTY("deleteRef", DeleteRef), + DECLARE_NODE_API_PROPERTY("addFinalizer", AddFinalizer), + DECLARE_NODE_API_PROPERTY("getFinalizeCount", GetFinalizeCount), + }; + + NODE_API_CALL( + env, + napi_define_properties( + env, exports, sizeof(properties) / sizeof(*properties), properties)); + + return exports; +} + +EXTERN_C_END From c8c6f565b85791ef748cd2b6c41f8f64323c9f7c Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 7 Apr 2023 09:07:20 -0700 Subject: [PATCH 02/15] fix node_api.cc formatting --- src/node_api.cc | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/node_api.cc b/src/node_api.cc index f79905330202df..e5d4deaef4022f 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -619,12 +619,11 @@ static void napi_module_register_cb(v8::Local exports, v8::Local context, void* priv) { const napi_module* napi_mod = static_cast(priv); - napi_module_register_by_symbol( - exports, - module, - context, - napi_mod->nm_register_func, - NAPI_DEFAULT_MODULE_API_VERSION); + napi_module_register_by_symbol(exports, + module, + context, + napi_mod->nm_register_func, + NAPI_DEFAULT_MODULE_API_VERSION); } void napi_module_register_by_symbol(v8::Local exports, From 4c11c9298dc798493223250c12326f232a7ce1d0 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 14 Apr 2023 06:05:48 -0700 Subject: [PATCH 03/15] merge tests --- .../test_reference_all_types/binding.gyp | 9 - .../node-api/test_reference_all_types/test.js | 90 --------- .../binding.gyp | 14 ++ .../test.js | 46 ++--- .../test_reference_by_node_api_version.c} | 0 .../test_reference_obj_only/binding.gyp | 9 - .../test_reference_obj_only.c | 175 ------------------ 7 files changed, 38 insertions(+), 305 deletions(-) delete mode 100644 test/node-api/test_reference_all_types/binding.gyp delete mode 100644 test/node-api/test_reference_all_types/test.js create mode 100644 test/node-api/test_reference_by_node_api_version/binding.gyp rename test/node-api/{test_reference_obj_only => test_reference_by_node_api_version}/test.js (67%) rename test/node-api/{test_reference_all_types/test_reference_all_types.c => test_reference_by_node_api_version/test_reference_by_node_api_version.c} (100%) delete mode 100644 test/node-api/test_reference_obj_only/binding.gyp delete mode 100644 test/node-api/test_reference_obj_only/test_reference_obj_only.c diff --git a/test/node-api/test_reference_all_types/binding.gyp b/test/node-api/test_reference_all_types/binding.gyp deleted file mode 100644 index f58807b0e5baf1..00000000000000 --- a/test/node-api/test_reference_all_types/binding.gyp +++ /dev/null @@ -1,9 +0,0 @@ -{ - "targets": [ - { - "target_name": "test_reference_all_types", - "sources": [ "test_reference_all_types.c" ], - "defines": [ 'NAPI_EXPERIMENTAL' ] - } - ] -} diff --git a/test/node-api/test_reference_all_types/test.js b/test/node-api/test_reference_all_types/test.js deleted file mode 100644 index caee3f4d803125..00000000000000 --- a/test/node-api/test_reference_all_types/test.js +++ /dev/null @@ -1,90 +0,0 @@ -'use strict'; -// Flags: --expose-gc -// -// Testing API calls for references to all value types. -// This test uses NAPI_MODULE_INIT macro to initialize module. -// -const { gcUntil, buildType } = require('../../common'); -const assert = require('assert'); -const addon = require(`./build/${buildType}/test_reference_all_types`); - -async function runTests() { - let allEntries = []; - - (() => { - // Create values of all napi_valuetype types. - const undefinedValue = undefined; - const nullValue = null; - const booleanValue = false; - const numberValue = 42; - const stringValue = 'test_string'; - const symbolValue = Symbol.for('test_symbol'); - const objectValue = { x: 1, y: 2 }; - const functionValue = (x, y) => x + y; - const externalValue = addon.createExternal(); - const bigintValue = 9007199254740991n; - - allEntries = [ - { value: undefinedValue, canBeWeak: false }, - { value: nullValue, canBeWeak: false }, - { value: booleanValue, canBeWeak: false }, - { value: numberValue, canBeWeak: false }, - { value: stringValue, canBeWeak: false }, - { value: symbolValue, canBeWeak: false }, - { value: objectValue, canBeWeak: true }, - { value: functionValue, canBeWeak: true }, - { value: externalValue, canBeWeak: true }, - { value: bigintValue, canBeWeak: false }, - ]; - - // Go over all values of different types, create strong ref values for - // them, read the stored values, and check how the ref count works. - for (const entry of allEntries) { - const index = addon.createRef(entry.value); - const refValue = addon.getRefValue(index); - assert.strictEqual(entry.value, refValue); - assert.strictEqual(addon.ref(index), 2); - assert.strictEqual(addon.unref(index), 1); - assert.strictEqual(addon.unref(index), 0); - } - - // The references become weak pointers when the ref count is 0. - // Here we know that the GC is not run yet because the values are - // still in the allEntries array. - allEntries.forEach((entry, index) => { - assert.strictEqual(addon.getRefValue(index), entry.value); - // Set to undefined to allow GC collect the value. - entry.value = undefined; - }); - - // To check that GC pass is done. - const objWithFinalizer = {}; - addon.addFinalizer(objWithFinalizer); - })(); - - assert.strictEqual(addon.getFinalizeCount(), 0); - await gcUntil('Wait until a finalizer is called', - () => (addon.getFinalizeCount() === 1)); - - // Create and call finalizer again to make sure that we had another GC pass. - (() => { - const objWithFinalizer = {}; - addon.addFinalizer(objWithFinalizer); - })(); - await gcUntil('Wait until a finalizer is called again', - () => (addon.getFinalizeCount() === 2)); - - // After GC and finalizers run, all values that support weak reference - // semantic must return undefined value. - // It also includes the value at index 0 because it is the undefined value. - // Other value types are not collected by GC. - allEntries.forEach((entry, index) => { - if (entry.canBeWeak || index === 0) { - assert.strictEqual(addon.getRefValue(index), undefined); - } else { - assert.notStrictEqual(addon.getRefValue(index), undefined); - } - addon.deleteRef(index); - }); -} -runTests(); diff --git a/test/node-api/test_reference_by_node_api_version/binding.gyp b/test/node-api/test_reference_by_node_api_version/binding.gyp new file mode 100644 index 00000000000000..11d8842abbec6d --- /dev/null +++ b/test/node-api/test_reference_by_node_api_version/binding.gyp @@ -0,0 +1,14 @@ +{ + "targets": [ + { + "target_name": "test_reference_all_types", + "sources": [ "test_reference_by_node_api_version.c" ], + "defines": [ "NAPI_EXPERIMENTAL" ], + }, + { + "target_name": "test_reference_obj_only", + "sources": [ "test_reference_by_node_api_version.c" ], + "defines": [ "NAPI_VERSION=8" ], + } + ] +} \ No newline at end of file diff --git a/test/node-api/test_reference_obj_only/test.js b/test/node-api/test_reference_by_node_api_version/test.js similarity index 67% rename from test/node-api/test_reference_obj_only/test.js rename to test/node-api/test_reference_by_node_api_version/test.js index a1f34c8ad0a7df..c34a10a7a786c0 100644 --- a/test/node-api/test_reference_obj_only/test.js +++ b/test/node-api/test_reference_by_node_api_version/test.js @@ -1,16 +1,18 @@ 'use strict'; // Flags: --expose-gc // -// Testing API calls for references to only object, function, and symbol types. -// This is the reference behavior without the napi_feature_reference_all_types -// feature enabled. -// This test uses NAPI_MODULE_INIT macro to initialize module. +// Testing API calls for Node-API references. +// We compare their behavior between Node-API version 8 and later. +// In version 8 references can be created only for object, function, +// and symbol types. While in newer versions they can be created for +// any value type. // const { gcUntil, buildType } = require('../../common'); const assert = require('assert'); -const addon = require(`./build/${buildType}/test_reference_obj_only`); +const addon_v8 = require(`./build/${buildType}/test_reference_obj_only`); +const addon_new = require(`./build/${buildType}/test_reference_all_types`); -async function runTests() { +async function runTests(addon, isVersion8) { let allEntries = []; (() => { @@ -27,22 +29,22 @@ async function runTests() { const bigintValue = 9007199254740991n; allEntries = [ - { value: undefinedValue, canBeWeak: false, canBeRef: false }, - { value: nullValue, canBeWeak: false, canBeRef: false }, - { value: booleanValue, canBeWeak: false, canBeRef: false }, - { value: numberValue, canBeWeak: false, canBeRef: false }, - { value: stringValue, canBeWeak: false, canBeRef: false }, - { value: symbolValue, canBeWeak: false, canBeRef: true }, - { value: objectValue, canBeWeak: true, canBeRef: true }, - { value: functionValue, canBeWeak: true, canBeRef: true }, - { value: externalValue, canBeWeak: true, canBeRef: true }, - { value: bigintValue, canBeWeak: false, canBeRef: false }, + { value: undefinedValue, canBeWeak: false, canBeRefV8: false }, + { value: nullValue, canBeWeak: false, canBeRefV8: false }, + { value: booleanValue, canBeWeak: false, canBeRefV8: false }, + { value: numberValue, canBeWeak: false, canBeRefV8: false }, + { value: stringValue, canBeWeak: false, canBeRefV8: false }, + { value: symbolValue, canBeWeak: false, canBeRefV8: true }, + { value: objectValue, canBeWeak: true, canBeRefV8: true }, + { value: functionValue, canBeWeak: true, canBeRefV8: true }, + { value: externalValue, canBeWeak: true, canBeRefV8: true }, + { value: bigintValue, canBeWeak: false, canBeRefV8: false }, ]; // Go over all values of different types, create strong ref values for // them, read the stored values, and check how the ref count works. for (const entry of allEntries) { - if (entry.canBeRef) { + if (!isVersion8 || entry.canBeRefV8) { const index = addon.createRef(entry.value); const refValue = addon.getRefValue(index); assert.strictEqual(entry.value, refValue); @@ -59,12 +61,10 @@ async function runTests() { } // The references become weak pointers when the ref count is 0. - // The old reference were supported for objects, external objects, - // functions, and symbols. // Here we know that the GC is not run yet because the values are // still in the allEntries array. allEntries.forEach((entry, index) => { - if (entry.canBeRef) { + if (!isVersion8 || entry.canBeRefV8) { assert.strictEqual(addon.getRefValue(index), entry.value); } // Set to undefined to allow GC collect the value. @@ -93,7 +93,7 @@ async function runTests() { // It also includes the value at index 0 because it is the undefined value. // Other value types are not collected by GC. allEntries.forEach((entry, index) => { - if (entry.canBeRef) { + if (!isVersion8 || entry.canBeRefV8) { if (entry.canBeWeak || index === 0) { assert.strictEqual(addon.getRefValue(index), undefined); } else { @@ -103,4 +103,6 @@ async function runTests() { } }); } -runTests(); + +runTests(addon_v8, /* isVersion8 */ true); +runTests(addon_new, /* isVersion8 */ false); diff --git a/test/node-api/test_reference_all_types/test_reference_all_types.c b/test/node-api/test_reference_by_node_api_version/test_reference_by_node_api_version.c similarity index 100% rename from test/node-api/test_reference_all_types/test_reference_all_types.c rename to test/node-api/test_reference_by_node_api_version/test_reference_by_node_api_version.c diff --git a/test/node-api/test_reference_obj_only/binding.gyp b/test/node-api/test_reference_obj_only/binding.gyp deleted file mode 100644 index 8165ac9092d594..00000000000000 --- a/test/node-api/test_reference_obj_only/binding.gyp +++ /dev/null @@ -1,9 +0,0 @@ -{ - "targets": [ - { - "target_name": "test_reference_obj_only", - "sources": [ "test_reference_obj_only.c" ], - "defines": [ "NAPI_VERSION=8" ] - } - ] -} diff --git a/test/node-api/test_reference_obj_only/test_reference_obj_only.c b/test/node-api/test_reference_obj_only/test_reference_obj_only.c deleted file mode 100644 index 332baa7e8433d2..00000000000000 --- a/test/node-api/test_reference_obj_only/test_reference_obj_only.c +++ /dev/null @@ -1,175 +0,0 @@ -#define NAPI_EXPERIMENTAL -#include -#include "../../js-native-api/common.h" -#include "stdlib.h" - -#define NODE_API_ASSERT_STATUS(env, assertion, message) \ - NODE_API_ASSERT_BASE(env, assertion, message, napi_generic_failure) - -#define NODE_API_CHECK_STATUS(env, the_call) \ - do { \ - napi_status status = (the_call); \ - if (status != napi_ok) { \ - return status; \ - } \ - } while (0) - -static uint32_t finalizeCount = 0; - -static void FreeData(napi_env env, void* data, void* hint) { - NODE_API_ASSERT_RETURN_VOID(env, data != NULL, "Expects non-NULL data."); - free(data); -} - -static void Finalize(napi_env env, void* data, void* hint) { - ++finalizeCount; -} - -static napi_status GetArgValue(napi_env env, - napi_callback_info info, - napi_value* argValue) { - size_t argc = 1; - NODE_API_CHECK_STATUS( - env, napi_get_cb_info(env, info, &argc, argValue, NULL, NULL)); - - NODE_API_ASSERT_STATUS(env, argc == 1, "Expects one arg."); - return napi_ok; -} - -static napi_status GetArgValueAsIndex(napi_env env, - napi_callback_info info, - uint32_t* index) { - napi_value argValue; - NODE_API_CHECK_STATUS(env, GetArgValue(env, info, &argValue)); - - napi_valuetype valueType; - NODE_API_CHECK_STATUS(env, napi_typeof(env, argValue, &valueType)); - NODE_API_ASSERT_STATUS( - env, valueType == napi_number, "Argument must be a number."); - - return napi_get_value_uint32(env, argValue, index); -} - -static napi_status GetRef(napi_env env, - napi_callback_info info, - napi_ref* ref) { - uint32_t index; - NODE_API_CHECK_STATUS(env, GetArgValueAsIndex(env, info, &index)); - - napi_ref* refValues; - NODE_API_CHECK_STATUS(env, napi_get_instance_data(env, (void**)&refValues)); - NODE_API_ASSERT_STATUS(env, refValues != NULL, "Cannot get instance data."); - - *ref = refValues[index]; - return napi_ok; -} - -static napi_value ToUInt32Value(napi_env env, uint32_t value) { - napi_value result; - NODE_API_CALL(env, napi_create_uint32(env, value, &result)); - return result; -} - -static napi_status InitRefArray(napi_env env) { - // valueRefs array has one entry per napi_valuetype - napi_ref* valueRefs = malloc(sizeof(napi_ref) * ((int)napi_bigint + 1)); - return napi_set_instance_data(env, valueRefs, &FreeData, NULL); -} - -static napi_value CreateExternal(napi_env env, napi_callback_info info) { - napi_value result; - int* data = (int*)malloc(sizeof(int)); - *data = 42; - NODE_API_CALL(env, napi_create_external(env, data, &FreeData, NULL, &result)); - return result; -} - -static napi_value CreateRef(napi_env env, napi_callback_info info) { - napi_value argValue; - NODE_API_CALL(env, GetArgValue(env, info, &argValue)); - - napi_valuetype valueType; - NODE_API_CALL(env, napi_typeof(env, argValue, &valueType)); - uint32_t index = (uint32_t)valueType; - - napi_ref* valueRefs; - NODE_API_CALL(env, napi_get_instance_data(env, (void**)&valueRefs)); - NODE_API_CALL(env, - napi_create_reference(env, argValue, 1, valueRefs + index)); - - return ToUInt32Value(env, index); -} - -static napi_value GetRefValue(napi_env env, napi_callback_info info) { - napi_ref refValue; - NODE_API_CALL(env, GetRef(env, info, &refValue)); - napi_value value; - NODE_API_CALL(env, napi_get_reference_value(env, refValue, &value)); - return value; -} - -static napi_value Ref(napi_env env, napi_callback_info info) { - napi_ref refValue; - NODE_API_CALL(env, GetRef(env, info, &refValue)); - uint32_t refCount; - NODE_API_CALL(env, napi_reference_ref(env, refValue, &refCount)); - return ToUInt32Value(env, refCount); -} - -static napi_value Unref(napi_env env, napi_callback_info info) { - napi_ref refValue; - NODE_API_CALL(env, GetRef(env, info, &refValue)); - uint32_t refCount; - NODE_API_CALL(env, napi_reference_unref(env, refValue, &refCount)); - return ToUInt32Value(env, refCount); -} - -static napi_value DeleteRef(napi_env env, napi_callback_info info) { - napi_ref refValue; - NODE_API_CALL(env, GetRef(env, info, &refValue)); - NODE_API_CALL(env, napi_delete_reference(env, refValue)); - return NULL; -} - -static napi_value AddFinalizer(napi_env env, napi_callback_info info) { - napi_value obj; - NODE_API_CALL(env, GetArgValue(env, info, &obj)); - - napi_valuetype valueType; - NODE_API_CALL(env, napi_typeof(env, obj, &valueType)); - NODE_API_ASSERT(env, valueType == napi_object, "Argument must be an object."); - - NODE_API_CALL(env, napi_add_finalizer(env, obj, NULL, &Finalize, NULL, NULL)); - return NULL; -} - -static napi_value GetFinalizeCount(napi_env env, napi_callback_info info) { - return ToUInt32Value(env, finalizeCount); -} - -EXTERN_C_START - -NAPI_MODULE_INIT() { - finalizeCount = 0; - NODE_API_CALL(env, InitRefArray(env)); - - napi_property_descriptor properties[] = { - DECLARE_NODE_API_PROPERTY("createExternal", CreateExternal), - DECLARE_NODE_API_PROPERTY("createRef", CreateRef), - DECLARE_NODE_API_PROPERTY("getRefValue", GetRefValue), - DECLARE_NODE_API_PROPERTY("ref", Ref), - DECLARE_NODE_API_PROPERTY("unref", Unref), - DECLARE_NODE_API_PROPERTY("deleteRef", DeleteRef), - DECLARE_NODE_API_PROPERTY("addFinalizer", AddFinalizer), - DECLARE_NODE_API_PROPERTY("getFinalizeCount", GetFinalizeCount), - }; - - NODE_API_CALL( - env, - napi_define_properties( - env, exports, sizeof(properties) / sizeof(*properties), properties)); - - return exports; -} - -EXTERN_C_END From 272ba928d479578a50fb5dd4696ff4e3369185d7 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 14 Apr 2023 06:06:46 -0700 Subject: [PATCH 04/15] use node_api prefix instead of napi for new code --- src/api/environment.cc | 2 +- src/js_native_api_v8.h | 4 ++-- src/node_api.cc | 2 +- src/node_api.h | 10 +++++----- src/node_binding.cc | 6 +++--- src/node_version.h | 2 +- 6 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 0aa5f78a00f4a8..35a4883f3221a4 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -888,7 +888,7 @@ void AddLinkedBinding(Environment* env, module, context, reinterpret_cast(priv), - NAPI_DEFAULT_MODULE_API_VERSION); + NODE_API_DEFAULT_MODULE_API_VERSION); }, name, reinterpret_cast(fn), diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 0b4482b94549b8..d320b25ecf4bb5 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -57,7 +57,7 @@ struct napi_env__ { context_persistent(isolate, context), module_api_version(module_api_version != 0 ? module_api_version - : NAPI_DEFAULT_MODULE_API_VERSION) { + : NODE_API_DEFAULT_MODULE_API_VERSION) { napi_clear_last_error(this); } @@ -149,7 +149,7 @@ struct napi_env__ { int open_callback_scopes = 0; int refs = 1; void* instance_data = nullptr; - int32_t module_api_version = NAPI_DEFAULT_MODULE_API_VERSION; + int32_t module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION; protected: // Should not be deleted directly. Delete with `napi_env__::DeleteMe()` diff --git a/src/node_api.cc b/src/node_api.cc index e5d4deaef4022f..f6f579575cf75b 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -623,7 +623,7 @@ static void napi_module_register_cb(v8::Local exports, module, context, napi_mod->nm_register_func, - NAPI_DEFAULT_MODULE_API_VERSION); + NODE_API_DEFAULT_MODULE_API_VERSION); } void napi_module_register_by_symbol(v8::Local exports, diff --git a/src/node_api.h b/src/node_api.h index b5355f457dae75..4c0356eaccb2fc 100644 --- a/src/node_api.h +++ b/src/node_api.h @@ -30,7 +30,7 @@ struct uv_loop_s; // Forward declaration. typedef napi_value(NAPI_CDECL* napi_addon_register_func)(napi_env env, napi_value exports); -typedef int32_t(NAPI_CDECL* napi_addon_get_api_version_func)(); +typedef int32_t(NAPI_CDECL* node_api_addon_get_api_version_func)(); // Used by deprecated registration method napi_module_register. typedef struct napi_module { @@ -55,18 +55,18 @@ typedef struct napi_module { #define NAPI_MODULE_INITIALIZER_BASE napi_register_module_v #endif -#define NAPI_MODULE_GET_API_VERSION_BASE napi_module_get_api_version_v +#define NODE_API_MODULE_GET_API_VERSION_BASE node_api_module_get_api_version_v #define NAPI_MODULE_INITIALIZER \ NAPI_MODULE_INITIALIZER_X(NAPI_MODULE_INITIALIZER_BASE, NAPI_MODULE_VERSION) -#define NAPI_MODULE_GET_API_VERSION \ - NAPI_MODULE_INITIALIZER_X(NAPI_MODULE_GET_API_VERSION_BASE, \ +#define NODE_API_MODULE_GET_API_VERSION \ + NAPI_MODULE_INITIALIZER_X(NODE_API_MODULE_GET_API_VERSION_BASE, \ NAPI_MODULE_VERSION) #define NAPI_MODULE_INIT() \ EXTERN_C_START \ - NAPI_MODULE_EXPORT int32_t NAPI_MODULE_GET_API_VERSION() { \ + NAPI_MODULE_EXPORT int32_t NODE_API_MODULE_GET_API_VERSION() { \ return NAPI_VERSION; \ } \ NAPI_MODULE_EXPORT napi_value NAPI_MODULE_INITIALIZER(napi_env env, \ diff --git a/src/node_binding.cc b/src/node_binding.cc index dfcf59736e33fe..4129f4fcc0a917 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -408,10 +408,10 @@ inline napi_addon_register_func GetNapiInitializerCallback(DLib* dlib) { dlib->GetSymbolAddress(name)); } -inline napi_addon_get_api_version_func GetNapiAddonGetApiVersionCallback( +inline node_api_addon_get_api_version_func GetNapiAddonGetApiVersionCallback( DLib* dlib) { - return reinterpret_cast( - dlib->GetSymbolAddress(STRINGIFY(NAPI_MODULE_GET_API_VERSION))); + return reinterpret_cast( + dlib->GetSymbolAddress(STRINGIFY(NODE_API_MODULE_GET_API_VERSION))); } // DLOpen is process.dlopen(module, filename, flags). diff --git a/src/node_version.h b/src/node_version.h index 3f93a18ef09525..dcfd4144c0f067 100644 --- a/src/node_version.h +++ b/src/node_version.h @@ -97,6 +97,6 @@ // The NAPI_VERSION used by default by Node API modules if it is not explicitly // specified. It must be always 8. -#define NAPI_DEFAULT_MODULE_API_VERSION 8 +#define NODE_API_DEFAULT_MODULE_API_VERSION 8 #endif // SRC_NODE_VERSION_H_ From f297f674ed00ff64446ea21725567f394392e758 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 14 Apr 2023 06:59:38 -0700 Subject: [PATCH 05/15] update `napi_ref` related docs --- doc/api/n-api.md | 71 +++++++++++++++++++++++++----------------------- 1 file changed, 37 insertions(+), 34 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 42598c6fdbb5ef..f6cf0442421449 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -1639,25 +1639,26 @@ If it is called more than once an error will be returned. This API can be called even if there is a pending JavaScript exception. -### References to objects with a lifespan longer than that of the native method +### References to values with a lifespan longer than that of the native method -In some cases an addon will need to be able to create and reference objects +In some cases, an addon will need to be able to create and reference values with a lifespan longer than that of a single native method invocation. For example, to create a constructor and later use that constructor -in a request to creates instances, it must be possible to reference +in a request to create instances, it must be possible to reference the constructor object across many different instance creation requests. This would not be possible with a normal handle returned as a `napi_value` as described in the earlier section. The lifespan of a normal handle is managed by scopes and all scopes must be closed before the end of a native method. -Node-API provides methods to create persistent references to an object. -Each persistent reference has an associated count with a value of 0 -or higher. The count determines if the reference will keep -the corresponding object live. References with a count of 0 do not -prevent the object from being collected and are often called 'weak' -references. Any count greater than 0 will prevent the object -from being collected. +Node-API provides methods to create persistent references to values. +Each reference has an associated count with a value of 0 or higher. +The count determines if the reference will keep the corresponding value alive. +References with a count of 0 do not prevent values of object types (object, +function, external) from being collected and are often called 'weak' +references. Values of non-object types cannot be collected if a reference +has count 0 because they do not support the 'weak' object semantic. +Any count greater than 0 will prevent the values from being collected. References can be created with an initial reference count. The count can then be modified through [`napi_reference_ref`][] and @@ -1668,21 +1669,24 @@ will return `NULL` for the returned `napi_value`. An attempt to call [`napi_reference_ref`][] for a reference whose object has been collected results in an error. +In Node-API version 8 and before we can only create references for a limited +set of value types: object, external, function, and symbol. In newer Node-API +versions the references can be created for any value type. + References must be deleted once they are no longer required by the addon. When -a reference is deleted, it will no longer prevent the corresponding object from -being collected. Failure to delete a persistent reference results in -a 'memory leak' with both the native memory for the persistent reference and -the corresponding object on the heap being retained forever. - -There can be multiple persistent references created which refer to the same -object, each of which will either keep the object live or not based on its -individual count. Multiple persistent references to the same object -can result in unexpectedly keeping alive native memory. The native structures -for a persistent reference must be kept alive until finalizers for the -referenced object are executed. If a new persistent reference is created -for the same object, the finalizers for that object will not be -run and the native memory pointed by the earlier persistent reference -will not be freed. This can be avoided by calling +a reference is deleted, it will no longer prevent the corresponding value from +being collected. If you don't delete a persistent reference, it can cause a +'memory leak'. This means that both the native memory for the persistent +reference and the corresponding value on the heap will be retained forever. + +You can create multiple persistent references that refer to the same value. +Each reference will keep the value alive or not based on its individual count. +If there are multiple persistent references to the same value, it can +unexpectedly keep alive native memory. The native structures for a persistent +reference must be kept alive until finalizers for the referenced value are +executed. If you create a new persistent reference for the same value, the +finalizers for that value will not be run and the native memory pointed by +the earlier persistent reference will not be freed. To avoid this, call `napi_delete_reference` in addition to `napi_reference_unref` when possible. #### `napi_create_reference` @@ -1700,15 +1704,18 @@ NAPI_EXTERN napi_status napi_create_reference(napi_env env, ``` * `[in] env`: The environment that the API is invoked under. -* `[in] value`: `napi_value` representing the `Object` to which we want a - reference. +* `[in] value`: `napi_value` to which we want to create a reference. * `[in] initial_refcount`: Initial reference count for the new reference. * `[out] result`: `napi_ref` pointing to the new reference. Returns `napi_ok` if the API succeeded. This API creates a new reference with the specified reference count -to the `Object` passed in. +to the value passed in. + +In Node-API version 8 and earlier, you could only create a reference for +object, function, external, and symbol value types. In newer Node-API +versions, you can create a reference for any value type. #### `napi_delete_reference` @@ -1787,18 +1794,14 @@ NAPI_EXTERN napi_status napi_get_reference_value(napi_env env, napi_value* result); ``` -the `napi_value passed` in or out of these methods is a handle to the -object to which the reference is related. - * `[in] env`: The environment that the API is invoked under. -* `[in] ref`: `napi_ref` for which we requesting the corresponding `Object`. -* `[out] result`: The `napi_value` for the `Object` referenced by the - `napi_ref`. +* `[in] ref`: `napi_ref` for which we requesting the corresponding value. +* `[out] result`: The `napi_value` referenced by the `napi_ref`. Returns `napi_ok` if the API succeeded. If still valid, this API returns the `napi_value` representing the -JavaScript `Object` associated with the `napi_ref`. Otherwise, result +JavaScript value associated with the `napi_ref`. Otherwise, result will be `NULL`. ### Cleanup on exit of the current Node.js environment From b4ae2a6446de950932a9c213a54d5af457c1c42e Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Tue, 18 Apr 2023 07:54:58 -0700 Subject: [PATCH 06/15] release non-object types from napi_ref on ref count 0 --- doc/api/n-api.md | 9 +++++---- src/js_native_api_v8.cc | 11 ++++++++--- src/js_native_api_v8.h | 1 + test/js-native-api/test_reference/test.js | 11 +++++++++-- .../test_reference_by_node_api_version/test.js | 17 ++++++++--------- 5 files changed, 31 insertions(+), 18 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index f6cf0442421449..7fd840b02edac7 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -1654,10 +1654,11 @@ method. Node-API provides methods to create persistent references to values. Each reference has an associated count with a value of 0 or higher. The count determines if the reference will keep the corresponding value alive. -References with a count of 0 do not prevent values of object types (object, -function, external) from being collected and are often called 'weak' -references. Values of non-object types cannot be collected if a reference -has count 0 because they do not support the 'weak' object semantic. +References with a count of 0 do not prevent values from being collected. +Values of object types (object, function, external) are becoming 'weak' +references and can still be accessed while they are not collected. +Values of non-object types are released when the count becomes 0 +and cannot be accessed from the reference any more. Any count greater than 0 will prevent the values from being collected. References can be created with an initial reference count. The count can diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index 4186ec02f5231c..abdde7dd497e47 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -551,7 +551,8 @@ void RefBase::Finalize() { template Reference::Reference(napi_env env, v8::Local value, Args&&... args) : RefBase(env, std::forward(args)...), - persistent_(env->isolate, value) { + persistent_(env->isolate, value), + can_be_weak_(value->IsObject()) { if (RefCount() == 0) { SetWeak(); } @@ -585,7 +586,7 @@ uint32_t Reference::Ref() { return 0; } uint32_t refcount = RefBase::Ref(); - if (refcount == 1) { + if (refcount == 1 && can_be_weak_) { persistent_.ClearWeak(); } return refcount; @@ -625,7 +626,11 @@ void Reference::Finalize() { // Mark the reference as weak and eligible for collection // by the gc. void Reference::SetWeak() { - persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); + if (can_be_weak_) { + persistent_.SetWeak(this, WeakCallback, v8::WeakCallbackType::kParameter); + } else { + persistent_.Reset(); + } } // The N-API finalizer callback may make calls into the engine. V8's heap is diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index d320b25ecf4bb5..b236a16415d80b 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -425,6 +425,7 @@ class Reference : public RefBase { void SetWeak(); v8impl::Persistent persistent_; + bool can_be_weak_; }; } // end of namespace v8impl diff --git a/test/js-native-api/test_reference/test.js b/test/js-native-api/test_reference/test.js index 6f128b788706cd..5c36dd8bdd210c 100644 --- a/test/js-native-api/test_reference/test.js +++ b/test/js-native-api/test_reference/test.js @@ -17,13 +17,20 @@ async function runTests() { (() => { const symbol = test_reference.createSymbol('testSym'); test_reference.createReference(symbol, 0); + assert.strictEqual(test_reference.referenceValue, undefined); + })(); + test_reference.deleteReference(); + + (() => { + const symbol = test_reference.createSymbol('testSym'); + test_reference.createReference(symbol, 1); assert.strictEqual(test_reference.referenceValue, symbol); })(); test_reference.deleteReference(); (() => { const symbol = test_reference.createSymbolFor('testSymFor'); - test_reference.createReference(symbol, 0); + test_reference.createReference(symbol, 1); assert.strictEqual(test_reference.referenceValue, symbol); assert.strictEqual(test_reference.referenceValue, Symbol.for('testSymFor')); })(); @@ -31,7 +38,7 @@ async function runTests() { (() => { const symbol = test_reference.createSymbolForEmptyString(); - test_reference.createReference(symbol, 0); + test_reference.createReference(symbol, 1); assert.strictEqual(test_reference.referenceValue, symbol); assert.strictEqual(test_reference.referenceValue, Symbol.for('')); })(); diff --git a/test/node-api/test_reference_by_node_api_version/test.js b/test/node-api/test_reference_by_node_api_version/test.js index c34a10a7a786c0..fe58e0c81be74e 100644 --- a/test/node-api/test_reference_by_node_api_version/test.js +++ b/test/node-api/test_reference_by_node_api_version/test.js @@ -60,12 +60,17 @@ async function runTests(addon, isVersion8) { } } - // The references become weak pointers when the ref count is 0. + // When the reference count is zero, then object types become weak pointers + // and other types are released. // Here we know that the GC is not run yet because the values are // still in the allEntries array. allEntries.forEach((entry, index) => { if (!isVersion8 || entry.canBeRefV8) { - assert.strictEqual(addon.getRefValue(index), entry.value); + if (entry.canBeWeak) { + assert.strictEqual(addon.getRefValue(index), entry.value); + } else { + assert.strictEqual(addon.getRefValue(index), undefined); + } } // Set to undefined to allow GC collect the value. entry.value = undefined; @@ -90,15 +95,9 @@ async function runTests(addon, isVersion8) { // After GC and finalizers run, all values that support weak reference // semantic must return undefined value. - // It also includes the value at index 0 because it is the undefined value. - // Other value types are not collected by GC. allEntries.forEach((entry, index) => { if (!isVersion8 || entry.canBeRefV8) { - if (entry.canBeWeak || index === 0) { - assert.strictEqual(addon.getRefValue(index), undefined); - } else { - assert.notStrictEqual(addon.getRefValue(index), undefined); - } + assert.strictEqual(addon.getRefValue(index), undefined); addon.deleteRef(index); } }); From fe46deeab06fcf27a269467af201d197ba78ca1a Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Tue, 18 Apr 2023 08:01:52 -0700 Subject: [PATCH 07/15] address PR feedback --- doc/api/n-api.md | 2 +- test/node-api/test_reference_by_node_api_version/test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 7fd840b02edac7..e997a3b2be3982 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -1796,7 +1796,7 @@ NAPI_EXTERN napi_status napi_get_reference_value(napi_env env, ``` * `[in] env`: The environment that the API is invoked under. -* `[in] ref`: `napi_ref` for which we requesting the corresponding value. +* `[in] ref`: `napi_ref` for which we are requesting the corresponding value. * `[out] result`: The `napi_value` referenced by the `napi_ref`. Returns `napi_ok` if the API succeeded. diff --git a/test/node-api/test_reference_by_node_api_version/test.js b/test/node-api/test_reference_by_node_api_version/test.js index fe58e0c81be74e..5ddae18897b375 100644 --- a/test/node-api/test_reference_by_node_api_version/test.js +++ b/test/node-api/test_reference_by_node_api_version/test.js @@ -4,7 +4,7 @@ // Testing API calls for Node-API references. // We compare their behavior between Node-API version 8 and later. // In version 8 references can be created only for object, function, -// and symbol types. While in newer versions they can be created for +// and symbol types, while in newer versions they can be created for // any value type. // const { gcUntil, buildType } = require('../../common'); From 3748a4e8611e0d499961f4c3ddfee29276420857 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Thu, 20 Apr 2023 22:26:26 -0700 Subject: [PATCH 08/15] address PR feedback --- src/api/environment.cc | 35 +++----- src/js_native_api_v8.h | 4 +- src/node.h | 3 +- src/node_api.cc | 11 +-- src/node_api.h | 3 +- src/node_binding.cc | 2 +- test/cctest/test_linked_binding.cc | 89 +++++++++++++++++-- test/cctest/test_node_api.cc | 2 +- .../test_reference_by_node_api_version.c | 1 - 9 files changed, 110 insertions(+), 40 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 35a4883f3221a4..3ae325129b1172 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -872,29 +872,20 @@ void AddLinkedBinding(Environment* env, void AddLinkedBinding(Environment* env, const char* name, - napi_addon_register_func fn) { - node_module mod = { - -1, - NM_F_LINKED, - nullptr, // nm_dso_handle - nullptr, // nm_filename - nullptr, // nm_register_func - [](v8::Local exports, - v8::Local module, - v8::Local context, - void* priv) { - napi_module_register_by_symbol( - exports, - module, - context, - reinterpret_cast(priv), - NODE_API_DEFAULT_MODULE_API_VERSION); - }, - name, - reinterpret_cast(fn), - nullptr // nm_link + napi_addon_register_func fn, + int32_t module_api_version) { + // We never free this allocated structure since we never unregister modules. + napi_module* mod = new napi_module{ + 1, // nm_version + 0, // nm_flags + nullptr, // nm_filename + fn, // nm_register_func + name, // nm_modname + nullptr, // nm_priv + module_api_version, // nm_api_version + {0}, // reserved }; - AddLinkedBinding(env, mod); + AddLinkedBinding(env, *mod); } static std::atomic next_thread_id{0}; diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index b236a16415d80b..2f4705dce67713 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -55,9 +55,7 @@ struct napi_env__ { int32_t module_api_version) : isolate(context->GetIsolate()), context_persistent(isolate, context), - module_api_version(module_api_version != 0 - ? module_api_version - : NODE_API_DEFAULT_MODULE_API_VERSION) { + module_api_version(module_api_version) { napi_clear_last_error(this); } diff --git a/src/node.h b/src/node.h index b484df642db1d8..cd84a05556818c 100644 --- a/src/node.h +++ b/src/node.h @@ -1239,7 +1239,8 @@ NODE_EXTERN void AddLinkedBinding(Environment* env, void* priv); NODE_EXTERN void AddLinkedBinding(Environment* env, const char* name, - napi_addon_register_func fn); + napi_addon_register_func fn, + int32_t module_api_version); /* Registers a callback with the passed-in Environment instance. The callback * is called after the event loop exits, but before the VM is disposed. diff --git a/src/node_api.cc b/src/node_api.cc index f6f579575cf75b..da55a8af37f15f 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -619,11 +619,12 @@ static void napi_module_register_cb(v8::Local exports, v8::Local context, void* priv) { const napi_module* napi_mod = static_cast(priv); - napi_module_register_by_symbol(exports, - module, - context, - napi_mod->nm_register_func, - NODE_API_DEFAULT_MODULE_API_VERSION); + int32_t module_api_version = static_cast(napi_mod->nm_api_version); + if (module_api_version == 0) { + module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION; + } + napi_module_register_by_symbol( + exports, module, context, napi_mod->nm_register_func, module_api_version); } void napi_module_register_by_symbol(v8::Local exports, diff --git a/src/node_api.h b/src/node_api.h index 4c0356eaccb2fc..b4ff67bbe2a7d1 100644 --- a/src/node_api.h +++ b/src/node_api.h @@ -40,7 +40,8 @@ typedef struct napi_module { napi_addon_register_func nm_register_func; const char* nm_modname; void* nm_priv; - void* reserved[4]; + intptr_t nm_api_version; + void* reserved[3]; } napi_module; #define NAPI_MODULE_VERSION 1 diff --git a/src/node_binding.cc b/src/node_binding.cc index 4129f4fcc0a917..cb27a497a3b6d1 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -490,7 +490,7 @@ void DLOpen(const FunctionCallbackInfo& args) { callback(exports, module, context); return true; } else if (auto napi_callback = GetNapiInitializerCallback(dlib)) { - int32_t module_api_version = 0; + int32_t module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION; if (auto get_version = GetNapiAddonGetApiVersionCallback(dlib)) { module_api_version = get_version(); } diff --git a/test/cctest/test_linked_binding.cc b/test/cctest/test_linked_binding.cc index 1a4e6a838b5d72..6de16d5420acd2 100644 --- a/test/cctest/test_linked_binding.cc +++ b/test/cctest/test_linked_binding.cc @@ -132,11 +132,15 @@ TEST_F(LinkedBindingTest, LocallyDefinedLinkedBindingNapiCallbackTest) { const Argv argv; Env test_env{handle_scope, argv}; - AddLinkedBinding(*test_env, "local_linked_napi", InitializeLocalNapiBinding); + AddLinkedBinding(*test_env, + "local_linked_napi_cb", + InitializeLocalNapiBinding, + NAPI_VERSION); v8::Local context = isolate_->GetCurrentContext(); - const char* run_script = "process._linkedBinding('local_linked_napi').hello"; + const char* run_script = + "process._linkedBinding('local_linked_napi_cb').hello"; v8::Local script = v8::Script::Compile( context, @@ -150,6 +154,79 @@ TEST_F(LinkedBindingTest, LocallyDefinedLinkedBindingNapiCallbackTest) { CHECK_EQ(strcmp(*utf8val, "world"), 0); } +static int32_t node_api_version = NODE_API_DEFAULT_MODULE_API_VERSION; + +napi_value InitializeLocalNapiRefBinding(napi_env env, napi_value exports) { + napi_value key, value; + CHECK_EQ( + napi_create_string_utf8(env, "napi_ref_created", NAPI_AUTO_LENGTH, &key), + napi_ok); + + // Starting with Node-API version 9 we can create napi_ref to any value type. + napi_ref ref{}; + CHECK_EQ(napi_create_reference(env, key, 1, &ref), + node_api_version <= 8 ? napi_invalid_arg : napi_ok); + CHECK_EQ(napi_get_boolean(env, ref != nullptr, &value), napi_ok); + CHECK_EQ(napi_set_property(env, exports, key, value), napi_ok); + return nullptr; +} + +// napi_ref in Node-API version 8 cannot accept strings. +TEST_F(LinkedBindingTest, LocallyDefinedLinkedBindingNapiRefVersion8Test) { + node_api_version = 8; + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + Env test_env{handle_scope, argv}; + + AddLinkedBinding(*test_env, + "local_linked_napi_ref_v8", + InitializeLocalNapiRefBinding, + node_api_version); + + v8::Local context = isolate_->GetCurrentContext(); + + const char* run_script = + "process._linkedBinding('local_linked_napi_ref_v8').napi_ref_created"; + v8::Local script = + v8::Script::Compile( + context, + v8::String::NewFromOneByte( + isolate_, reinterpret_cast(run_script)) + .ToLocalChecked()) + .ToLocalChecked(); + v8::Local completion_value = script->Run(context).ToLocalChecked(); + CHECK(completion_value->IsBoolean()); + CHECK(!completion_value.As()->Value()); +} + +// napi_ref in Node-API version after 8 can accept strings. +TEST_F(LinkedBindingTest, LocallyDefinedLinkedBindingNapiRefVersion9Test) { + node_api_version = 9; + const v8::HandleScope handle_scope(isolate_); + const Argv argv; + Env test_env{handle_scope, argv}; + + AddLinkedBinding(*test_env, + "local_linked_napi_ref_v9", + InitializeLocalNapiRefBinding, + node_api_version); + + v8::Local context = isolate_->GetCurrentContext(); + + const char* run_script = + "process._linkedBinding('local_linked_napi_ref_v9').napi_ref_created"; + v8::Local script = + v8::Script::Compile( + context, + v8::String::NewFromOneByte( + isolate_, reinterpret_cast(run_script)) + .ToLocalChecked()) + .ToLocalChecked(); + v8::Local completion_value = script->Run(context).ToLocalChecked(); + CHECK(completion_value->IsBoolean()); + CHECK(completion_value.As()->Value()); +} + napi_value NapiLinkedWithInstanceData(napi_env env, napi_value exports) { int* instance_data = new int(0); CHECK_EQ(napi_set_instance_data( @@ -223,13 +300,15 @@ TEST_F(LinkedBindingTest, const Argv argv; Env test_env{handle_scope, argv}; - AddLinkedBinding( - *test_env, "local_linked_napi_id", NapiLinkedWithInstanceData); + AddLinkedBinding(*test_env, + "local_linked_napi_id_cb", + NapiLinkedWithInstanceData, + NAPI_VERSION); v8::Local context = isolate_->GetCurrentContext(); const char* run_script = - "process._linkedBinding('local_linked_napi_id').hello"; + "process._linkedBinding('local_linked_napi_id_cb').hello"; v8::Local script = v8::Script::Compile( context, diff --git a/test/cctest/test_node_api.cc b/test/cctest/test_node_api.cc index 261e3e368c1012..8921b9d8d373db 100644 --- a/test/cctest/test_node_api.cc +++ b/test/cctest/test_node_api.cc @@ -35,7 +35,7 @@ TEST_F(NodeApiTest, CreateNodeApiEnv) { Local module_obj = Object::New(isolate_); Local exports_obj = Object::New(isolate_); napi_module_register_by_symbol( - exports_obj, module_obj, env->context(), init, 0); + exports_obj, module_obj, env->context(), init, NAPI_VERSION); ASSERT_NE(addon_env, nullptr); node_napi_env internal_env = reinterpret_cast(addon_env); EXPECT_EQ(internal_env->node_env(), env); diff --git a/test/node-api/test_reference_by_node_api_version/test_reference_by_node_api_version.c b/test/node-api/test_reference_by_node_api_version/test_reference_by_node_api_version.c index 332baa7e8433d2..5dcc46d96926c6 100644 --- a/test/node-api/test_reference_by_node_api_version/test_reference_by_node_api_version.c +++ b/test/node-api/test_reference_by_node_api_version/test_reference_by_node_api_version.c @@ -1,4 +1,3 @@ -#define NAPI_EXPERIMENTAL #include #include "../../js-native-api/common.h" #include "stdlib.h" From bd38aaf7d397e7157f2ac30034e31193d6b56456 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 21 Apr 2023 17:22:35 -0700 Subject: [PATCH 09/15] avoid memory leaks reported by ASAN --- src/api/environment.cc | 22 ++++++------- src/node.h | 9 +++--- src/node_api.cc | 52 ++++++++++++++++++++++++++---- src/node_api.h | 3 +- src/node_binding.h | 14 +++++--- src/node_version.h | 2 +- test/cctest/test_linked_binding.cc | 25 ++++++++------ 7 files changed, 88 insertions(+), 39 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 3ae325129b1172..3a369fbcacb5f4 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -874,18 +874,18 @@ void AddLinkedBinding(Environment* env, const char* name, napi_addon_register_func fn, int32_t module_api_version) { - // We never free this allocated structure since we never unregister modules. - napi_module* mod = new napi_module{ - 1, // nm_version - 0, // nm_flags - nullptr, // nm_filename - fn, // nm_register_func - name, // nm_modname - nullptr, // nm_priv - module_api_version, // nm_api_version - {0}, // reserved + node_module mod = { + -1, // nm_version for Node-API + NM_F_LINKED, // nm_flags + nullptr, // nm_dso_handle + nullptr, // nm_filename + nullptr, // nm_register_func + get_node_api_context_register_func(env, module_api_version), + name, // nm_modname + reinterpret_cast(fn), // nm_priv + nullptr // nm_link }; - AddLinkedBinding(env, *mod); + AddLinkedBinding(env, mod); } static std::atomic next_thread_id{0}; diff --git a/src/node.h b/src/node.h index cd84a05556818c..c71a4a7b76039d 100644 --- a/src/node.h +++ b/src/node.h @@ -1237,10 +1237,11 @@ NODE_EXTERN void AddLinkedBinding(Environment* env, const char* name, addon_context_register_func fn, void* priv); -NODE_EXTERN void AddLinkedBinding(Environment* env, - const char* name, - napi_addon_register_func fn, - int32_t module_api_version); +NODE_EXTERN void AddLinkedBinding( + Environment* env, + const char* name, + napi_addon_register_func fn, + int32_t module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION); /* Registers a callback with the passed-in Environment instance. The callback * is called after the event loop exits, but before the VM is disposed. diff --git a/src/node_api.cc b/src/node_api.cc index da55a8af37f15f..f314620f28c67f 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -157,6 +157,17 @@ inline napi_env NewEnv(v8::Local context, int32_t module_api_version) { node_napi_env result; + // Validate module_api_version. + if (module_api_version < NODE_API_DEFAULT_MODULE_API_VERSION) { + module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION; + } else if (module_api_version > NAPI_VERSION && + module_api_version != NAPI_VERSION_EXPERIMENTAL) { + node::Environment* node_env = node::Environment::GetCurrent(context); + CHECK_NOT_NULL(node_env); + node_env->ThrowError("Node API module targets unsupported version."); + return nullptr; + } + result = new node_napi_env__(context, module_filename, module_api_version); // TODO(addaleax): There was previously code that tried to delete the // napi_env when its v8::Context was garbage collected; @@ -618,13 +629,42 @@ static void napi_module_register_cb(v8::Local exports, v8::Local module, v8::Local context, void* priv) { - const napi_module* napi_mod = static_cast(priv); - int32_t module_api_version = static_cast(napi_mod->nm_api_version); - if (module_api_version == 0) { - module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION; - } napi_module_register_by_symbol( - exports, module, context, napi_mod->nm_register_func, module_api_version); + exports, + module, + context, + static_cast(priv)->nm_register_func); +} + +template +static void node_api_context_register_func(v8::Local exports, + v8::Local module, + v8::Local context, + void* priv) { + napi_module_register_by_symbol( + exports, + module, + context, + reinterpret_cast(priv), + module_api_version); +} + +// This function must be augmented for each new Node API version. +// The key role of this function is to encode module_api_version in the function +// pointer. We are not going to have many Node API versions and having one +// function per version is relatively cheap. It avoids dynamic memory +// allocations or implementing more expensive changes to module registration. +// Currently AddLinkedBinding is the only user of this function. +node::addon_context_register_func get_node_api_context_register_func( + node::Environment* node_env, int32_t module_api_version) { + if (module_api_version <= NODE_API_DEFAULT_MODULE_API_VERSION) { + return node_api_context_register_func; + } else if (module_api_version == NAPI_VERSION_EXPERIMENTAL) { + return node_api_context_register_func; + } else { + node_env->ThrowError("Node API module targets unsupported version."); + return nullptr; + } } void napi_module_register_by_symbol(v8::Local exports, diff --git a/src/node_api.h b/src/node_api.h index b4ff67bbe2a7d1..4c0356eaccb2fc 100644 --- a/src/node_api.h +++ b/src/node_api.h @@ -40,8 +40,7 @@ typedef struct napi_module { napi_addon_register_func nm_register_func; const char* nm_modname; void* nm_priv; - intptr_t nm_api_version; - void* reserved[3]; + void* reserved[4]; } napi_module; #define NAPI_MODULE_VERSION 1 diff --git a/src/node_binding.h b/src/node_binding.h index 805064b36ebd4b..da705ff5762239 100644 --- a/src/node_binding.h +++ b/src/node_binding.h @@ -49,11 +49,15 @@ static_assert(static_cast(NM_F_LINKED) == nullptr}; \ void _register_##modname() { node_module_register(&_module); } -void napi_module_register_by_symbol(v8::Local exports, - v8::Local module, - v8::Local context, - napi_addon_register_func init, - int32_t module_api_version); +void napi_module_register_by_symbol( + v8::Local exports, + v8::Local module, + v8::Local context, + napi_addon_register_func init, + int32_t module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION); + +node::addon_context_register_func get_node_api_context_register_func( + node::Environment* node_env, int32_t module_api_version); namespace node { diff --git a/src/node_version.h b/src/node_version.h index dcfd4144c0f067..abd90885d054e2 100644 --- a/src/node_version.h +++ b/src/node_version.h @@ -95,7 +95,7 @@ // which the Node binary being built supports. #define NAPI_VERSION 8 -// The NAPI_VERSION used by default by Node API modules if it is not explicitly +// Node API modules use NAPI_VERSION 8 by default if it is not explicitly // specified. It must be always 8. #define NODE_API_DEFAULT_MODULE_API_VERSION 8 diff --git a/test/cctest/test_linked_binding.cc b/test/cctest/test_linked_binding.cc index 6de16d5420acd2..6f2efda77268c5 100644 --- a/test/cctest/test_linked_binding.cc +++ b/test/cctest/test_linked_binding.cc @@ -162,10 +162,15 @@ napi_value InitializeLocalNapiRefBinding(napi_env env, napi_value exports) { napi_create_string_utf8(env, "napi_ref_created", NAPI_AUTO_LENGTH, &key), napi_ok); - // Starting with Node-API version 9 we can create napi_ref to any value type. + // In experimental Node-API version we can create napi_ref to any value type. + // Here we are trying to create a reference to the key string. napi_ref ref{}; - CHECK_EQ(napi_create_reference(env, key, 1, &ref), - node_api_version <= 8 ? napi_invalid_arg : napi_ok); + if (node_api_version == NAPI_VERSION_EXPERIMENTAL) { + CHECK_EQ(napi_create_reference(env, key, 1, &ref), napi_ok); + CHECK_EQ(napi_delete_reference(env, ref), napi_ok); + } else { + CHECK_EQ(napi_create_reference(env, key, 1, &ref), napi_invalid_arg); + } CHECK_EQ(napi_get_boolean(env, ref != nullptr, &value), napi_ok); CHECK_EQ(napi_set_property(env, exports, key, value), napi_ok); return nullptr; @@ -173,7 +178,7 @@ napi_value InitializeLocalNapiRefBinding(napi_env env, napi_value exports) { // napi_ref in Node-API version 8 cannot accept strings. TEST_F(LinkedBindingTest, LocallyDefinedLinkedBindingNapiRefVersion8Test) { - node_api_version = 8; + node_api_version = NODE_API_DEFAULT_MODULE_API_VERSION; const v8::HandleScope handle_scope(isolate_); const Argv argv; Env test_env{handle_scope, argv}; @@ -199,22 +204,22 @@ TEST_F(LinkedBindingTest, LocallyDefinedLinkedBindingNapiRefVersion8Test) { CHECK(!completion_value.As()->Value()); } -// napi_ref in Node-API version after 8 can accept strings. -TEST_F(LinkedBindingTest, LocallyDefinedLinkedBindingNapiRefVersion9Test) { - node_api_version = 9; +// Experimental version of napi_ref in Node-API can accept strings. +TEST_F(LinkedBindingTest, LocallyDefinedLinkedBindingNapiRefExperimentalTest) { + node_api_version = NAPI_VERSION_EXPERIMENTAL; const v8::HandleScope handle_scope(isolate_); const Argv argv; Env test_env{handle_scope, argv}; AddLinkedBinding(*test_env, - "local_linked_napi_ref_v9", + "local_linked_napi_ref_experimental", InitializeLocalNapiRefBinding, node_api_version); v8::Local context = isolate_->GetCurrentContext(); - const char* run_script = - "process._linkedBinding('local_linked_napi_ref_v9').napi_ref_created"; + const char* run_script = "process._linkedBinding('local_linked_napi_ref_" + "experimental').napi_ref_created"; v8::Local script = v8::Script::Compile( context, From ac53d6b48fc6845b508cddd3c55866caf70ed3af Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Mon, 24 Apr 2023 08:44:25 -0700 Subject: [PATCH 10/15] improve error messages for unsupported versions --- src/api/environment.cc | 2 +- src/node_api.cc | 25 ++++++++++++++++--- src/node_binding.h | 4 ++- .../binding.gyp | 2 +- 4 files changed, 27 insertions(+), 6 deletions(-) diff --git a/src/api/environment.cc b/src/api/environment.cc index 3a369fbcacb5f4..2128ca6c4ebb75 100644 --- a/src/api/environment.cc +++ b/src/api/environment.cc @@ -880,7 +880,7 @@ void AddLinkedBinding(Environment* env, nullptr, // nm_dso_handle nullptr, // nm_filename nullptr, // nm_register_func - get_node_api_context_register_func(env, module_api_version), + get_node_api_context_register_func(env, name, module_api_version), name, // nm_modname reinterpret_cast(fn), // nm_priv nullptr // nm_link diff --git a/src/node_api.cc b/src/node_api.cc index f314620f28c67f..efe1d09ed8ed0c 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -152,6 +152,18 @@ class BufferFinalizer : private Finalizer { ~BufferFinalizer() { env_->Unref(); } }; +void ThrowNodeApiVersionError(node::Environment* node_env, + const char* module_name, + int32_t module_api_version) { + std::string error_message; + error_message += module_name; + error_message += " requires Node-API version "; + error_message += std::to_string(module_api_version); + error_message += ", but this version of Node.js only supports version "; + error_message += NODE_STRINGIFY(NAPI_VERSION) " add-ons."; + node_env->ThrowError(error_message.c_str()); +} + inline napi_env NewEnv(v8::Local context, const std::string& module_filename, int32_t module_api_version) { @@ -164,7 +176,8 @@ inline napi_env NewEnv(v8::Local context, module_api_version != NAPI_VERSION_EXPERIMENTAL) { node::Environment* node_env = node::Environment::GetCurrent(context); CHECK_NOT_NULL(node_env); - node_env->ThrowError("Node API module targets unsupported version."); + ThrowNodeApiVersionError( + node_env, module_filename.c_str(), module_api_version); return nullptr; } @@ -656,13 +669,19 @@ static void node_api_context_register_func(v8::Local exports, // allocations or implementing more expensive changes to module registration. // Currently AddLinkedBinding is the only user of this function. node::addon_context_register_func get_node_api_context_register_func( - node::Environment* node_env, int32_t module_api_version) { + node::Environment* node_env, + const char* module_name, + int32_t module_api_version) { + static_assert( + NAPI_VERSION == 8, + "New version of Node-API requires adding another else-if statement below " + "for the new version and updating this assert condition."); if (module_api_version <= NODE_API_DEFAULT_MODULE_API_VERSION) { return node_api_context_register_func; } else if (module_api_version == NAPI_VERSION_EXPERIMENTAL) { return node_api_context_register_func; } else { - node_env->ThrowError("Node API module targets unsupported version."); + v8impl::ThrowNodeApiVersionError(node_env, module_name, module_api_version); return nullptr; } } diff --git a/src/node_binding.h b/src/node_binding.h index da705ff5762239..6e1f2678579bf4 100644 --- a/src/node_binding.h +++ b/src/node_binding.h @@ -57,7 +57,9 @@ void napi_module_register_by_symbol( int32_t module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION); node::addon_context_register_func get_node_api_context_register_func( - node::Environment* node_env, int32_t module_api_version); + node::Environment* node_env, + const char* module_name, + int32_t module_api_version); namespace node { diff --git a/test/node-api/test_reference_by_node_api_version/binding.gyp b/test/node-api/test_reference_by_node_api_version/binding.gyp index 11d8842abbec6d..2ee1d24763b0b3 100644 --- a/test/node-api/test_reference_by_node_api_version/binding.gyp +++ b/test/node-api/test_reference_by_node_api_version/binding.gyp @@ -11,4 +11,4 @@ "defines": [ "NAPI_VERSION=8" ], } ] -} \ No newline at end of file +} From f10f20f8962c310afeab3b53331d4ea3f5bf1f98 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 28 Apr 2023 08:19:45 -0700 Subject: [PATCH 11/15] allow local symbols be weak references --- src/js_native_api_v8.cc | 45 ++++++++++++++++++- src/js_native_api_v8.h | 1 + test/js-native-api/test_reference/test.js | 15 +++++-- .../test.js | 21 ++++++--- .../test_reference_by_node_api_version.c | 6 +++ 5 files changed, 78 insertions(+), 10 deletions(-) diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index abdde7dd497e47..ae517c9d072473 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -457,6 +457,49 @@ inline napi_status Wrap(napi_env env, return GET_RETURN_STATUS(env); } +// In JavaScript weak references can be created for object types (Object, +// Function, external Object) and for local symbols that are created with +// the `Symbol` function call. Global symbols created with `Symbol.for` method +// cannot be weak references because they are never collected. +// We use the `Symbol.keyFor` function call to differentiate between them. This +// function must return `undefined` for local symbols. +// +// It would be more efficient to check for the internal symbol flag +// `is_in_public_symbol_table` but it is not accessible in the public V8 API. +// V8 has an internal function `CanBeHeldWeakly`. It could be used if it is +// available in the public V8 API. +inline bool CanBeHeldWeakly(napi_env env, v8::Local value) { + if (value->IsObject()) { + return true; + } + if (value->IsSymbol()) { + v8::Isolate* isolate = env->isolate; + v8::Local context = env->context(); + v8::Local symbol_key_for; + if (env->symbol_key_for_.IsEmpty()) { + v8::Local global = context->Global(); + v8::Local symbol_name = v8::String::NewFromUtf8Literal( + isolate, "Symbol", v8::NewStringType::kInternalized); + v8::Local key_for_name = v8::String::NewFromUtf8Literal( + isolate, "keyFor", v8::NewStringType::kInternalized); + v8::Local symbol_class = + global->Get(context, symbol_name).ToLocalChecked().As(); + symbol_key_for = symbol_class->Get(context, key_for_name) + .ToLocalChecked() + .As(); + env->symbol_key_for_ = + v8impl::Persistent(isolate, symbol_key_for); + } else { + symbol_key_for = + v8::Local::New(isolate, env->symbol_key_for_); + } + v8::MaybeLocal result = + symbol_key_for->Call(context, v8::Undefined(isolate), 1, &value); + return result.IsEmpty() || result.ToLocalChecked()->IsUndefined(); + } + return false; +} + } // end of anonymous namespace void Finalizer::ResetFinalizer() { @@ -552,7 +595,7 @@ template Reference::Reference(napi_env env, v8::Local value, Args&&... args) : RefBase(env, std::forward(args)...), persistent_(env->isolate, value), - can_be_weak_(value->IsObject()) { + can_be_weak_(CanBeHeldWeakly(env, value)) { if (RefCount() == 0) { SetWeak(); } diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 2f4705dce67713..06891bccab1ebc 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -148,6 +148,7 @@ struct napi_env__ { int refs = 1; void* instance_data = nullptr; int32_t module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION; + v8impl::Persistent symbol_key_for_{}; protected: // Should not be deleted directly. Delete with `napi_env__::DeleteMe()` diff --git a/test/js-native-api/test_reference/test.js b/test/js-native-api/test_reference/test.js index 5c36dd8bdd210c..48897c98645514 100644 --- a/test/js-native-api/test_reference/test.js +++ b/test/js-native-api/test_reference/test.js @@ -17,14 +17,14 @@ async function runTests() { (() => { const symbol = test_reference.createSymbol('testSym'); test_reference.createReference(symbol, 0); - assert.strictEqual(test_reference.referenceValue, undefined); + assert.strictEqual(test_reference.referenceValue, symbol); })(); test_reference.deleteReference(); (() => { - const symbol = test_reference.createSymbol('testSym'); - test_reference.createReference(symbol, 1); - assert.strictEqual(test_reference.referenceValue, symbol); + const symbol = test_reference.createSymbolFor('testSymFor'); + test_reference.createReference(symbol, 0); + assert.strictEqual(test_reference.referenceValue, undefined); })(); test_reference.deleteReference(); @@ -36,6 +36,13 @@ async function runTests() { })(); test_reference.deleteReference(); + (() => { + const symbol = test_reference.createSymbolForEmptyString(); + test_reference.createReference(symbol, 0); + assert.strictEqual(test_reference.referenceValue, undefined); + })(); + test_reference.deleteReference(); + (() => { const symbol = test_reference.createSymbolForEmptyString(); test_reference.createReference(symbol, 1); diff --git a/test/node-api/test_reference_by_node_api_version/test.js b/test/node-api/test_reference_by_node_api_version/test.js index 5ddae18897b375..9711d85a07baea 100644 --- a/test/node-api/test_reference_by_node_api_version/test.js +++ b/test/node-api/test_reference_by_node_api_version/test.js @@ -12,7 +12,7 @@ const assert = require('assert'); const addon_v8 = require(`./build/${buildType}/test_reference_obj_only`); const addon_new = require(`./build/${buildType}/test_reference_all_types`); -async function runTests(addon, isVersion8) { +async function runTests(addon, isVersion8, isLocalSymbol) { let allEntries = []; (() => { @@ -22,19 +22,23 @@ async function runTests(addon, isVersion8) { const booleanValue = false; const numberValue = 42; const stringValue = 'test_string'; - const symbolValue = Symbol.for('test_symbol'); + const globalSymbolValue = Symbol.for('test_symbol_global'); + const localSymbolValue = Symbol('test_symbol_local'); + const symbolValue = isLocalSymbol ? localSymbolValue : globalSymbolValue; const objectValue = { x: 1, y: 2 }; const functionValue = (x, y) => x + y; const externalValue = addon.createExternal(); const bigintValue = 9007199254740991n; + // The position of entries in the allEntries array correspond to the napi_valuetype enum values. + // See the CreateRef implementation in C code. allEntries = [ { value: undefinedValue, canBeWeak: false, canBeRefV8: false }, { value: nullValue, canBeWeak: false, canBeRefV8: false }, { value: booleanValue, canBeWeak: false, canBeRefV8: false }, { value: numberValue, canBeWeak: false, canBeRefV8: false }, { value: stringValue, canBeWeak: false, canBeRefV8: false }, - { value: symbolValue, canBeWeak: false, canBeRefV8: true }, + { value: symbolValue, canBeWeak: isLocalSymbol, canBeRefV8: true }, { value: objectValue, canBeWeak: true, canBeRefV8: true }, { value: functionValue, canBeWeak: true, canBeRefV8: true }, { value: externalValue, canBeWeak: true, canBeRefV8: true }, @@ -81,6 +85,7 @@ async function runTests(addon, isVersion8) { addon.addFinalizer(objWithFinalizer); })(); + addon.initFinalizeCount(); assert.strictEqual(addon.getFinalizeCount(), 0); await gcUntil('Wait until a finalizer is called', () => (addon.getFinalizeCount() === 1)); @@ -103,5 +108,11 @@ async function runTests(addon, isVersion8) { }); } -runTests(addon_v8, /* isVersion8 */ true); -runTests(addon_new, /* isVersion8 */ false); +async function runAllTests() { + await runTests(addon_v8, /* isVersion8 */ true, /* isLocalSymbol */ true); + await runTests(addon_v8, /* isVersion8 */ true, /* isLocalSymbol */ false); + await runTests(addon_new, /* isVersion8 */ false, /* isLocalSymbol */ true); + await runTests(addon_new, /* isVersion8 */ false, /* isLocalSymbol */ false); +} + +runAllTests(); diff --git a/test/node-api/test_reference_by_node_api_version/test_reference_by_node_api_version.c b/test/node-api/test_reference_by_node_api_version/test_reference_by_node_api_version.c index 5dcc46d96926c6..23e5b1988b441c 100644 --- a/test/node-api/test_reference_by_node_api_version/test_reference_by_node_api_version.c +++ b/test/node-api/test_reference_by_node_api_version/test_reference_by_node_api_version.c @@ -146,6 +146,11 @@ static napi_value GetFinalizeCount(napi_env env, napi_callback_info info) { return ToUInt32Value(env, finalizeCount); } +static napi_value InitFinalizeCount(napi_env env, napi_callback_info info) { + finalizeCount = 0; + return NULL; +} + EXTERN_C_START NAPI_MODULE_INIT() { @@ -161,6 +166,7 @@ NAPI_MODULE_INIT() { DECLARE_NODE_API_PROPERTY("deleteRef", DeleteRef), DECLARE_NODE_API_PROPERTY("addFinalizer", AddFinalizer), DECLARE_NODE_API_PROPERTY("getFinalizeCount", GetFinalizeCount), + DECLARE_NODE_API_PROPERTY("initFinalizeCount", InitFinalizeCount), }; NODE_API_CALL( From bb3cc12bb86d79ac0df9e04c5e812c1f8549f17f Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Fri, 28 Apr 2023 09:18:13 -0700 Subject: [PATCH 12/15] add a comment for symbol_key_for_ field --- src/js_native_api_v8.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 06891bccab1ebc..a4f4330f701171 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -148,6 +148,7 @@ struct napi_env__ { int refs = 1; void* instance_data = nullptr; int32_t module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION; + // Cached JavaScript Symbol.keyFor function. v8impl::Persistent symbol_key_for_{}; protected: From 4b709c10c6ad0e745693b8f1ba7aa4ad69189d75 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Mon, 1 May 2023 08:27:54 -0700 Subject: [PATCH 13/15] do not call JS while creating napi_ref --- doc/api/n-api.md | 15 ++++-- src/js_native_api_v8.cc | 51 ++++--------------- src/js_native_api_v8.h | 2 - test/js-native-api/test_reference/test.js | 4 +- .../test.js | 16 ++++-- 5 files changed, 35 insertions(+), 53 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index e997a3b2be3982..acc018c09fd63f 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -1655,12 +1655,21 @@ Node-API provides methods to create persistent references to values. Each reference has an associated count with a value of 0 or higher. The count determines if the reference will keep the corresponding value alive. References with a count of 0 do not prevent values from being collected. -Values of object types (object, function, external) are becoming 'weak' -references and can still be accessed while they are not collected. -Values of non-object types are released when the count becomes 0 +Values of object (object, function, external) and symbol types are becoming +'weak' references and can still be accessed while they are not collected. +Values of other types are released when the count becomes 0 and cannot be accessed from the reference any more. Any count greater than 0 will prevent the values from being collected. +It is important to note that Symbol values have different flavors. +The true weak reference behavior is only supported by local symbols created +with the `Symbol()` constructor call. The globally registered symbols created +with the `Symbol.for()` call remain always strong because the garbage collector +does not collect them. The same is true for well-known symbols such as +`Symbol.iterator`. They are also never collected by the garbage collector. +JavaScript's `WeakRef` and `WeakMap` types return an error when the +registered symbols are used. They succeed for local and well-known symbols. + References can be created with an initial reference count. The count can then be modified through [`napi_reference_ref`][] and [`napi_reference_unref`][]. If an object is collected while the count diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index ae517c9d072473..9cbcd2445d1980 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -457,47 +457,16 @@ inline napi_status Wrap(napi_env env, return GET_RETURN_STATUS(env); } -// In JavaScript weak references can be created for object types (Object, -// Function, external Object) and for local symbols that are created with -// the `Symbol` function call. Global symbols created with `Symbol.for` method -// cannot be weak references because they are never collected. -// We use the `Symbol.keyFor` function call to differentiate between them. This -// function must return `undefined` for local symbols. +// In JavaScript, weak references can be created for object types (Object, +// Function, and external Object) and for local symbols that are created with +// the `Symbol` function call. Global symbols created with the `Symbol.for` +// method cannot be weak references because they are never collected. // -// It would be more efficient to check for the internal symbol flag -// `is_in_public_symbol_table` but it is not accessible in the public V8 API. -// V8 has an internal function `CanBeHeldWeakly`. It could be used if it is -// available in the public V8 API. -inline bool CanBeHeldWeakly(napi_env env, v8::Local value) { - if (value->IsObject()) { - return true; - } - if (value->IsSymbol()) { - v8::Isolate* isolate = env->isolate; - v8::Local context = env->context(); - v8::Local symbol_key_for; - if (env->symbol_key_for_.IsEmpty()) { - v8::Local global = context->Global(); - v8::Local symbol_name = v8::String::NewFromUtf8Literal( - isolate, "Symbol", v8::NewStringType::kInternalized); - v8::Local key_for_name = v8::String::NewFromUtf8Literal( - isolate, "keyFor", v8::NewStringType::kInternalized); - v8::Local symbol_class = - global->Get(context, symbol_name).ToLocalChecked().As(); - symbol_key_for = symbol_class->Get(context, key_for_name) - .ToLocalChecked() - .As(); - env->symbol_key_for_ = - v8impl::Persistent(isolate, symbol_key_for); - } else { - symbol_key_for = - v8::Local::New(isolate, env->symbol_key_for_); - } - v8::MaybeLocal result = - symbol_key_for->Call(context, v8::Undefined(isolate), 1, &value); - return result.IsEmpty() || result.ToLocalChecked()->IsUndefined(); - } - return false; +// Currently, V8 has no API to detect if a symbol is local or global. +// Until we have a V8 API for it, we consider that all symbols can be weak. +// This matches the current Node-API behavior. +inline bool CanBeHeldWeakly(v8::Local value) { + return value->IsObject() || value->IsSymbol(); } } // end of anonymous namespace @@ -595,7 +564,7 @@ template Reference::Reference(napi_env env, v8::Local value, Args&&... args) : RefBase(env, std::forward(args)...), persistent_(env->isolate, value), - can_be_weak_(CanBeHeldWeakly(env, value)) { + can_be_weak_(CanBeHeldWeakly(value)) { if (RefCount() == 0) { SetWeak(); } diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index a4f4330f701171..2f4705dce67713 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -148,8 +148,6 @@ struct napi_env__ { int refs = 1; void* instance_data = nullptr; int32_t module_api_version = NODE_API_DEFAULT_MODULE_API_VERSION; - // Cached JavaScript Symbol.keyFor function. - v8impl::Persistent symbol_key_for_{}; protected: // Should not be deleted directly. Delete with `napi_env__::DeleteMe()` diff --git a/test/js-native-api/test_reference/test.js b/test/js-native-api/test_reference/test.js index 48897c98645514..e65847fbdb0596 100644 --- a/test/js-native-api/test_reference/test.js +++ b/test/js-native-api/test_reference/test.js @@ -24,7 +24,7 @@ async function runTests() { (() => { const symbol = test_reference.createSymbolFor('testSymFor'); test_reference.createReference(symbol, 0); - assert.strictEqual(test_reference.referenceValue, undefined); + assert.strictEqual(test_reference.referenceValue, symbol); })(); test_reference.deleteReference(); @@ -39,7 +39,7 @@ async function runTests() { (() => { const symbol = test_reference.createSymbolForEmptyString(); test_reference.createReference(symbol, 0); - assert.strictEqual(test_reference.referenceValue, undefined); + assert.strictEqual(test_reference.referenceValue, Symbol.for('')); })(); test_reference.deleteReference(); diff --git a/test/node-api/test_reference_by_node_api_version/test.js b/test/node-api/test_reference_by_node_api_version/test.js index 9711d85a07baea..32530f681508c8 100644 --- a/test/node-api/test_reference_by_node_api_version/test.js +++ b/test/node-api/test_reference_by_node_api_version/test.js @@ -30,15 +30,17 @@ async function runTests(addon, isVersion8, isLocalSymbol) { const externalValue = addon.createExternal(); const bigintValue = 9007199254740991n; - // The position of entries in the allEntries array correspond to the napi_valuetype enum values. - // See the CreateRef implementation in C code. + // The position of entries in the allEntries array corresponds to the + // napi_valuetype enum value. See the CreateRef function for the + // implementation details. allEntries = [ { value: undefinedValue, canBeWeak: false, canBeRefV8: false }, { value: nullValue, canBeWeak: false, canBeRefV8: false }, { value: booleanValue, canBeWeak: false, canBeRefV8: false }, { value: numberValue, canBeWeak: false, canBeRefV8: false }, { value: stringValue, canBeWeak: false, canBeRefV8: false }, - { value: symbolValue, canBeWeak: isLocalSymbol, canBeRefV8: true }, + { value: symbolValue, canBeWeak: isLocalSymbol, canBeRefV8: true, + isAlwaysStrong: !isLocalSymbol }, { value: objectValue, canBeWeak: true, canBeRefV8: true }, { value: functionValue, canBeWeak: true, canBeRefV8: true }, { value: externalValue, canBeWeak: true, canBeRefV8: true }, @@ -70,7 +72,7 @@ async function runTests(addon, isVersion8, isLocalSymbol) { // still in the allEntries array. allEntries.forEach((entry, index) => { if (!isVersion8 || entry.canBeRefV8) { - if (entry.canBeWeak) { + if (entry.canBeWeak || entry.isAlwaysStrong) { assert.strictEqual(addon.getRefValue(index), entry.value); } else { assert.strictEqual(addon.getRefValue(index), undefined); @@ -102,7 +104,11 @@ async function runTests(addon, isVersion8, isLocalSymbol) { // semantic must return undefined value. allEntries.forEach((entry, index) => { if (!isVersion8 || entry.canBeRefV8) { - assert.strictEqual(addon.getRefValue(index), undefined); + if (!entry.isAlwaysStrong) { + assert.strictEqual(addon.getRefValue(index), undefined); + } else { + assert.notStrictEqual(addon.getRefValue(index), undefined); + } addon.deleteRef(index); } }); From ad50859fbd5c9104c7538535bb7680201dcdd4fe Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Mon, 1 May 2023 09:05:11 -0700 Subject: [PATCH 14/15] fix md linter issues --- doc/api/n-api.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index acc018c09fd63f..1823402aad0968 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -1661,14 +1661,14 @@ Values of other types are released when the count becomes 0 and cannot be accessed from the reference any more. Any count greater than 0 will prevent the values from being collected. -It is important to note that Symbol values have different flavors. -The true weak reference behavior is only supported by local symbols created -with the `Symbol()` constructor call. The globally registered symbols created -with the `Symbol.for()` call remain always strong because the garbage collector -does not collect them. The same is true for well-known symbols such as -`Symbol.iterator`. They are also never collected by the garbage collector. -JavaScript's `WeakRef` and `WeakMap` types return an error when the -registered symbols are used. They succeed for local and well-known symbols. +The Symbol values have different flavors. The true weak reference behavior is +only supported by local symbols created with the `Symbol()` constructor call. +The globally registered symbols created with the `Symbol.for()` call remain +always strong references because the garbage collector does not collect them. +The same is true for well-known symbols such as `Symbol.iterator`. They are +also never collected by the garbage collector. JavaScript's `WeakRef` and +`WeakMap` types return an error when the registered symbols are used. +They succeed for local and well-known symbols. References can be created with an initial reference count. The count can then be modified through [`napi_reference_ref`][] and From 38d5f0f5507bedb8c0a40b8c71f2f0b714fb3977 Mon Sep 17 00:00:00 2001 From: Vladimir Morozov Date: Tue, 2 May 2023 22:30:15 -0700 Subject: [PATCH 15/15] avoid using pronouns in the docs --- doc/api/n-api.md | 64 +++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 31 deletions(-) diff --git a/doc/api/n-api.md b/doc/api/n-api.md index 1823402aad0968..77cc49946c4231 100644 --- a/doc/api/n-api.md +++ b/doc/api/n-api.md @@ -1651,9 +1651,9 @@ described in the earlier section. The lifespan of a normal handle is managed by scopes and all scopes must be closed before the end of a native method. -Node-API provides methods to create persistent references to values. -Each reference has an associated count with a value of 0 or higher. -The count determines if the reference will keep the corresponding value alive. +Node-API provides methods for creating persistent references to values. +Each reference has an associated count with a value of 0 or higher, +which determines whether the reference will keep the corresponding value alive. References with a count of 0 do not prevent values from being collected. Values of object (object, function, external) and symbol types are becoming 'weak' references and can still be accessed while they are not collected. @@ -1661,14 +1661,14 @@ Values of other types are released when the count becomes 0 and cannot be accessed from the reference any more. Any count greater than 0 will prevent the values from being collected. -The Symbol values have different flavors. The true weak reference behavior is +Symbol values have different flavors. The true weak reference behavior is only supported by local symbols created with the `Symbol()` constructor call. -The globally registered symbols created with the `Symbol.for()` call remain +Globally registered symbols created with the `Symbol.for()` call remain always strong references because the garbage collector does not collect them. The same is true for well-known symbols such as `Symbol.iterator`. They are also never collected by the garbage collector. JavaScript's `WeakRef` and -`WeakMap` types return an error when the registered symbols are used. -They succeed for local and well-known symbols. +`WeakMap` types return an error when registered symbols are used, +but they succeed for local and well-known symbols. References can be created with an initial reference count. The count can then be modified through [`napi_reference_ref`][] and @@ -1679,24 +1679,26 @@ will return `NULL` for the returned `napi_value`. An attempt to call [`napi_reference_ref`][] for a reference whose object has been collected results in an error. -In Node-API version 8 and before we can only create references for a limited -set of value types: object, external, function, and symbol. In newer Node-API -versions the references can be created for any value type. +Node-API versions 8 and earlier only allow references to be created for a +limited set of value types, including object, external, function, and symbol. +However, in newer Node-API versions, references can be created for any +value type. References must be deleted once they are no longer required by the addon. When -a reference is deleted, it will no longer prevent the corresponding value from -being collected. If you don't delete a persistent reference, it can cause a -'memory leak'. This means that both the native memory for the persistent -reference and the corresponding value on the heap will be retained forever. - -You can create multiple persistent references that refer to the same value. -Each reference will keep the value alive or not based on its individual count. -If there are multiple persistent references to the same value, it can -unexpectedly keep alive native memory. The native structures for a persistent -reference must be kept alive until finalizers for the referenced value are -executed. If you create a new persistent reference for the same value, the -finalizers for that value will not be run and the native memory pointed by -the earlier persistent reference will not be freed. To avoid this, call +a reference is deleted, it will no longer prevent the corresponding object from +being collected. Failure to delete a persistent reference results in +a 'memory leak' with both the native memory for the persistent reference and +the corresponding object on the heap being retained forever. + +There can be multiple persistent references created which refer to the same +object, each of which will either keep the object live or not based on its +individual count. Multiple persistent references to the same object +can result in unexpectedly keeping alive native memory. The native structures +for a persistent reference must be kept alive until finalizers for the +referenced object are executed. If a new persistent reference is created +for the same object, the finalizers for that object will not be +run and the native memory pointed by the earlier persistent reference +will not be freed. This can be avoided by calling `napi_delete_reference` in addition to `napi_reference_unref` when possible. #### `napi_create_reference` @@ -1714,7 +1716,7 @@ NAPI_EXTERN napi_status napi_create_reference(napi_env env, ``` * `[in] env`: The environment that the API is invoked under. -* `[in] value`: `napi_value` to which we want to create a reference. +* `[in] value`: The `napi_value` for which a reference is being created. * `[in] initial_refcount`: Initial reference count for the new reference. * `[out] result`: `napi_ref` pointing to the new reference. @@ -1723,9 +1725,9 @@ Returns `napi_ok` if the API succeeded. This API creates a new reference with the specified reference count to the value passed in. -In Node-API version 8 and earlier, you could only create a reference for -object, function, external, and symbol value types. In newer Node-API -versions, you can create a reference for any value type. +In Node-API version 8 and earlier, a reference could only be created for +object, function, external, and symbol value types. However, in newer Node-API +versions, a reference can be created for any value type. #### `napi_delete_reference` @@ -1805,7 +1807,8 @@ NAPI_EXTERN napi_status napi_get_reference_value(napi_env env, ``` * `[in] env`: The environment that the API is invoked under. -* `[in] ref`: `napi_ref` for which we are requesting the corresponding value. +* `[in] ref`: The `napi_ref` for which the corresponding value is + being requested. * `[out] result`: The `napi_value` referenced by the `napi_ref`. Returns `napi_ok` if the API succeeded. @@ -5082,9 +5085,8 @@ napi_status napi_define_class(napi_env env, ``` * `[in] env`: The environment that the API is invoked under. -* `[in] utf8name`: Name of the JavaScript constructor function; When wrapping a - C++ class, we recommend for clarity that this name be the same as that of - the C++ class. +* `[in] utf8name`: Name of the JavaScript constructor function. For clarity, + it is recommended to use the C++ class name when wrapping a C++ class. * `[in] length`: The length of the `utf8name` in bytes, or `NAPI_AUTO_LENGTH` if it is null-terminated. * `[in] constructor`: Callback function that handles constructing instances