worker: improve integration with native addons

Allow loading add-ons from multiple Node.js instances if they are
declared context-aware; in particular, this applies to N-API addons.

Also, plug a memory leak that occurred when registering N-API addons.

Refs: nodejs#23319

Author: addaleax

doc/api/addons.md

@@ -251,6 +251,11 @@ down. If necessary, such hooks can be removed using
 `RemoveEnvironmentCleanupHook()` before they are run, which has the same
 signature.
 
+In order to be loaded from multiple Node.js environments,
+such as a main thread and a Worker thread, an add-on needs to either:
+- Be an N-API addon, or
+- Be declared as context-aware using `NODE_MODULE_INIT()` as described above
+
 ### Building
 
 Once the source code has been written, it must be compiled into the binary

doc/api/worker_threads.md

@@ -360,11 +360,12 @@ Notable differences inside a Worker environment are:
   being invoked.
 - IPC channels from parent processes are not accessible.
 - The [`trace_events`][] module is not supported.
+- Native add-ons can only be loaded from multiple threads if they fulfill
+  [certain conditions][Addons worker support].
 
 Currently, the following differences also exist until they are addressed:
 
 - The [`inspector`][] module is not available yet.
-- Native addons are not supported yet.
 
 Creating `Worker` instances inside of other `Worker`s is possible.
 
@@ -602,6 +603,7 @@ active handle in the event system. If the worker is already `unref()`ed calling
 [`worker.postMessage()`]: #worker_threads_worker_postmessage_value_transferlist
 [`worker.terminate()`]: #worker_threads_worker_terminate_callback
 [`worker.threadId`]: #worker_threads_worker_threadid_1
+[Addons worker support]: addons.html#addons_worker_support
 [HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm
 [Signals events]: process.html#process_signal_events
 [Web Workers]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API

src/node_api.cc

@@ -484,7 +484,7 @@ void napi_module_register_by_symbol(v8::Local<v8::Object> exports,
 void napi_module_register(napi_module* mod) {
   node::node_module* nm = new node::node_module {
     -1,
-    mod->nm_flags,
+    mod->nm_flags | NM_F_DELETEME,
     nullptr,
     mod->nm_filename,
     nullptr,

src/node_binding.cc

@@ -103,7 +103,6 @@ using v8::Value;
 // Globals per process
 static node_module* modlist_internal;
 static node_module* modlist_linked;
-static node_module* modlist_addon;
 static uv_once_t init_modpending_once = UV_ONCE_INIT;
 static uv_key_t thread_local_modpending;
 
@@ -129,6 +128,48 @@ extern "C" void node_module_register(void* m) {
 
 namespace binding {
 
+static struct global_handle_map_t {
+ public:
+  void set(void* handle, node_module* mod) {
+    CHECK_NE(handle, nullptr);
+    Mutex::ScopedLock lock(mutex_);
+
+    map_[handle] = Entry { 1, mod };
+  }
+
+  node_module* get_and_increase_refcount(void* handle) {
+    CHECK_NE(handle, nullptr);
+    Mutex::ScopedLock lock(mutex_);
+
+    auto it = map_.find(handle);
+    if (it == map_.end()) return nullptr;
+    it->second.refcount++;
+    return it->second.module;
+  }
+
+  void erase(void* handle) {
+    CHECK_NE(handle, nullptr);
+    Mutex::ScopedLock lock(mutex_);
+
+    auto it = map_.find(handle);
+    if (it == map_.end()) return;
+    CHECK_GE(it->second.refcount, 1);
+    if (--it->second.refcount == 0) {
+      if (it->second.module->nm_flags & NM_F_DELETEME)
+        delete it->second.module;
+      map_.erase(handle);
+    }
+  }
+
+ private:
+  Mutex mutex_;
+  struct Entry {
+    unsigned int refcount;
+    node_module* module;
+  };
+  std::unordered_map<void*, Entry> map_;
+} global_handle_map;
+
 DLib::DLib(const char* filename, int flags)
     : filename_(filename), flags_(flags), handle_(nullptr) {}
 
@@ -142,6 +183,7 @@ bool DLib::Open() {
 
 void DLib::Close() {
   if (handle_ == nullptr) return;
+  global_handle_map.erase(handle_);
   dlclose(handle_);
   handle_ = nullptr;
 }
@@ -233,7 +275,7 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
     // Objects containing v14 or later modules will have registered themselves
     // on the pending list.  Activate all of them now.  At present, only one
     // module per object is supported.
-    node_module* const mp =
+    node_module* mp =
         static_cast<node_module*>(uv_key_get(&thread_local_modpending));
     uv_key_set(&thread_local_modpending, nullptr);
 
@@ -250,17 +292,24 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
       return false;
     }
 
-    if (mp == nullptr) {
+    if (mp != nullptr) {
+      mp->nm_dso_handle = dlib->handle_;
+      global_handle_map.set(dlib->handle_, mp);
+    } else {
       if (auto callback = GetInitializerCallback(dlib)) {
         callback(exports, module, context);
+        return true;
       } else if (auto napi_callback = GetNapiInitializerCallback(dlib)) {
         napi_module_register_by_symbol(exports, module, context, napi_callback);
+        return true;
       } else {
-        dlib->Close();
-        env->ThrowError("Module did not self-register.");
-        return false;
+        mp = global_handle_map.get_and_increase_refcount(dlib->handle_);
+        if (mp == nullptr || mp->nm_context_register_func == nullptr) {
+          dlib->Close();
+          env->ThrowError("Module did not self-register.");
+          return false;
+        }
       }
-      return true;
     }
 
     // -1 is used for N-API modules
@@ -293,10 +342,6 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
     }
     CHECK_EQ(mp->nm_flags & NM_F_BUILTIN, 0);
 
-    mp->nm_dso_handle = dlib->handle_;
-    mp->nm_link = modlist_addon;
-    modlist_addon = mp;
-
     if (mp->nm_context_register_func != nullptr) {
       mp->nm_context_register_func(exports, module, context, mp->nm_priv);
     } else if (mp->nm_register_func != nullptr) {

src/node_binding.h

@@ -18,6 +18,7 @@ enum {
   NM_F_BUILTIN = 1 << 0,  // Unused.
   NM_F_LINKED = 1 << 1,
   NM_F_INTERNAL = 1 << 2,
+  NM_F_DELETEME = 1 << 3,
 };
 
 #define NODE_MODULE_CONTEXT_AWARE_CPP(modname, regfunc, priv, flags)           \

test/addons/dlopen-ping-pong/test-worker.js

@@ -0,0 +1,16 @@
+'use strict';
+const common = require('../../common');
+const assert = require('assert');
+const { Worker } = require('worker_threads');
+
+// Check that modules that are not declared as context-aware cannot be re-loaded
+// from workers.
+
+const bindingPath = require.resolve(`./build/${common.buildType}/binding`);
+require(bindingPath);
+
+new Worker(`require(${JSON.stringify(bindingPath)})`, { eval: true })
+  .on('error', common.mustCall((err) => {
+    assert.strictEqual(err.constructor, Error);
+    assert.strictEqual(err.message, 'Module did not self-register.');
+  }));

test/addons/worker-addon/test.js

@@ -6,21 +6,30 @@ const path = require('path');
 const { Worker } = require('worker_threads');
 const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`);
 
-if (process.argv[2] === 'worker') {
-  new Worker(`require(${JSON.stringify(binding)});`, { eval: true });
-  return;
-} else if (process.argv[2] === 'main-thread') {
-  process.env.addExtraItemToEventLoop = 'yes';
-  require(binding);
-  return;
+switch (process.argv[2]) {
+  case 'both':
+    require(binding);
+    // fallthrough
+  case 'worker':
+    new Worker(`require(${JSON.stringify(binding)});`, { eval: true });
+    return;
+  case 'main-thread':
+    process.env.addExtraItemToEventLoop = 'yes';
+    require(binding);
+    return;
 }
 
-for (const test of ['worker', 'main-thread']) {
+for (const test of ['worker', 'main-thread', 'both']) {
   const proc = child_process.spawnSync(process.execPath, [
     __filename,
     test
   ]);
   assert.strictEqual(proc.stderr.toString(), '');
-  assert.strictEqual(proc.stdout.toString(), 'ctor cleanup dtor');
+  // We always only have 1 instance of the shared object in memory, so
+  // 1 ctor and 1 dtor call. If we attach the module to 2 Environments,
+  // we expect 2 cleanup calls, otherwise one.
+  assert.strictEqual(
+    proc.stdout.toString(),
+    test === 'both' ? 'ctor cleanup cleanup dtor' : 'ctor cleanup dtor');
   assert.strictEqual(proc.status, 0);
 }

test/node-api/1_hello_world/test.js

@@ -1,6 +1,8 @@
 'use strict';
 const common = require('../../common');
 const assert = require('assert');
+const { Worker } = require('worker_threads');
+
 const bindingPath = require.resolve(`./build/${common.buildType}/binding`);
 const binding = require(bindingPath);
 assert.strictEqual(binding.hello(), 'world');
@@ -11,3 +13,10 @@ delete require.cache[bindingPath];
 const rebinding = require(bindingPath);
 assert.strictEqual(rebinding.hello(), 'world');
 assert.notStrictEqual(binding.hello, rebinding.hello);
+
+// Test that workers can load addons declared using NAPI_MODULE_INIT().
+new Worker(`
+const { parentPort } = require('worker_threads');
+const msg = require(${JSON.stringify(bindingPath)}).hello();
+parentPort.postMessage(msg)`, { eval: true })
+  .on('message', common.mustCall((msg) => assert.strictEqual(msg, 'world')));

test/node-api/test_worker_terminate/test.js

@@ -4,6 +4,11 @@ const assert = require('assert');
 const { Worker, isMainThread, workerData } = require('worker_threads');
 
 if (isMainThread) {
+  // Load the addon in the main thread first.
+  // This checks that N-API addons can be loaded from multiple contexts
+  // when they are not loaded through NAPI_MODULE().
+  require(`./build/${common.buildType}/test_worker_terminate`);
+
   const counter = new Int32Array(new SharedArrayBuffer(4));
   const worker = new Worker(__filename, { workerData: { counter } });
   worker.on('exit', common.mustCall(() => {

test/node-api/test_worker_terminate/test_worker_terminate.c

@@ -36,4 +36,6 @@ napi_value Init(napi_env env, napi_value exports) {
   return exports;
 }
 
+// Do not start using NAPI_MODULE_INIT() here, so that we can test
+// compatibility of Workers with NAPI_MODULE().
 NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)