addons: allow multiple requires of the same addon

Replace the global state tracking a single pending addon that is to be
loaded with a map from addon name to 'node_module*'.  This allows
multiple, independent 'node::Environment's to load the same addon.

Add a test to validate this new behavior by clearing the require cache
and reloading the addon.

NOTE: This will break any user that is not following the documentation
and requiring an addon via a different name than that passed as the
first argument to `NODE_MODULE(...)`.

src/node.cc

@@ -82,6 +82,7 @@
 #include <sys/types.h>
 
 #include <string>
+#include <unordered_map>
 #include <vector>
 
 #if defined(NODE_HAVE_I18N_SUPPORT)
@@ -186,7 +187,7 @@ static int v8_thread_pool_size = v8_default_thread_pool_size;
 static bool prof_process = false;
 static bool v8_is_profiling = false;
 static bool node_is_initialized = false;
-static node_module* modpending;
+static std::unordered_map<std::string, node_module*> mods;
 static node_module* modlist_builtin;
 static node_module* modlist_internal;
 static node_module* modlist_linked;
@@ -2118,8 +2119,8 @@ extern "C" void node_module_register(void* m) {
     mp->nm_flags = NM_F_LINKED;
     mp->nm_link = modlist_linked;
     modlist_linked = mp;
-  } else {
-    modpending = mp;
+  } else if (mods.count(mp->nm_modname) == 0) {
+    mods[mp->nm_modname] = mp;
   }
 }
 
@@ -2227,16 +2228,10 @@ inline InitializerCallback GetInitializerCallback(DLib* dlib) {
 
 // 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();
 
-  CHECK_EQ(modpending, nullptr);
-
   if (args.Length() < 2) {
     env->ThrowError("process.dlopen needs at least 2 arguments.");
     return;
@@ -2260,12 +2255,6 @@ static void DLOpen(const FunctionCallbackInfo<Value>& args) {
   DLib 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 = modpending;
-  modpending = nullptr;
-
   if (!is_opened) {
     Local<String> errmsg = OneByteString(env->isolate(), dlib.errmsg_.c_str());
     dlib.Close();
@@ -2278,12 +2267,37 @@ static void DLOpen(const FunctionCallbackInfo<Value>& args) {
     return;
   }
 
+  size_t lastdot = dlib.filename_.find_last_of('.');
+  CHECK_NE(lastdot, std::string::npos);
+  CHECK_EQ(lastdot, dlib.filename_.find(".node", lastdot));
+
+#ifdef _WIN32
+  size_t lastslash = dlib.filename_.find_last_of('\\');
+#else
+  size_t lastslash = dlib.filename_.find_last_of('/');
+#endif
+  size_t basenameloc = 0;
+  if (lastslash == std::string::npos) {
+    // No separators in the path
+    basenameloc = 0;
+  } else {
+    // At least one separator in the path
+    basenameloc = lastslash + 1;
+  }
+
+  std::string name = dlib.filename_.substr(basenameloc, lastdot - basenameloc);
+  node_module* mp = nullptr;
+  if (mods.find(name) != mods.end()) {
+    mp = mods[name];
+  }
+
   if (mp == nullptr) {
     if (auto callback = GetInitializerCallback(&dlib)) {
       callback(exports, module, context);
     } else {
       dlib.Close();
-      env->ThrowError("Module did not self-register.");
+      env->ThrowError("Module did not self-register or module required and "
+                      "registered with different names.");
     }
     return;
   }

test/addons-napi/9_multiply_required/binding.c

@@ -0,0 +1,19 @@
+#include <node_api.h>
+#include "../common.h"
+#include <string.h>
+
+napi_value Method(napi_env env, napi_callback_info info) {
+  napi_value world;
+  const char* str = "world";
+  size_t str_len = strlen(str);
+  NAPI_CALL(env, napi_create_string_utf8(env, str, str_len, &world));
+  return world;
+}
+
+napi_value Init(napi_env env, napi_value exports) {
+  napi_property_descriptor desc = DECLARE_NAPI_PROPERTY("hello", Method);
+  NAPI_CALL(env, napi_define_properties(env, exports, 1, &desc));
+  return exports;
+}
+
+NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)

test/addons-napi/9_multiply_required/binding.gyp

@@ -0,0 +1,8 @@
+{
+  "targets": [
+    {
+      "target_name": "binding",
+      "sources": [ "binding.c" ]
+    }
+  ]
+}

test/addons-napi/9_multiply_required/test.js

@@ -0,0 +1,8 @@
+'use strict';
+const common = require('../../common');
+
+const bindingPath = require.resolve(`./build/${common.buildType}/binding`);
+
+require(bindingPath);
+delete require.cache[bindingPath];
+require(bindingPath);

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

@@ -13,8 +13,3 @@ process.dlopen(module, bindingPath,
                os.constants.dlopen.RTLD_NOW | os.constants.dlopen.RTLD_GLOBAL);
 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);

test/addons/multiply-required-addon/binding.cc

@@ -0,0 +1,15 @@
+#include <node.h>
+#include <v8.h>
+
+void Method(const v8::FunctionCallbackInfo<v8::Value>& args) {
+  v8::Isolate* isolate = args.GetIsolate();
+  args.GetReturnValue().Set(v8::String::NewFromUtf8(isolate, "world"));
+}
+
+void define(v8::Local<v8::Object> exports,
+            v8::Local<v8::Value> module,
+            v8::Local<v8::Context> context) {
+  NODE_SET_METHOD(exports, "hello", Method);
+}
+
+NODE_MODULE(binding, &define);

test/addons/multiply-required-addon/binding.gyp

@@ -0,0 +1,9 @@
+{
+  'targets': [
+    {
+      'target_name': 'binding',
+      'defines': [ 'V8_DEPRECATION_WARNINGS=1' ],
+      'sources': [ 'binding.cc' ]
+    }
+  ]
+}

test/addons/multiply-required-addon/test.js

@@ -0,0 +1,8 @@
+'use strict';
+const common = require('../../common');
+
+const bindingPath = require.resolve(`./build/${common.buildType}/binding`);
+
+require(bindingPath);
+delete require.cache[bindingPath];
+require(bindingPath);

test/addons/not-a-binding/test.js

@@ -2,5 +2,5 @@
 const common = require('../../common');
 const assert = require('assert');
 
-const re = /^Error: Module did not self-register\.$/;
+const re = /^Error: Module did not self-register or module required and registered with different names\.$/;
 assert.throws(() => require(`./build/${common.buildType}/binding`), re);