From 53ca0b9ae145c430842bf78e553e3b6cbd2823aa Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Thu, 5 Sep 2019 18:38:19 -0700 Subject: [PATCH] src: render N-API weak callbacks as cleanup hooks Since worker threads are complete Node.js environments, including the ability to load native addons, and since those native addons can allocate resources to be freed when objects go out of scope, and since, upon worker thread exit, the engine does not invoke the weak callbacks responsible for freeing resources which still have references, this modification introduces tracking for weak references such that a list of outstanding weak references is maintained. This list is traversed during environment teardown. The callbacks for the remaining weak references are called. This change is also relevant for Node.js embedder scenarios, because in those cases the process also outlives the `node::Environment` and therefore weak callbacks should also be rendered as environment cleanup hooks to ensure proper cleanup after native addons. This changes introduces the means by which this can be accomplished. A benchmark is included which measures the time it takes to execute the weak reference callback for a given number of weak references. Re: https://github.com/tc39/proposal-weakrefs/issues/125#issuecomment-535832130 PR-URL: https://github.com/nodejs/node/pull/28428 Reviewed-By: Anna Henningsen Reviewed-By: Michael Dawson --- benchmark/napi/ref/addon.c | 82 +++++++++++++++++++ benchmark/napi/ref/binding.gyp | 10 +++ benchmark/napi/ref/index.js | 17 ++++ src/js_native_api_v8.cc | 46 ++++++----- src/js_native_api_v8.h | 13 +++ src/js_native_api_v8_internals.h | 39 +++++++++ .../test_general/testEnvCleanup.js | 57 +++++++++++++ .../js-native-api/test_general/test_general.c | 57 ++++++++++--- 8 files changed, 288 insertions(+), 33 deletions(-) create mode 100644 benchmark/napi/ref/addon.c create mode 100644 benchmark/napi/ref/binding.gyp create mode 100644 benchmark/napi/ref/index.js create mode 100644 test/js-native-api/test_general/testEnvCleanup.js diff --git a/benchmark/napi/ref/addon.c b/benchmark/napi/ref/addon.c new file mode 100644 index 00000000000000..3fb8de603d3ced --- /dev/null +++ b/benchmark/napi/ref/addon.c @@ -0,0 +1,82 @@ +#include +#define NAPI_EXPERIMENTAL +#include + +#define NAPI_CALL(env, call) \ + do { \ + napi_status status = (call); \ + if (status != napi_ok) { \ + napi_throw_error((env), NULL, #call " failed"); \ + return NULL; \ + } \ + } while (0) + +static napi_value +GetCount(napi_env env, napi_callback_info info) { + napi_value result; + size_t* count; + + NAPI_CALL(env, napi_get_instance_data(env, (void**)&count)); + NAPI_CALL(env, napi_create_uint32(env, *count, &result)); + + return result; +} + +static napi_value +SetCount(napi_env env, napi_callback_info info) { + size_t* count; + + NAPI_CALL(env, napi_get_instance_data(env, (void**)&count)); + + // Set the count to zero irrespective of what is passed into the setter. + *count = 0; + + return NULL; +} + +static void +IncrementCounter(napi_env env, void* data, void* hint) { + size_t* count = data; + (*count) = (*count) + 1; +} + +static napi_value +NewWeak(napi_env env, napi_callback_info info) { + napi_value result; + void* instance_data; + + NAPI_CALL(env, napi_create_object(env, &result)); + NAPI_CALL(env, napi_get_instance_data(env, &instance_data)); + NAPI_CALL(env, napi_add_finalizer(env, + result, + instance_data, + IncrementCounter, + NULL, + NULL)); + + return result; +} + +static void +FreeCount(napi_env env, void* data, void* hint) { + free(data); +} + +/* napi_value */ +NAPI_MODULE_INIT(/* napi_env env, napi_value exports */) { + napi_property_descriptor props[] = { + { "count", NULL, NULL, GetCount, SetCount, NULL, napi_enumerable, NULL }, + { "newWeak", NULL, NewWeak, NULL, NULL, NULL, napi_enumerable, NULL } + }; + + size_t* count = malloc(sizeof(*count)); + *count = 0; + + NAPI_CALL(env, napi_define_properties(env, + exports, + sizeof(props) / sizeof(*props), + props)); + NAPI_CALL(env, napi_set_instance_data(env, count, FreeCount, NULL)); + + return exports; +} diff --git a/benchmark/napi/ref/binding.gyp b/benchmark/napi/ref/binding.gyp new file mode 100644 index 00000000000000..d641e99efd77cf --- /dev/null +++ b/benchmark/napi/ref/binding.gyp @@ -0,0 +1,10 @@ +{ + 'targets': [ + { + 'target_name': 'addon', + 'sources': [ + 'addon.c' + ] + } + ] +} diff --git a/benchmark/napi/ref/index.js b/benchmark/napi/ref/index.js new file mode 100644 index 00000000000000..3a5e1988275eaa --- /dev/null +++ b/benchmark/napi/ref/index.js @@ -0,0 +1,17 @@ +'use strict'; +const common = require('../../common'); +const addon = require(`./build/${common.buildType}/addon`); +const bench = common.createBenchmark(main, { n: [1e7] }); + +function callNewWeak() { + addon.newWeak(); +} + +function main({ n }) { + addon.count = 0; + bench.start(); + while (addon.count < n) { + callNewWeak(); + } + bench.end(n); +} diff --git a/src/js_native_api_v8.cc b/src/js_native_api_v8.cc index a87af79a890832..eea8adea3769ee 100644 --- a/src/js_native_api_v8.cc +++ b/src/js_native_api_v8.cc @@ -186,7 +186,7 @@ inline static napi_status ConcludeDeferred(napi_env env, } // Wrapper around v8impl::Persistent that implements reference counting. -class Reference : private Finalizer { +class Reference : private Finalizer, RefTracker { private: Reference(napi_env env, v8::Local value, @@ -203,6 +203,9 @@ class Reference : private Finalizer { _persistent.SetWeak( this, FinalizeCallback, v8::WeakCallbackType::kParameter); } + Link(finalize_callback == nullptr + ? &env->reflist + : &env->finalizing_reflist); } public: @@ -242,6 +245,7 @@ class Reference : private Finalizer { // the finalizer and _delete_self is set. In this case we // know we need to do the deletion so just do it. static void Delete(Reference* reference) { + reference->Unlink(); if ((reference->RefCount() != 0) || (reference->_delete_self) || (reference->_finalize_ran)) { @@ -286,6 +290,26 @@ class Reference : private Finalizer { } private: + void Finalize(bool is_env_teardown = false) override { + if (_finalize_callback != nullptr) { + _env->CallIntoModuleThrow([&](napi_env env) { + _finalize_callback( + env, + _finalize_data, + _finalize_hint); + }); + } + + // this is safe because if a request to delete the reference + // is made in the finalize_callback it will defer deletion + // to this block and set _delete_self to true + if (_delete_self || is_env_teardown) { + Delete(this); + } else { + _finalize_ran = true; + } + } + // The N-API finalizer callback may make calls into the engine. V8's heap is // not in a consistent state during the weak callback, and therefore it does // not support calls back into it. However, it provides a mechanism for adding @@ -303,25 +327,7 @@ class Reference : private Finalizer { } static void SecondPassCallback(const v8::WeakCallbackInfo& data) { - Reference* reference = data.GetParameter(); - - if (reference->_finalize_callback != nullptr) { - reference->_env->CallIntoModuleThrow([&](napi_env env) { - reference->_finalize_callback( - env, - reference->_finalize_data, - reference->_finalize_hint); - }); - } - - // this is safe because if a request to delete the reference - // is made in the finalize_callback it will defer deletion - // to this block and set _delete_self to true - if (reference->_delete_self) { - Delete(reference); - } else { - reference->_finalize_ran = true; - } + data.GetParameter()->Finalize(); } v8impl::Persistent _persistent; diff --git a/src/js_native_api_v8.h b/src/js_native_api_v8.h index 506e693f821227..2e0a7a1d6add20 100644 --- a/src/js_native_api_v8.h +++ b/src/js_native_api_v8.h @@ -15,6 +15,13 @@ struct napi_env__ { CHECK_EQ(isolate, context->GetIsolate()); } virtual ~napi_env__() { + // First we must finalize those references that have `napi_finalizer` + // callbacks. The reason is that addons might store other references which + // they delete during their `napi_finalizer` callbacks. If we deleted such + // references here first, they would be doubly deleted when the + // `napi_finalizer` deleted them subsequently. + v8impl::RefTracker::FinalizeAll(&finalizing_reflist); + v8impl::RefTracker::FinalizeAll(&reflist); if (instance_data.finalize_cb != nullptr) { CallIntoModuleThrow([&](napi_env env) { instance_data.finalize_cb(env, instance_data.data, instance_data.hint); @@ -55,6 +62,12 @@ struct napi_env__ { } v8impl::Persistent last_exception; + + // We store references in two different lists, depending on whether they have + // `napi_finalizer` callbacks, because we must first finalize the ones that + // have such a callback. See `~napi_env__()` above for details. + v8impl::RefTracker::RefList reflist; + v8impl::RefTracker::RefList finalizing_reflist; napi_extended_error_info last_error; int open_handle_scopes = 0; int open_callback_scopes = 0; diff --git a/src/js_native_api_v8_internals.h b/src/js_native_api_v8_internals.h index ddd219818cdfa9..74afd1172e527e 100644 --- a/src/js_native_api_v8_internals.h +++ b/src/js_native_api_v8_internals.h @@ -28,6 +28,45 @@ namespace v8impl { +class RefTracker { + public: + RefTracker() {} + virtual ~RefTracker() {} + virtual void Finalize(bool isEnvTeardown) {} + + typedef RefTracker RefList; + + inline void Link(RefList* list) { + prev_ = list; + next_ = list->next_; + if (next_ != nullptr) { + next_->prev_ = this; + } + list->next_ = this; + } + + inline void Unlink() { + if (prev_ != nullptr) { + prev_->next_ = next_; + } + if (next_ != nullptr) { + next_->prev_ = prev_; + } + prev_ = nullptr; + next_ = nullptr; + } + + static void FinalizeAll(RefList* list) { + while (list->next_ != nullptr) { + list->next_->Finalize(true); + } + } + + private: + RefList* next_ = nullptr; + RefList* prev_ = nullptr; +}; + template using Persistent = v8::Global; diff --git a/test/js-native-api/test_general/testEnvCleanup.js b/test/js-native-api/test_general/testEnvCleanup.js new file mode 100644 index 00000000000000..8d567bef4518ce --- /dev/null +++ b/test/js-native-api/test_general/testEnvCleanup.js @@ -0,0 +1,57 @@ +'use strict'; + +if (process.argv[2] === 'child') { + const common = require('../../common'); + const test_general = require(`./build/${common.buildType}/test_general`); + + // The second argument to `envCleanupWrap()` is an index into the global + // static string array named `env_cleanup_finalizer_messages` on the native + // side. A reverse mapping is reproduced here for clarity. + const finalizerMessages = { + 'simple wrap': 0, + 'wrap, removeWrap': 1, + 'first wrap': 2, + 'second wrap': 3 + }; + + // We attach the three objects we will test to `module.exports` to ensure they + // will not be garbage-collected before the process exits. + + // Make sure the finalizer for a simple wrap will be called at env cleanup. + module.exports['simple wrap'] = + test_general.envCleanupWrap({}, finalizerMessages['simple wrap']); + + // Make sure that a removed wrap does not result in a call to its finalizer at + // env cleanup. + module.exports['wrap, removeWrap'] = + test_general.envCleanupWrap({}, finalizerMessages['wrap, removeWrap']); + test_general.removeWrap(module.exports['wrap, removeWrap']); + + // Make sure that only the latest attached version of a re-wrapped item's + // finalizer gets called at env cleanup. + module.exports['first wrap'] = + test_general.envCleanupWrap({}, finalizerMessages['first wrap']), + test_general.removeWrap(module.exports['first wrap']); + test_general.envCleanupWrap(module.exports['first wrap'], + finalizerMessages['second wrap']); +} else { + const assert = require('assert'); + const { spawnSync } = require('child_process'); + + const child = spawnSync(process.execPath, [__filename, 'child'], { + stdio: [ process.stdin, 'pipe', process.stderr ] + }); + + // Grab the child's output and construct an object whose keys are the rows of + // the output and whose values are `true`, so we can compare the output while + // ignoring the order in which the lines of it were produced. + assert.deepStrictEqual( + child.stdout.toString().split(/\r\n|\r|\n/g).reduce((obj, item) => + Object.assign(obj, item ? { [item]: true } : {}), {}), { + 'finalize at env cleanup for simple wrap': true, + 'finalize at env cleanup for second wrap': true + }); + + // Ensure that the child exited successfully. + assert.strictEqual(child.status, 0); +} diff --git a/test/js-native-api/test_general/test_general.c b/test/js-native-api/test_general/test_general.c index a7453e42f7456b..f6e641167d5bcc 100644 --- a/test/js-native-api/test_general/test_general.c +++ b/test/js-native-api/test_general/test_general.c @@ -1,5 +1,7 @@ -#include +#include #include +#include +#include #include "../common.h" static napi_value testStrictEquals(napi_env env, napi_callback_info info) { @@ -146,16 +148,22 @@ static napi_value deref_item_was_called(napi_env env, napi_callback_info info) { return it_was_called; } -static napi_value wrap(napi_env env, napi_callback_info info) { +static napi_value wrap_first_arg(napi_env env, + napi_callback_info info, + napi_finalize finalizer, + void* data) { size_t argc = 1; napi_value to_wrap; - deref_item_called = false; - NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &to_wrap, NULL, NULL)); - NAPI_CALL(env, napi_wrap(env, to_wrap, &deref_item_called, deref_item, NULL, NULL)); + NAPI_CALL(env, napi_wrap(env, to_wrap, data, finalizer, NULL, NULL)); - return NULL; + return to_wrap; +} + +static napi_value wrap(napi_env env, napi_callback_info info) { + deref_item_called = false; + return wrap_first_arg(env, info, deref_item, &deref_item_called); } static napi_value unwrap(napi_env env, napi_callback_info info) { @@ -186,13 +194,7 @@ static void test_finalize(napi_env env, void* data, void* hint) { } static napi_value test_finalize_wrap(napi_env env, napi_callback_info info) { - size_t argc = 1; - napi_value js_object; - - NAPI_CALL(env, napi_get_cb_info(env, info, &argc, &js_object, NULL, NULL)); - NAPI_CALL(env, napi_wrap(env, js_object, NULL, test_finalize, NULL, NULL)); - - return NULL; + return wrap_first_arg(env, info, test_finalize, NULL); } static napi_value finalize_was_called(napi_env env, napi_callback_info info) { @@ -251,6 +253,34 @@ static napi_value add_finalizer_only(napi_env env, napi_callback_info info) { return NULL; } +static const char* env_cleanup_finalizer_messages[] = { + "simple wrap", + "wrap, removeWrap", + "first wrap", + "second wrap" +}; + +static void cleanup_env_finalizer(napi_env env, void* data, void* hint) { + (void) env; + (void) hint; + + printf("finalize at env cleanup for %s\n", + env_cleanup_finalizer_messages[(uintptr_t)data]); +} + +static napi_value env_cleanup_wrap(napi_env env, napi_callback_info info) { + size_t argc = 2; + napi_value argv[2]; + uint32_t value; + uintptr_t ptr_value; + NAPI_CALL(env, napi_get_cb_info(env, info, &argc, argv, NULL, NULL)); + + NAPI_CALL(env, napi_get_value_uint32(env, argv[1], &value)); + + ptr_value = value; + return wrap_first_arg(env, info, cleanup_env_finalizer, (void*)ptr_value); +} + EXTERN_C_START napi_value Init(napi_env env, napi_value exports) { napi_property_descriptor descriptors[] = { @@ -265,6 +295,7 @@ napi_value Init(napi_env env, napi_value exports) { DECLARE_NAPI_PROPERTY("testNapiErrorCleanup", testNapiErrorCleanup), DECLARE_NAPI_PROPERTY("testNapiTypeof", testNapiTypeof), DECLARE_NAPI_PROPERTY("wrap", wrap), + DECLARE_NAPI_PROPERTY("envCleanupWrap", env_cleanup_wrap), DECLARE_NAPI_PROPERTY("unwrap", unwrap), DECLARE_NAPI_PROPERTY("removeWrap", remove_wrap), DECLARE_NAPI_PROPERTY("addFinalizerOnly", add_finalizer_only),