From efc57aa2d66ffd1d7ecf995be17b4824d7ed1bc4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 10 Sep 2017 00:22:30 +0200 Subject: [PATCH 1/3] worker: improve integration with native addons Native addons are now unloaded if all Environments referring to it have been cleaned up, except if it also loaded by the main Environment. Thanks go to Alexey Orlenko, Olivia Hugger and Stephen Belanger for reviewing the original version of this change. Refs: https://github.com/ayojs/ayo/pull/118 --- src/node.cc | 240 +++++++++++++++++-------- test/addons/dlopen-ping-pong/test.js | 6 +- test/addons/hello-world/test-worker.js | 14 ++ test/addons/worker-addon/binding.cc | 45 +++++ test/addons/worker-addon/binding.gyp | 9 + test/addons/worker-addon/test.js | 17 ++ 6 files changed, 255 insertions(+), 76 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 6250acfd5d540f..6cb4dc6d11b8b8 100644 --- a/src/node.cc +++ b/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> dlopen_cache; +std::unordered_map> + handle_to_dlib; + +class DLib : public std::enable_shared_from_this { 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_; + std::string filename_; + int flags_; std::string errmsg_; - void* handle_; + void* handle_ = nullptr; + std::unordered_set 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 info; + Environment* env; + }; + env->AddCleanupHook([](void* arg) { + Mutex::ScopedLock lock(dlib_mutex); + std::unique_ptr cbdata( + static_cast(arg)); + std::shared_ptr 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 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); - auto context = env->context(); - uv_once(&init_modpending_once, InitModpendingOnce); - CHECK_NULL(uv_key_get(&thread_local_modpending)); + + Local context = env->context(); + node_module* mp; + std::shared_ptr dlib; if (args.Length() < 2) { env->ThrowError("process.dlopen needs at least 2 arguments."); @@ -1257,83 +1308,128 @@ static void DLOpen(const FunctionCallbackInfo& 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_ = *filename; + dlib->flags_ = 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( - 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 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(uv_key_get(&thread_local_modpending)); + uv_key_set(&thread_local_modpending, nullptr); + + 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( - 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; } diff --git a/test/addons/dlopen-ping-pong/test.js b/test/addons/dlopen-ping-pong/test.js index c533593496090a..2b5c65dae9f8f6 100644 --- a/test/addons/dlopen-ping-pong/test.js +++ b/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`); diff --git a/test/addons/hello-world/test-worker.js b/test/addons/hello-world/test-worker.js new file mode 100644 index 00000000000000..f989c738c873c7 --- /dev/null +++ b/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'); +})); diff --git a/test/addons/worker-addon/binding.cc b/test/addons/worker-addon/binding.cc new file mode 100644 index 00000000000000..cb87a43757dfc5 --- /dev/null +++ b/test/addons/worker-addon/binding.cc @@ -0,0 +1,45 @@ +#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 Initialize(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(NODE_GYP_MODULE_NAME, Initialize) diff --git a/test/addons/worker-addon/binding.gyp b/test/addons/worker-addon/binding.gyp new file mode 100644 index 00000000000000..7ede63d94a0d77 --- /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 00000000000000..bdaac8659f2459 --- /dev/null +++ b/test/addons/worker-addon/test.js @@ -0,0 +1,17 @@ +// Flags: --experimental-worker +'use strict'; +const common = require('../../common'); +const assert = require('assert'); +const child_process = require('child_process'); +const path = require('path'); +const { Worker } = require('worker_threads'); +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); +} From f93bca3707c3fa925ba1ae180798ed9d7b85b65a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 7 Oct 2018 16:08:38 -0700 Subject: [PATCH 2/3] fixup! worker: improve integration with native addons --- src/node.cc | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/node.cc b/src/node.cc index 6cb4dc6d11b8b8..94fe8c10286d4c 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1166,8 +1166,8 @@ class DLib : public std::enable_shared_from_this { inline void* GetSymbolAddress(const char* name); inline void AddEnvironment(Environment* env); - std::string filename_; - int flags_; + const std::string filename_; + const int flags_ = 0; std::string errmsg_; void* handle_ = nullptr; std::unordered_set users_; @@ -1334,9 +1334,7 @@ static void DLOpen(const FunctionCallbackInfo& args) { break; } - dlib = std::make_shared(); - dlib->filename_ = *filename; - dlib->flags_ = flags; + dlib = std::make_shared(*filename, flags); bool is_opened = dlib->Open(); if (is_opened && handle_to_dlib.count(dlib->handle_) > 0) { From b7685b69c2a4157384167169cb3501bef1b7256d Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 7 Oct 2018 16:51:12 -0700 Subject: [PATCH 3/3] squash! worker: improve integration with native addons --- doc/api/addons.md | 18 ++++++++++++++++++ doc/api/worker_threads.md | 10 ++++------ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/doc/api/addons.md b/doc/api/addons.md index b2c52d5128b5bc..334c0642b05cb1 100644 --- a/doc/api/addons.md +++ b/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 diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md index 37751c21bbb44d..4beae369ebaf8b 100644 --- a/doc/api/worker_threads.md +++ b/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