worker: improve integration with native addons

doc/api/addons.md

@@ -232,6 +232,23 @@ NODE_MODULE_INIT(/* exports, module, context */) {
 }
 ```
 
+#### Worker support
+
+In order to support [`Worker`][] threads, addons need to clean up any resources
+they may have allocated when such a thread exists. This can be achieved through
+the usage of the `AddEnvironmentCleanupHook()` function:
+
+```c++
+void AddEnvironmentCleanupHook(v8::Isolate* isolate,
+                               void (*fun)(void* arg),
+                               void* arg);
+```
+
+This function adds a hook that will run before a given Node.js instance shuts
+down. If necessary, such hooks can be removed using
+`RemoveEnvironmentCleanupHook()` before they are run, which has the same
+signature.
+
 ### Building
 
 Once the source code has been written, it must be compiled into the binary
@@ -1316,6 +1333,7 @@ Test in JavaScript by running:
 require('./build/Release/addon');
 ```
 
+[`Worker`]: worker_threads.html#worker_threads_class_worker
 [Electron]: https://electronjs.org/
 [Embedder's Guide]: https://github.com/v8/v8/wiki/Embedder's%20Guide
 [Linking to Node.js' own dependencies]: #addons_linking_to_node_js_own_dependencies

doc/api/worker_threads.md

@@ -253,11 +253,9 @@ Notable differences inside a Worker environment are:
 - Execution may stop at any point as a result of [`worker.terminate()`][]
   being invoked.
 - IPC channels from parent processes are not accessible.
-
-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 are unloaded if they are only used inside `Worker` instances
+  when those workers shut down.
+  See the [addons documentation][] for more detials.
 
 Creating `Worker` instances inside of other `Worker`s is possible.
 
@@ -484,7 +482,7 @@ active handle in the event system. If the worker is already `unref()`ed calling
 [`require('worker_threads').parentPort`]: #worker_threads_worker_parentport
 [`require('worker_threads').threadId`]: #worker_threads_worker_threadid
 [`cluster` module]: cluster.html
-[`inspector`]: inspector.html
+[addons documentation]: addons.html#addons_worker_support
 [v8.serdes]: v8.html#v8_serialization_api
 [`SharedArrayBuffer`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/SharedArrayBuffer
 [Signals events]: process.html#process_signal_events

src/node.cc

@@ -1135,7 +1135,17 @@ node_module* get_linked_module(const char* name) {
   return FindModule(modlist_linked, name, NM_F_LINKED);
 }
 
-class DLib {
+namespace {
+
+Mutex dlib_mutex;
+
+class DLib;
+
+std::unordered_map<std::string, std::shared_ptr<DLib>> dlopen_cache;
+std::unordered_map<decltype(uv_lib_t().handle), std::shared_ptr<DLib>>
+    handle_to_dlib;
+
+class DLib : public std::enable_shared_from_this<DLib> {
  public:
 #ifdef __POSIX__
   static const int kDefaultFlags = RTLD_LAZY;
@@ -1146,17 +1156,27 @@ class DLib {
   inline DLib(const char* filename, int flags)
       : filename_(filename), flags_(flags), handle_(nullptr) {}
 
+  inline DLib() {}
+  inline ~DLib() {
+    Close();
+  }
+
   inline bool Open();
   inline void Close();
   inline void* GetSymbolAddress(const char* name);
+  inline void AddEnvironment(Environment* env);
 
   const std::string filename_;
-  const int flags_;
+  const int flags_ = 0;
   std::string errmsg_;
-  void* handle_;
+  void* handle_ = nullptr;
+  std::unordered_set<Environment*> users_;
+  node_module* own_info = nullptr;
+
 #ifndef __POSIX__
   uv_lib_t lib_;
 #endif
+
  private:
   DISALLOW_COPY_AND_ASSIGN(DLib);
 };
@@ -1225,18 +1245,49 @@ void InitModpendingOnce() {
   CHECK_EQ(0, uv_key_create(&thread_local_modpending));
 }
 
+void DLib::AddEnvironment(Environment* env) {
+  if (users_.count(env) > 0) return;
+  users_.insert(env);
+  if (env->is_main_thread()) return;
+  struct cleanup_hook_data {
+    std::shared_ptr<DLib> info;
+    Environment* env;
+  };
+  env->AddCleanupHook([](void* arg) {
+    Mutex::ScopedLock lock(dlib_mutex);
+    std::unique_ptr<cleanup_hook_data> cbdata(
+        static_cast<cleanup_hook_data*>(arg));
+    std::shared_ptr<DLib> info = cbdata->info;
+    info->users_.erase(cbdata->env);
+
+    if (info->users_.empty()) {
+      // No more Environments left using this DLib -> remove filename from
+      // caches.
+      std::vector<std::string> filenames;
+
+      for (const auto& entry : dlopen_cache) {
+        if (entry.second == info)
+          filenames.push_back(entry.first);
+      }
+      for (const std::string& filename : filenames)
+        dlopen_cache.erase(filename);
+
+      handle_to_dlib.erase(info->handle_);
+    }
+  }, static_cast<void*>(new cleanup_hook_data { shared_from_this(), env }));
+}
+
+}  // anonymous namespace
+
 // DLOpen is process.dlopen(module, filename, flags).
 // Used to load 'module.node' dynamically shared objects.
-//
-// FIXME(bnoordhuis) Not multi-context ready. TBD how to resolve the conflict
-// when two contexts try to load the same shared object. Maybe have a shadow
-// cache that's a plain C list or hash table that's shared across contexts?
 static void DLOpen(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-  auto context = env->context();
-
   uv_once(&init_modpending_once, InitModpendingOnce);
-  CHECK_NULL(uv_key_get(&thread_local_modpending));
+
+  Local<Context> context = env->context();
+  node_module* mp;
+  std::shared_ptr<DLib> dlib;
 
   if (args.Length() < 2) {
     env->ThrowError("process.dlopen needs at least 2 arguments.");
@@ -1257,83 +1308,126 @@ static void DLOpen(const FunctionCallbackInfo<Value>& args) {
     return;  // Exception pending.
   }
 
-  node::Utf8Value filename(env->isolate(), args[1]);  // Cast
-  DLib dlib(*filename, flags);
-  bool is_opened = dlib.Open();
+  // Use a do { ... } while(false) loop to be able to break out early
+  // if a cached entry was found.
+  do {
+    CHECK_NULL(uv_key_get(&thread_local_modpending));
+    Mutex::ScopedLock lock(dlib_mutex);
+
+    if (args.Length() < 2) {
+      env->ThrowError("process.dlopen needs at least 2 arguments.");
+      return;
+    }
+
+    int32_t flags = DLib::kDefaultFlags;
+    if (args.Length() > 2 && !args[2]->Int32Value(context).To(&flags)) {
+      return env->ThrowTypeError("flag argument must be an integer.");
+    }
+
+    node::Utf8Value filename(env->isolate(), args[1]);  // Cast
+    auto it = dlopen_cache.find(*filename);
+
+    if (it != dlopen_cache.end()) {
+      dlib = it->second;
+      mp = dlib->own_info;
+      dlib->AddEnvironment(env);
+      break;
+    }
+
+    dlib = std::make_shared<DLib>(*filename, flags);
+    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 = static_cast<node_module*>(
-      uv_key_get(&thread_local_modpending));
-  uv_key_set(&thread_local_modpending, nullptr);
+    if (is_opened && handle_to_dlib.count(dlib->handle_) > 0) {
+      dlib = handle_to_dlib[dlib->handle_];
+      mp = dlib->own_info;
+      dlib->AddEnvironment(env);
+      break;
+    }
 
-  if (!is_opened) {
-    Local<String> errmsg = OneByteString(env->isolate(), dlib.errmsg_.c_str());
-    dlib.Close();
+    // 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.
+    mp = static_cast<node_module*>(uv_key_get(&thread_local_modpending));
+    uv_key_set(&thread_local_modpending, nullptr);
+
+    if (!is_opened) {
+      Local<String> errmsg =
+          OneByteString(env->isolate(), dlib->errmsg_.c_str());
 #ifdef _WIN32
-    // Windows needs to add the filename into the error message
-    errmsg = String::Concat(
-        env->isolate(), errmsg, args[1]->ToString(context).ToLocalChecked());
+      // Windows needs to add the filename into the error message
+      errmsg = String::Concat(env->isolate(),
+                              errmsg,
+                              args[1]->ToString(context).ToLocalChecked());
 #endif  // _WIN32
-    env->isolate()->ThrowException(Exception::Error(errmsg));
-    return;
-  }
+      env->isolate()->ThrowException(Exception::Error(errmsg));
+      return;
+    }
 
-  if (mp == nullptr) {
-    if (auto callback = GetInitializerCallback(&dlib)) {
-      callback(exports, module, context);
-    } else if (auto napi_callback = GetNapiInitializerCallback(&dlib)) {
-      napi_module_register_by_symbol(exports, module, context, napi_callback);
-    } else {
-      dlib.Close();
-      env->ThrowError("Module did not self-register.");
+    if (mp == nullptr) {
+      if (auto callback = GetInitializerCallback(dlib.get())) {
+        callback(exports, module, context);
+      } else if (auto napi_callback = GetNapiInitializerCallback(dlib.get())) {
+        napi_module_register_by_symbol(exports, module, context, napi_callback);
+      } else {
+        dlib->Close();
+        env->ThrowError("Module did not self-register.");
+      }
+      return;
     }
-    return;
-  }
 
-  // -1 is used for N-API modules
-  if ((mp->nm_version != -1) && (mp->nm_version != NODE_MODULE_VERSION)) {
-    // Even if the module did self-register, it may have done so with the wrong
-    // version. We must only give up after having checked to see if it has an
-    // appropriate initializer callback.
-    if (auto callback = GetInitializerCallback(&dlib)) {
-      callback(exports, module, context);
+    // -1 is used for N-API modules
+    if ((mp->nm_version != -1) && (mp->nm_version != NODE_MODULE_VERSION)) {
+      // Even if the module did self-register, it may have done so with the
+      // wrong version. We must only give up after having checked to see if it
+      // has an appropriate initializer callback.
+      if (auto callback = GetInitializerCallback(dlib.get())) {
+        callback(exports, module, context);
+        return;
+      }
+      char errmsg[1024];
+      snprintf(errmsg,
+               sizeof(errmsg),
+               "The module '%s'"
+               "\nwas compiled against a different Node.js version using"
+               "\nNODE_MODULE_VERSION %d. This version of Node.js requires"
+               "\nNODE_MODULE_VERSION %d. Please try re-compiling or "
+               "re-installing\nthe module (for instance, using `npm rebuild` "
+               "or `npm install`).",
+               *filename, mp->nm_version, NODE_MODULE_VERSION);
+
+      // NOTE: `mp` is allocated inside of the shared library's memory,
+      // calling `dlclose` will deallocate it
+      env->ThrowError(errmsg);
       return;
     }
-    char errmsg[1024];
-    snprintf(errmsg,
-             sizeof(errmsg),
-             "The module '%s'"
-             "\nwas compiled against a different Node.js version using"
-             "\nNODE_MODULE_VERSION %d. This version of Node.js requires"
-             "\nNODE_MODULE_VERSION %d. Please try re-compiling or "
-             "re-installing\nthe module (for instance, using `npm rebuild` "
-             "or `npm install`).",
-             *filename, mp->nm_version, NODE_MODULE_VERSION);
-
-    // NOTE: `mp` is allocated inside of the shared library's memory, calling
-    // `dlclose` will deallocate it
-    dlib.Close();
-    env->ThrowError(errmsg);
-    return;
-  }
-  if (mp->nm_flags & NM_F_BUILTIN) {
-    dlib.Close();
-    env->ThrowError("Built-in module self-registered.");
-    return;
-  }
 
-  mp->nm_dso_handle = dlib.handle_;
-  mp->nm_link = modlist_addon;
-  modlist_addon = mp;
+    if (mp->nm_flags & NM_F_BUILTIN) {
+      env->ThrowError("Built-in module self-registered.");
+      return;
+    }
+
+    if (mp->nm_context_register_func == nullptr &&
+        mp->nm_register_func == nullptr) {
+      env->ThrowError("Module has no declared entry point.");
+      return;
+    }
+
+    dlib->own_info = mp;
+    handle_to_dlib[dlib->handle_] = dlib;
+    dlopen_cache[*filename] = dlib;
+
+    dlib->AddEnvironment(env);
+
+    mp->nm_dso_handle = dlib->handle_;
+    mp->nm_link = modlist_addon;
+    modlist_addon = mp;
+  } while (false);
 
   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) {
     mp->nm_register_func(exports, module, mp->nm_priv);
   } else {
-    dlib.Close();
     env->ThrowError("Module has no declared entry point.");
     return;
   }

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

@@ -14,7 +14,5 @@ process.dlopen(module, bindingPath,
 module.exports.load(`${path.dirname(bindingPath)}/ping.so`);
 assert.strictEqual(module.exports.ping(), 'pong');
 
-// Check that after the addon is loaded with
-// process.dlopen() a require() call fails.
-const re = /^Error: Module did not self-register\.$/;
-assert.throws(() => require(`./build/${common.buildType}/binding`), re);
+// This second `require()` call should not throw an error.
+require(`./build/${common.buildType}/binding`);

test/addons/hello-world/test-worker.js

@@ -0,0 +1,14 @@
+// Flags: --experimental-worker
+'use strict';
+const common = require('../../common');
+const assert = require('assert');
+const path = require('path');
+const { Worker } = require('worker_threads');
+const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`);
+
+const w = new Worker(`
+require('worker_threads').parentPort.postMessage(
+  require(${JSON.stringify(binding)}).hello());`, { eval: true });
+w.on('message', common.mustCall((message) => {
+  assert.strictEqual(message, 'world');
+}));