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: tc39/proposal-weakrefs#125 (comment)
PR-URL: #28428
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

benchmark/napi/ref/addon.c

@@ -0,0 +1,82 @@
+#include <stdlib.h>
+#define NAPI_EXPERIMENTAL
+#include <node_api.h>
+
+#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;
+}

benchmark/napi/ref/binding.gyp

@@ -0,0 +1,10 @@
+{
+  'targets': [
+    {
+      'target_name': 'addon',
+      'sources': [
+        'addon.c'
+      ]
+    }
+  ]
+}

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);
+}

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<v8::Value> 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<Reference>& 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<v8::Value> _persistent;

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<v8::Value> 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;

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 <typename T>
 using Persistent = v8::Global<T>;
 

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);
+}