feat: add support for requiring basic finalizers (#1568)

* feat: add support for requiring basic finalizers

* Address review comments
- Use passive voice in existing and new docs
- Revert unnecessary change

Author: KevinEady

.gitignore

@@ -3,6 +3,7 @@
 /benchmark/build
 /benchmark/src
 /test/addon_build/addons
+/test/require_basic_finalizers/addons
 /.vscode
 
 # ignore node-gyp generated files outside its build directory

doc/finalization.md

@@ -8,7 +8,9 @@ provide more efficient memory management, optimizations, improved execution, or
 other benefits.
 
 In general, it is best to use basic finalizers whenever possible (eg. when
-access to JavaScript is _not_ needed).
+access to JavaScript is _not_ needed). The
+`NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS` preprocessor directive can be defined
+to ensure that all finalizers are basic.
 
 ## Finalizers
 

doc/setup.md

@@ -81,12 +81,23 @@ To use **Node-API** in a native module:
 At build time, the Node-API back-compat library code will be used only when the
 targeted node version *does not* have Node-API built-in.
 
-The preprocessor directive `NODE_ADDON_API_DISABLE_DEPRECATED` can be defined at
-compile time before including `napi.h` to skip the definition of deprecated APIs.
+The `NODE_ADDON_API_DISABLE_DEPRECATED` preprocessor directive can be defined at
+compile time before including `napi.h` to skip the definition of deprecated
+APIs.
 
 By default, throwing an exception on a terminating environment (eg. worker
 threads) will cause a fatal exception, terminating the Node process. This is to
 provide feedback to the user of the runtime error, as it is impossible to pass
-the error to JavaScript when the environment is terminating. In order to bypass
-this behavior such that the Node process will not terminate, define the
-preprocessor directive `NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS`.
+the error to JavaScript when the environment is terminating.  The
+`NODE_API_SWALLOW_UNTHROWABLE_EXCEPTIONS` preprocessor directive can be defined
+to bypass this behavior, such that the Node process will not terminate.
+
+Various Node-API constructs provide a mechanism to run a callback in response to
+a garbage collection event of that object. These callbacks are called
+[_finalizers_]. Some finalizers have restrictions on the type of Node-APIs
+available within the callback. node-addon-api provides convenience helpers that
+bypass this limitation, but may cause the add-on to run less efficiently. The
+`NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS` preprocessor directive can be defined
+to disable the convenience helpers.
+
+[_finalizers_]: ./finalization.md

napi-inl.h

@@ -213,6 +213,11 @@ struct FinalizeData {
   static inline void Wrapper(node_api_nogc_env env,
                              void* data,
                              void* finalizeHint) NAPI_NOEXCEPT {
+#ifdef NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS
+    static_assert(false,
+                  "NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS defined: Finalizer "
+                  "must be basic.");
+#endif
     napi_status status =
         node_api_post_finalizer(env, WrapperGC, data, finalizeHint);
     NAPI_FATAL_IF_FAILED(
@@ -243,6 +248,11 @@ struct FinalizeData {
   static inline void WrapperWithHint(node_api_nogc_env env,
                                      void* data,
                                      void* finalizeHint) NAPI_NOEXCEPT {
+#ifdef NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS
+    static_assert(false,
+                  "NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS defined: Finalizer "
+                  "must be basic.");
+#endif
     napi_status status =
         node_api_post_finalizer(env, WrapperGCWithHint, data, finalizeHint);
     NAPI_FATAL_IF_FAILED(

test/require_basic_finalizers/index.js

@@ -0,0 +1,38 @@
+'use strict';
+
+const { promisify } = require('util');
+const exec = promisify(require('child_process').exec);
+const { copy, remove } = require('fs-extra');
+const path = require('path');
+const assert = require('assert');
+
+async function test () {
+  const addon = 'require-basic-finalizers';
+  const ADDON_FOLDER = path.join(__dirname, 'addons', addon);
+
+  await remove(ADDON_FOLDER);
+  await copy(path.join(__dirname, 'tpl'), ADDON_FOLDER);
+
+  console.log('   >Building addon');
+
+  // Fail when NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS is enabled
+  await assert.rejects(exec('npm --require-basic-finalizers install', {
+    cwd: ADDON_FOLDER
+  }), 'Addon unexpectedly compiled successfully');
+
+  // Succeed when NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS is not enabled
+  return assert.doesNotReject(exec('npm install', {
+    cwd: ADDON_FOLDER
+  }));
+}
+
+module.exports = (function () {
+  // This test will only run under an experimental version test.
+  const isExperimental = Number(process.env.NAPI_VERSION) === 2147483647;
+
+  if (isExperimental) {
+    return test();
+  } else {
+    console.log('   >Skipped (non-experimental test run)');
+  }
+})();

test/require_basic_finalizers/tpl/.npmrc

@@ -0,0 +1 @@
+package-lock=false

test/require_basic_finalizers/tpl/addon.cc

@@ -0,0 +1,12 @@
+#include <napi.h>
+
+Napi::Object Init(Napi::Env env, Napi::Object exports) {
+  exports.Set(
+      "external",
+      Napi::External<int>::New(
+          env, new int(1), [](Napi::Env /*env*/, int* data) { delete data; }));
+
+  return exports;
+}
+
+NODE_API_MODULE(NODE_GYP_MODULE_NAME, Init)

test/require_basic_finalizers/tpl/binding.gyp

@@ -0,0 +1,48 @@
+{
+  'target_defaults': {
+    'include_dirs': [
+        "<!(node -p \"require('node-addon-api').include_dir\")"
+    ],
+    'variables': {
+      'NAPI_VERSION%': "<!(node -p \"process.env.NAPI_VERSION || process.versions.napi\")",
+      'require_basic_finalizers': "<!(node -p \"process.env['npm_config_require_basic_finalizers']\")",
+    },
+    'conditions': [
+      ['NAPI_VERSION!=""', { 'defines': ['NAPI_VERSION=<@(NAPI_VERSION)'] } ],
+      ['NAPI_VERSION==2147483647', { 'defines': ['NAPI_EXPERIMENTAL'] } ],
+      ['require_basic_finalizers=="true"', {
+        'defines': ['NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS'],
+      }],
+      ['OS=="mac"', {
+        'cflags+': ['-fvisibility=hidden'],
+        'xcode_settings': {
+          'OTHER_CFLAGS': ['-fvisibility=hidden']
+        }
+      }]
+    ],
+    'sources': [
+      'addon.cc',
+    ],
+  },
+  'targets': [
+    {
+      'target_name': 'addon',
+      'defines': [
+        'NAPI_CPP_EXCEPTIONS'
+      ],
+      'cflags!': [ '-fno-exceptions' ],
+      'cflags_cc!': [ '-fno-exceptions' ],
+      'msvs_settings': {
+        'VCCLCompilerTool': {
+          'ExceptionHandling': 1,
+          'EnablePREfast': 'true',
+        },
+      },
+      'xcode_settings': {
+        'CLANG_CXX_LIBRARY': 'libc++',
+        'MACOSX_DEPLOYMENT_TARGET': '10.7',
+        'GCC_ENABLE_CPP_EXCEPTIONS': 'YES',
+      },
+    }
+  ]
+}

test/require_basic_finalizers/tpl/index.js

@@ -0,0 +1,3 @@
+'use strict';
+
+module.exports = require('bindings')('addon');

test/require_basic_finalizers/tpl/package.json

@@ -0,0 +1,11 @@
+{
+  "name": "addon",
+  "version": "0.0.0",
+  "description": "Node.js addon",
+  "private": true,
+  "scripts": {},
+  "dependencies": {
+    "node-addon-api": "file:../../../../"
+  },
+  "gypfile": true
+}