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: https://github.com/nodejs/node/pull/23319

PR-URL: https://github.com/nodejs/node/pull/26175
Fixes: https://github.com/nodejs/node/issues/21481
Fixes: https://github.com/nodejs/node/issues/21783
Fixes: https://github.com/nodejs/node/issues/25662
Fixes: https://github.com/nodejs/node/issues/20239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

11 files changed, 322 insertions, 29 deletions

diff --git a/doc/api/addons.md b/doc/api/addons.md
index 4941d3de9a..1c811b50c2 100644
--- a/doc/api/addons.md
+++ b/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
diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md
index 95cdae5f9a..54bd66a6ea 100644
--- a/doc/api/worker_threads.md
+++ b/doc/api/worker_threads.md
@@ -360,11 +360,8 @@ Notable differences inside a Worker environment are:
   being invoked.
 - IPC channels from parent processes are not accessible.
 - The [`trace_events`][] module is not supported.
-
-Currently, the following differences also exist until they are addressed:
-
-- The [`inspector`][] module is not available yet.
-- Native addons are not supported yet.
+- Native add-ons can only be loaded from multiple threads if they fulfill
+  [certain conditions][Addons worker support].
 
 Creating `Worker` instances inside of other `Worker`s is possible.
 
@@ -580,7 +577,6 @@ active handle in the event system. If the worker is already `unref()`ed calling
 [`WebAssembly.Module`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/Module
 [`Worker`]: #worker_threads_class_worker
 [`cluster` module]: cluster.html
-[`inspector`]: inspector.html
 [`port.on('message')`]: #worker_threads_event_message
 [`port.postMessage()`]: #worker_threads_port_postmessage_value_transferlist
 [`process.abort()`]: process.html#process_process_abort
@@ -602,6 +598,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
diff --git a/src/node_api.cc b/src/node_api.cc
index 4e1a779c42..bfc0c12aad 100644
--- a/src/node_api.cc
+++ b/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,
diff --git a/src/node_binding.cc b/src/node_binding.cc
index c9ff7be46f..7b87ba6bff 100644
--- a/src/node_binding.cc
+++ b/src/node_binding.cc
@@ -2,6 +2,7 @@
 #include "env-inl.h"
 #include "node_native_module.h"
 #include "util.h"
+#include <atomic>
 
 #if HAVE_OPENSSL
 #define NODE_BUILTIN_OPENSSL_MODULES(V) V(crypto) V(tls_wrap)
@@ -96,6 +97,130 @@
 NODE_BUILTIN_MODULES(V)
 #undef V
 
+#ifdef _AIX
+// On AIX, dlopen() behaves differently from other operating systems, in that
+// it returns unique values from each call, rather than identical values, when
+// loading the same handle.
+// We try to work around that by providing wrappers for the dlopen() family of
+// functions, and using st_dev and st_ino for the file that is to be loaded
+// as keys for a cache.
+
+namespace node {
+namespace dlwrapper {
+
+struct dl_wrap {
+  uint64_t st_dev;
+  uint64_t st_ino;
+  uint64_t refcount;
+  void* real_handle;
+
+  struct hash {
+    size_t operator()(const dl_wrap* wrap) const {
+      return std::hash<uint64_t>()(wrap->st_dev) ^
+             std::hash<uint64_t>()(wrap->st_ino);
+    }
+  };
+
+  struct equal {
+    bool operator()(const dl_wrap* a,
+                    const dl_wrap* b) const {
+      return a->st_dev == b->st_dev && a->st_ino == b->st_ino;
+    }
+  };
+};
+
+static Mutex dlhandles_mutex;
+static std::unordered_set<dl_wrap*, dl_wrap::hash, dl_wrap::equal>
+    dlhandles;
+static thread_local std::string dlerror_storage;
+
+char* wrapped_dlerror() {
+  return &dlerror_storage[0];
+}
+
+void* wrapped_dlopen(const char* filename, int flags) {
+  CHECK_NOT_NULL(filename);  // This deviates from the 'real' dlopen().
+  Mutex::ScopedLock lock(dlhandles_mutex);
+
+  uv_fs_t req;
+  OnScopeLeave cleanup([&]() { uv_fs_req_cleanup(&req); });
+  int rc = uv_fs_stat(nullptr, &req, filename, nullptr);
+
+  if (rc != 0) {
+    dlerror_storage = uv_strerror(rc);
+    return nullptr;
+  }
+
+  dl_wrap search = {
+    req.statbuf.st_dev,
+    req.statbuf.st_ino,
+    0, nullptr
+  };
+
+  auto it = dlhandles.find(&search);
+  if (it != dlhandles.end()) {
+    (*it)->refcount++;
+    return *it;
+  }
+
+  void* real_handle = dlopen(filename, flags);
+  if (real_handle == nullptr) {
+    dlerror_storage = dlerror();
+    return nullptr;
+  }
+  dl_wrap* wrap = new dl_wrap();
+  wrap->st_dev = req.statbuf.st_dev;
+  wrap->st_ino = req.statbuf.st_ino;
+  wrap->refcount = 1;
+  wrap->real_handle = real_handle;
+  dlhandles.insert(wrap);
+  return wrap;
+}
+
+int wrapped_dlclose(void* handle) {
+  Mutex::ScopedLock lock(dlhandles_mutex);
+  dl_wrap* wrap = static_cast<dl_wrap*>(handle);
+  int ret = 0;
+  CHECK_GE(wrap->refcount, 1);
+  if (--wrap->refcount == 0) {
+    ret = dlclose(wrap->real_handle);
+    if (ret != 0) dlerror_storage = dlerror();
+    dlhandles.erase(wrap);
+    delete wrap;
+  }
+  return ret;
+}
+
+void* wrapped_dlsym(void* handle, const char* symbol) {
+  if (handle == RTLD_DEFAULT || handle == RTLD_NEXT)
+    return dlsym(handle, symbol);
+  dl_wrap* wrap = static_cast<dl_wrap*>(handle);
+  return dlsym(wrap->real_handle, symbol);
+}
+
+#define dlopen node::dlwrapper::wrapped_dlopen
+#define dlerror node::dlwrapper::wrapped_dlerror
+#define dlclose node::dlwrapper::wrapped_dlclose
+#define dlsym node::dlwrapper::wrapped_dlsym
+
+}  // namespace dlwrapper
+}  // namespace node
+
+#endif  // _AIX
+
+#ifdef __linux__
+static bool libc_may_be_musl() {
+  static std::atomic_bool retval;  // Cache the return value.
+  static std::atomic_bool has_cached_retval { false };
+  if (has_cached_retval) return retval;
+  retval = dlsym(RTLD_DEFAULT, "gnu_get_libc_version") == nullptr;
+  has_cached_retval = true;
+  return retval;
+}
+#else  // __linux__
+static bool libc_may_be_musl() { return false; }
+#endif  // __linux__
+
 namespace node {
 
 using v8::Context;
@@ -110,7 +235,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;
 
@@ -136,6 +260,57 @@ 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].module = mod;
+    // We need to store this flag internally to avoid a chicken-and-egg problem
+    // during cleanup. By the time we actually use the flag's value,
+    // the shared object has been unloaded, and its memory would be gone,
+    // making it impossible to access fields of `mod` --
+    // unless `mod` *is* dynamically allocated, but we cannot know that
+    // without checking the flag.
+    map_[handle].wants_delete_module = mod->nm_flags & NM_F_DELETEME;
+    map_[handle].refcount++;
+  }
+
+  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.wants_delete_module)
+        delete it->second.module;
+      map_.erase(handle);
+    }
+  }
+
+ private:
+  Mutex mutex_;
+  struct Entry {
+    unsigned int refcount;
+    bool wants_delete_module;
+    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) {}
 
@@ -149,7 +324,21 @@ bool DLib::Open() {
 
 void DLib::Close() {
   if (handle_ == nullptr) return;
-  dlclose(handle_);
+
+  if (libc_may_be_musl()) {
+    // musl libc implements dlclose() as a no-op which returns 0.
+    // As a consequence, trying to re-load a previously closed addon at a later
+    // point will not call its static constructors, which Node.js uses.
+    // Therefore, when we may be using musl libc, we assume that the shared
+    // object exists indefinitely and keep it in our handle map.
+    return;
+  }
+
+  int err = dlclose(handle_);
+  if (err == 0) {
+    if (has_entry_in_global_handle_map_)
+      global_handle_map.erase(handle_);
+  }
   handle_ = nullptr;
 }
 
@@ -170,6 +359,8 @@ bool DLib::Open() {
 
 void DLib::Close() {
   if (handle_ == nullptr) return;
+  if (has_entry_in_global_handle_map_)
+    global_handle_map.erase(handle_);
   uv_dlclose(&lib_);
   handle_ = nullptr;
 }
@@ -181,6 +372,16 @@ void* DLib::GetSymbolAddress(const char* name) {
 }
 #endif  // !__POSIX__
 
+void DLib::SaveInGlobalHandleMap(node_module* mp) {
+  has_entry_in_global_handle_map_ = true;
+  global_handle_map.set(handle_, mp);
+}
+
+node_module* DLib::GetSavedModuleFromGlobalHandleMap() {
+  has_entry_in_global_handle_map_ = true;
+  return global_handle_map.get_and_increase_refcount(handle_);
+}
+
 using InitializerCallback = void (*)(Local<Object> exports,
                                      Local<Value> module,
                                      Local<Context> context);
@@ -235,12 +436,15 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
 
   node::Utf8Value filename(env->isolate(), args[1]);  // Cast
   env->TryLoadAddon(*filename, flags, [&](DLib* dlib) {
+    static Mutex dlib_load_mutex;
+    Mutex::ScopedLock lock(dlib_load_mutex);
+
     const bool is_opened = dlib->Open();
 
     // 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);
 
@@ -257,17 +461,24 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
       return false;
     }
 
-    if (mp == nullptr) {
+    if (mp != nullptr) {
+      mp->nm_dso_handle = dlib->handle_;
+      dlib->SaveInGlobalHandleMap(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 = dlib->GetSavedModuleFromGlobalHandleMap();
+        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
@@ -300,10 +511,8 @@ 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;
-
+    // Do not keep the lock while running userland addon loading code.
+    Mutex::ScopedUnlock unlock(lock);
     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) {
diff --git a/src/node_binding.h b/src/node_binding.h
index d73e18548f..fcd7f34ac6 100644
--- a/src/node_binding.h
+++ b/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)           \
@@ -62,6 +63,8 @@ class DLib {
   bool Open();
   void Close();
   void* GetSymbolAddress(const char* name);
+  void SaveInGlobalHandleMap(node_module* mp);
+  node_module* GetSavedModuleFromGlobalHandleMap();
 
   const std::string filename_;
   const int flags_;
@@ -70,6 +73,7 @@ class DLib {
 #ifndef __POSIX__
   uv_lib_t lib_;
 #endif
+  bool has_entry_in_global_handle_map_ = false;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(DLib);
diff --git a/test/addons/dlopen-ping-pong/test-worker.js b/test/addons/dlopen-ping-pong/test-worker.js
new file mode 100644
index 0000000000..feba6aa5eb
--- /dev/null
+++ b/test/addons/dlopen-ping-pong/test-worker.js
@@ -0,0 +1,20 @@
+'use strict';
+const common = require('../../common');
+
+if (common.isWindows)
+  common.skip('dlopen global symbol loading is not supported on this os.');
+
+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.');
+  }));
diff --git a/test/addons/worker-addon/binding.cc b/test/addons/worker-addon/binding.cc
index faf3ba8647..01c857c43e 100644
--- a/test/addons/worker-addon/binding.cc
+++ b/test/addons/worker-addon/binding.cc
@@ -21,7 +21,7 @@ struct statically_allocated {
   }
   ~statically_allocated() {
     assert(count == 0);
-    printf("dtor");
+    printf("dtor ");
   }
 } var;
 
diff --git a/test/addons/worker-addon/test.js b/test/addons/worker-addon/test.js
index 7fb7c1a87a..ef158e98b1 100644
--- a/test/addons/worker-addon/test.js
+++ b/test/addons/worker-addon/test.js
@@ -1,3 +1,4 @@
+// Flags: --experimental-report
 'use strict';
 const common = require('../../common');
 const assert = require('assert');
@@ -6,21 +7,62 @@ 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-twice':
+  case 'worker':
+    const worker = new Worker(`require(${JSON.stringify(binding)});`, {
+      eval: true
+    });
+    if (process.argv[2] === 'worker-twice') {
+      worker.on('exit', common.mustCall(() => {
+        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']) {
+// Use process.report to figure out if we might be running under musl libc.
+const glibc = JSON.parse(process.report.getReport()).header.glibcVersionRuntime;
+assert(typeof glibc === 'string' || glibc === undefined, glibc);
+
+const libcMayBeMusl = common.isLinux && glibc === undefined;
+
+for (const { test, expected } of [
+  { test: 'worker', expected: [ 'ctor cleanup dtor ' ] },
+  { test: 'main-thread', expected: [ '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.
+  { test: 'both', expected: [ 'ctor cleanup cleanup dtor ' ] },
+  {
+    test: 'worker-twice',
+    // In this case, we load and unload an addon, then load and unload again.
+    // musl doesn't support unloading, so the output may be missing
+    // a dtor + ctor pair.
+    expected: [
+      'ctor cleanup dtor ctor cleanup dtor '
+    ].concat(libcMayBeMusl ? [
+      'ctor cleanup cleanup dtor ',
+    ] : [])
+  },
+]) {
+  console.log('spawning test', test);
   const proc = child_process.spawnSync(process.execPath, [
     __filename,
     test
   ]);
+  process.stderr.write(proc.stderr.toString());
   assert.strictEqual(proc.stderr.toString(), '');
-  assert.strictEqual(proc.stdout.toString(), 'ctor cleanup dtor');
+  assert(expected.includes(proc.stdout.toString()),
+         `${proc.stdout.toString()} is not included in ${expected}`);
   assert.strictEqual(proc.status, 0);
 }
diff --git a/test/node-api/1_hello_world/test.js b/test/node-api/1_hello_world/test.js
index d1d67d34f6..dd28e26a56 100644
--- a/test/node-api/1_hello_world/test.js
+++ b/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')));
diff --git a/test/node-api/test_worker_terminate/test.js b/test/node-api/test_worker_terminate/test.js
index 2bfaab8e8e..7c7224c073 100644
--- a/test/node-api/test_worker_terminate/test.js
+++ b/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(() => {
diff --git a/test/node-api/test_worker_terminate/test_worker_terminate.c b/test/node-api/test_worker_terminate/test_worker_terminate.c
index a88d936293..517cae4203 100644
--- a/test/node-api/test_worker_terminate/test_worker_terminate.c
+++ b/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)