n-api: render finalizers as env cleanup hooks

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