From 62497a56ec259817bf82a6225270d319827be506 Mon Sep 17 00:00:00 2001 From: Masud Rahman Date: Sat, 31 Mar 2018 11:36:14 -0400 Subject: [PATCH] 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 | 46 ++++++++++++------- .../addons-napi/9_multiply_required/binding.c | 19 ++++++++ .../9_multiply_required/binding.gyp | 8 ++++ test/addons-napi/9_multiply_required/test.js | 8 ++++ test/addons/dlopen-ping-pong/test.js | 5 -- .../addons/multiply-required-addon/binding.cc | 15 ++++++ .../multiply-required-addon/binding.gyp | 9 ++++ test/addons/multiply-required-addon/test.js | 8 ++++ test/addons/not-a-binding/test.js | 2 +- 9 files changed, 98 insertions(+), 22 deletions(-) create mode 100644 test/addons-napi/9_multiply_required/binding.c create mode 100644 test/addons-napi/9_multiply_required/binding.gyp create mode 100644 test/addons-napi/9_multiply_required/test.js create mode 100644 test/addons/multiply-required-addon/binding.cc create mode 100644 test/addons/multiply-required-addon/binding.gyp create mode 100644 test/addons/multiply-required-addon/test.js diff --git a/src/node.cc b/src/node.cc index b5084331156bb6..4cf7337f5580fe 100644 --- a/src/node.cc +++ b/src/node.cc @@ -82,6 +82,7 @@ #include #include +#include #include #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 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& 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& 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 errmsg = OneByteString(env->isolate(), dlib.errmsg_.c_str()); dlib.Close(); @@ -2278,12 +2267,37 @@ static void DLOpen(const FunctionCallbackInfo& 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; } diff --git a/test/addons-napi/9_multiply_required/binding.c b/test/addons-napi/9_multiply_required/binding.c new file mode 100644 index 00000000000000..27d49079966d09 --- /dev/null +++ b/test/addons-napi/9_multiply_required/binding.c @@ -0,0 +1,19 @@ +#include +#include "../common.h" +#include + +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) diff --git a/test/addons-napi/9_multiply_required/binding.gyp b/test/addons-napi/9_multiply_required/binding.gyp new file mode 100644 index 00000000000000..62381d5e54f22b --- /dev/null +++ b/test/addons-napi/9_multiply_required/binding.gyp @@ -0,0 +1,8 @@ +{ + "targets": [ + { + "target_name": "binding", + "sources": [ "binding.c" ] + } + ] +} diff --git a/test/addons-napi/9_multiply_required/test.js b/test/addons-napi/9_multiply_required/test.js new file mode 100644 index 00000000000000..2c50b73c7a639b --- /dev/null +++ b/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); diff --git a/test/addons/dlopen-ping-pong/test.js b/test/addons/dlopen-ping-pong/test.js index c533593496090a..80f058eb4a2c6b 100644 --- a/test/addons/dlopen-ping-pong/test.js +++ b/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); diff --git a/test/addons/multiply-required-addon/binding.cc b/test/addons/multiply-required-addon/binding.cc new file mode 100644 index 00000000000000..008526b0375b11 --- /dev/null +++ b/test/addons/multiply-required-addon/binding.cc @@ -0,0 +1,15 @@ +#include +#include + +void Method(const v8::FunctionCallbackInfo& args) { + v8::Isolate* isolate = args.GetIsolate(); + args.GetReturnValue().Set(v8::String::NewFromUtf8(isolate, "world")); +} + +void define(v8::Local exports, + v8::Local module, + v8::Local context) { + NODE_SET_METHOD(exports, "hello", Method); +} + +NODE_MODULE(binding, &define); diff --git a/test/addons/multiply-required-addon/binding.gyp b/test/addons/multiply-required-addon/binding.gyp new file mode 100644 index 00000000000000..7ede63d94a0d77 --- /dev/null +++ b/test/addons/multiply-required-addon/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'defines': [ 'V8_DEPRECATION_WARNINGS=1' ], + 'sources': [ 'binding.cc' ] + } + ] +} diff --git a/test/addons/multiply-required-addon/test.js b/test/addons/multiply-required-addon/test.js new file mode 100644 index 00000000000000..2c50b73c7a639b --- /dev/null +++ b/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); diff --git a/test/addons/not-a-binding/test.js b/test/addons/not-a-binding/test.js index a0ce2d0629ac1d..b77d4c4160784f 100644 --- a/test/addons/not-a-binding/test.js +++ b/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);