From 66f85d916795c985b6f0cfc5c1cc0784ef2f348d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 10 Sep 2017 00:22:30 +0200 Subject: [PATCH] addons: integrate workers with native addons Native addons need to use flags to indicate that they are capable of being loaded by worker threads. Native addons are unloaded if all Environments referring to it have been cleaned up, except if it also loaded by the main Environment. --- src/node.cc | 226 +++++++++++++++++-------- src/node.h | 5 + src/node_api.cc | 2 + src/node_api.h | 5 + test/addons/dlopen-ping-pong/test.js | 5 +- test/addons/hello-world/test-worker.js | 15 ++ test/addons/worker-addon/binding.cc | 43 +++++ test/addons/worker-addon/binding.gyp | 9 + test/addons/worker-addon/test.js | 16 ++ 9 files changed, 252 insertions(+), 74 deletions(-) create mode 100644 test/addons/hello-world/test-worker.js create mode 100644 test/addons/worker-addon/binding.cc create mode 100644 test/addons/worker-addon/binding.gyp create mode 100644 test/addons/worker-addon/test.js diff --git a/src/node.cc b/src/node.cc index 5ab020b213..d9dfbee0e6 100644 --- a/src/node.cc +++ b/src/node.cc @@ -2668,11 +2668,28 @@ node_module* get_linked_module(const char* name) { return FindModule(modlist_linked, name, NM_F_LINKED); } -struct DLib { +namespace { + +Mutex dlib_mutex; + +struct DLib; + +std::unordered_map> dlopen_cache; +std::unordered_map> + handle_to_dlib; + +struct DLib : public std::enable_shared_from_this { std::string filename_; std::string errmsg_; - void* handle_; + void* handle_ = nullptr; int flags_; + std::unordered_set users_; + node_module* own_info = nullptr; + + DLib() {} + ~DLib() { + Close(); + } #ifdef __POSIX__ static const int kDefaultFlags = RTLD_LAZY; @@ -2708,90 +2725,160 @@ struct DLib { uv_dlclose(&lib_); } #endif // !__POSIX__ + + DLib(const DLib& other) = delete; + DLib(DLib&& other) = delete; + DLib& operator=(const DLib& other) = delete; + DLib& operator=(DLib&& other) = delete; + + void 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 info; + Environment* env; + }; + env->AddCleanupHook([](void* arg) { + Mutex::ScopedLock lock(dlib_mutex); + cleanup_hook_data* cbdata = static_cast(arg); + std::shared_ptr info = cbdata->info; + info->users_.erase(cbdata->env); + delete cbdata; + if (info->users_.empty()) { + std::vector 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(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& args) { Environment* env = Environment::GetCurrent(args); + node_module* mp; + std::shared_ptr dlib; + Local module = args[0]->ToObject(env->isolate()); - CHECK_EQ(modpending, nullptr); + do { + Mutex::ScopedLock lock(dlib_mutex); + CHECK_EQ(modpending, nullptr); - if (args.Length() < 2) { - env->ThrowError("process.dlopen needs at least 2 arguments."); - return; - } + 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(env->context()).To(&flags)) { - return env->ThrowTypeError("flag argument must be an integer."); - } + int32_t flags = DLib::kDefaultFlags; + if (args.Length() > 2 && !args[2]->Int32Value(env->context()).To(&flags)) { + return env->ThrowTypeError("flag argument must be an integer."); + } - Local module = args[0]->ToObject(env->isolate()); // Cast - node::Utf8Value filename(env->isolate(), args[1]); // Cast - DLib dlib; - dlib.filename_ = *filename; - dlib.flags_ = flags; - bool is_opened = dlib.Open(); + 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_ = *filename; + dlib->flags_ = flags; + bool is_opened = dlib->Open(); + + if (is_opened && handle_to_dlib.count(dlib->handle_) > 0) { + dlib = handle_to_dlib[dlib->handle_]; + mp = dlib->own_info; + dlib->AddEnvironment(env); + break; + } - // 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; + // 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 = modpending; + modpending = nullptr; - if (!is_opened) { - Local errmsg = OneByteString(env->isolate(), dlib.errmsg_.c_str()); - dlib.Close(); + if (!is_opened) { + Local errmsg = + OneByteString(env->isolate(), dlib->errmsg_.c_str()); #ifdef _WIN32 - // Windows needs to add the filename into the error message - errmsg = String::Concat(errmsg, args[1]->ToString(env->isolate())); + // Windows needs to add the filename into the error message + errmsg = String::Concat(errmsg, args[1]->ToString(env->isolate())); #endif // _WIN32 - env->isolate()->ThrowException(Exception::Error(errmsg)); - return; - } + env->isolate()->ThrowException(Exception::Error(errmsg)); + return; + } - if (mp == nullptr) { - dlib.Close(); - env->ThrowError("Module did not self-register."); - return; - } - if (mp->nm_version == -1) { - if (env->EmitNapiWarning()) { - ProcessEmitWarning(env, "N-API is an experimental feature and could " - "change at any time."); + if (mp == nullptr) { + env->ThrowError("Module did not self-register."); + return; } - } else if (mp->nm_version != NODE_MODULE_VERSION) { - 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; - } + if (mp->nm_version == -1) { + if (env->EmitNapiWarning()) { + ProcessEmitWarning(env, "N-API is an experimental feature and could " + "change at any time."); + } + } else if (mp->nm_version != NODE_MODULE_VERSION) { + 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; + } + + if (mp->nm_flags & NM_F_BUILTIN) { + env->ThrowError("Built-in module self-registered."); + return; + } + + if (!env->is_main_thread() && !(mp->nm_flags & NM_F_WORKER_ENABLED)) { + env->ThrowError("Native modules need to explicitly indicate multi-" + "isolate/multi-thread support by using " + "`NODE_MODULE_WORKER_ENABLED` or " + "`NAPI_MODULE_WORKER_ENABLED` to register themselves."); + 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; + mp->nm_dso_handle = dlib->handle_; + mp->nm_link = modlist_addon; + modlist_addon = mp; + } while (false); Local exports_string = env->exports_string(); Local exports = module->Get(exports_string)->ToObject(env->isolate()); @@ -2801,7 +2888,6 @@ static void DLOpen(const FunctionCallbackInfo& args) { } 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; } diff --git a/src/node.h b/src/node.h index 511dcb6242..9202939ba1 100644 --- a/src/node.h +++ b/src/node.h @@ -445,6 +445,7 @@ typedef void (*addon_context_register_func)( #define NM_F_BUILTIN 0x01 #define NM_F_LINKED 0x02 #define NM_F_INTERNAL 0x04 +#define NM_F_WORKER_ENABLED 0x400 struct node_module { int nm_version; @@ -532,6 +533,10 @@ extern "C" NODE_EXTERN void node_module_register(void* mod); #define NODE_MODULE_CONTEXT_AWARE_BUILTIN(modname, regfunc) \ NODE_MODULE_CONTEXT_AWARE_X(modname, regfunc, NULL, NM_F_BUILTIN) \ +#define NODE_MODULE_WORKER_ENABLED(modname, regfunc) \ + NODE_MODULE_CONTEXT_AWARE_X(modname, regfunc, NULL, \ + NM_F_WORKER_ENABLED) + /* * For backward compatibility in add-on modules. */ diff --git a/src/node_api.cc b/src/node_api.cc index 59ca97bbb4..bc82894226 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -856,6 +856,8 @@ void napi_module_register_cb(v8::Local exports, // Registers a NAPI module. void napi_module_register(napi_module* mod) { + static_assert(NAPI_F_WORKER_ENABLED == NM_F_WORKER_ENABLED, + "Worker-enabled flags match for N-API and Node"); int module_version = -1; #ifdef EXTERNAL_NAPI module_version = NODE_MODULE_VERSION; diff --git a/src/node_api.h b/src/node_api.h index 29070c3ec8..8af363ef9c 100644 --- a/src/node_api.h +++ b/src/node_api.h @@ -80,6 +80,8 @@ typedef struct { #define EXTERN_C_END #endif +#define NAPI_F_WORKER_ENABLED 0x400 + #define NAPI_MODULE_X(modname, regfunc, priv, flags) \ EXTERN_C_START \ static napi_module _module = \ @@ -100,6 +102,9 @@ typedef struct { #define NAPI_MODULE(modname, regfunc) \ NAPI_MODULE_X(modname, regfunc, NULL, 0) +#define NAPI_MODULE_WORKER_ENABLED(modname, regfunc) \ + NAPI_MODULE_X(modname, regfunc, NULL, NAPI_F_WORKER_ENABLED) + #define NAPI_AUTO_LENGTH SIZE_MAX EXTERN_C_START diff --git a/test/addons/dlopen-ping-pong/test.js b/test/addons/dlopen-ping-pong/test.js index c533593496..6d4a648951 100644 --- a/test/addons/dlopen-ping-pong/test.js +++ b/test/addons/dlopen-ping-pong/test.js @@ -14,7 +14,4 @@ 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); +assert.doesNotThrow(() => require(`./build/${common.buildType}/binding`)); diff --git a/test/addons/hello-world/test-worker.js b/test/addons/hello-world/test-worker.js new file mode 100644 index 0000000000..6794fda7b6 --- /dev/null +++ b/test/addons/hello-world/test-worker.js @@ -0,0 +1,15 @@ +'use strict'; +const common = require('../../common'); +const assert = require('assert'); +const path = require('path'); +const { Worker } = require('worker'); +const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`); + +const w = new Worker(`require(${JSON.stringify(binding)});`, { eval: true }); +w.on('error', common.mustCall((err) => { + assert(String(err).includes( + 'Error: Native modules need to explicitly indicate ' + + 'multi-isolate/multi-thread support by using ' + + '`NODE_MODULE_WORKER_ENABLED` or `NAPI_MODULE_WORKER_ENABLED` to ' + + 'register themselves.')); +})); diff --git a/test/addons/worker-addon/binding.cc b/test/addons/worker-addon/binding.cc new file mode 100644 index 0000000000..3d3c367d7d --- /dev/null +++ b/test/addons/worker-addon/binding.cc @@ -0,0 +1,43 @@ +#include +#include +#include +#include +#include + +using v8::Context; +using v8::HandleScope; +using v8::Isolate; +using v8::Local; +using v8::Object; +using v8::Value; + +size_t count = 0; + +struct statically_allocated { + statically_allocated() { + assert(count == 0); + printf("ctor "); + } + ~statically_allocated() { + assert(count == 0); + printf("dtor"); + } +} var; + +void Dummy(void*) { + assert(0); +} + +void Cleanup(void* str) { + printf("%s ", static_cast(str)); +} + +void Init(Local exports, Local module, Local context) { + node::AddEnvironmentCleanupHook( + context->GetIsolate(), Cleanup, + const_cast(static_cast("cleanup"))); + node::AddEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr); + node::RemoveEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr); +} + +NODE_MODULE_WORKER_ENABLED(binding, Init) diff --git a/test/addons/worker-addon/binding.gyp b/test/addons/worker-addon/binding.gyp new file mode 100644 index 0000000000..7ede63d94a --- /dev/null +++ b/test/addons/worker-addon/binding.gyp @@ -0,0 +1,9 @@ +{ + 'targets': [ + { + 'target_name': 'binding', + 'defines': [ 'V8_DEPRECATION_WARNINGS=1' ], + 'sources': [ 'binding.cc' ] + } + ] +} diff --git a/test/addons/worker-addon/test.js b/test/addons/worker-addon/test.js new file mode 100644 index 0000000000..b3eb35824a --- /dev/null +++ b/test/addons/worker-addon/test.js @@ -0,0 +1,16 @@ +'use strict'; +const common = require('../../common'); +const assert = require('assert'); +const child_process = require('child_process'); +const path = require('path'); +const { Worker } = require('worker'); +const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`); + +if (process.argv[2] === 'child') { + new Worker(`require(${JSON.stringify(binding)});`, { eval: true }); +} else { + const proc = child_process.spawnSync(process.execPath, [__filename, 'child']); + assert.strictEqual(proc.stderr.toString(), ''); + assert.strictEqual(proc.stdout.toString(), 'ctor cleanup dtor'); + assert.strictEqual(proc.status, 0); +}