From 8ebd339031cf9826629ad780dd35fee130e95985 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 17 Feb 2019 23:45:14 +0100 Subject: [PATCH] worker: improve integration with native addons Allow loading add-ons from multiple Node.js instances if they are declared context-aware; in particular, this applies to N-API addons. Also, plug a memory leak that occurred when registering N-API addons. Refs: https://github.com/nodejs/node/pull/23319 PR-URL: https://github.com/nodejs/node/pull/26175 Fixes: https://github.com/nodejs/node/issues/21481 Fixes: https://github.com/nodejs/node/issues/21783 Fixes: https://github.com/nodejs/node/issues/25662 Fixes: https://github.com/nodejs/node/issues/20239 Reviewed-By: James M Snell Reviewed-By: Gireesh Punathil Reviewed-By: Richard Lau --- doc/api/addons.md | 5 + doc/api/worker_threads.md | 9 +- src/node_api.cc | 2 +- src/node_binding.cc | 233 +++++++++++++++++- src/node_binding.h | 4 + test/addons/dlopen-ping-pong/test-worker.js | 20 ++ test/addons/worker-addon/binding.cc | 2 +- test/addons/worker-addon/test.js | 60 ++++- test/node-api/1_hello_world/test.js | 9 + test/node-api/test_worker_terminate/test.js | 5 + .../test_worker_terminate.c | 2 + 11 files changed, 322 insertions(+), 29 deletions(-) create mode 100644 test/addons/dlopen-ping-pong/test-worker.js diff --git a/doc/api/addons.md b/doc/api/addons.md index 4941d3de9add8f..1c811b50c21a47 100644 --- a/doc/api/addons.md +++ b/doc/api/addons.md @@ -251,6 +251,11 @@ down. If necessary, such hooks can be removed using `RemoveEnvironmentCleanupHook()` before they are run, which has the same signature. +In order to be loaded from multiple Node.js environments, +such as a main thread and a Worker thread, an add-on needs to either: +- Be an N-API addon, or +- Be declared as context-aware using `NODE_MODULE_INIT()` as described above + ### Building Once the source code has been written, it must be compiled into the binary diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md index 95cdae5f9a81af..54bd66a6ea683e 100644 --- a/doc/api/worker_threads.md +++ b/doc/api/worker_threads.md @@ -360,11 +360,8 @@ Notable differences inside a Worker environment are: being invoked. - IPC channels from parent processes are not accessible. - The [`trace_events`][] module is not supported. - -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 can only be loaded from multiple threads if they fulfill + [certain conditions][Addons worker support]. Creating `Worker` instances inside of other `Worker`s is possible. @@ -580,7 +577,6 @@ active handle in the event system. If the worker is already `unref()`ed calling [`WebAssembly.Module`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WebAssembly/Module [`Worker`]: #worker_threads_class_worker [`cluster` module]: cluster.html -[`inspector`]: inspector.html [`port.on('message')`]: #worker_threads_event_message [`port.postMessage()`]: #worker_threads_port_postmessage_value_transferlist [`process.abort()`]: process.html#process_process_abort @@ -602,6 +598,7 @@ active handle in the event system. If the worker is already `unref()`ed calling [`worker.postMessage()`]: #worker_threads_worker_postmessage_value_transferlist [`worker.terminate()`]: #worker_threads_worker_terminate_callback [`worker.threadId`]: #worker_threads_worker_threadid_1 +[Addons worker support]: addons.html#addons_worker_support [HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm [Signals events]: process.html#process_signal_events [Web Workers]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API diff --git a/src/node_api.cc b/src/node_api.cc index 4e1a779c42e6f6..bfc0c12aad2e77 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -484,7 +484,7 @@ void napi_module_register_by_symbol(v8::Local exports, void napi_module_register(napi_module* mod) { node::node_module* nm = new node::node_module { -1, - mod->nm_flags, + mod->nm_flags | NM_F_DELETEME, nullptr, mod->nm_filename, nullptr, diff --git a/src/node_binding.cc b/src/node_binding.cc index c9ff7be46f80fc..7b87ba6bffd2e7 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -2,6 +2,7 @@ #include "env-inl.h" #include "node_native_module.h" #include "util.h" +#include #if HAVE_OPENSSL #define NODE_BUILTIN_OPENSSL_MODULES(V) V(crypto) V(tls_wrap) @@ -96,6 +97,130 @@ NODE_BUILTIN_MODULES(V) #undef V +#ifdef _AIX +// On AIX, dlopen() behaves differently from other operating systems, in that +// it returns unique values from each call, rather than identical values, when +// loading the same handle. +// We try to work around that by providing wrappers for the dlopen() family of +// functions, and using st_dev and st_ino for the file that is to be loaded +// as keys for a cache. + +namespace node { +namespace dlwrapper { + +struct dl_wrap { + uint64_t st_dev; + uint64_t st_ino; + uint64_t refcount; + void* real_handle; + + struct hash { + size_t operator()(const dl_wrap* wrap) const { + return std::hash()(wrap->st_dev) ^ + std::hash()(wrap->st_ino); + } + }; + + struct equal { + bool operator()(const dl_wrap* a, + const dl_wrap* b) const { + return a->st_dev == b->st_dev && a->st_ino == b->st_ino; + } + }; +}; + +static Mutex dlhandles_mutex; +static std::unordered_set + dlhandles; +static thread_local std::string dlerror_storage; + +char* wrapped_dlerror() { + return &dlerror_storage[0]; +} + +void* wrapped_dlopen(const char* filename, int flags) { + CHECK_NOT_NULL(filename); // This deviates from the 'real' dlopen(). + Mutex::ScopedLock lock(dlhandles_mutex); + + uv_fs_t req; + OnScopeLeave cleanup([&]() { uv_fs_req_cleanup(&req); }); + int rc = uv_fs_stat(nullptr, &req, filename, nullptr); + + if (rc != 0) { + dlerror_storage = uv_strerror(rc); + return nullptr; + } + + dl_wrap search = { + req.statbuf.st_dev, + req.statbuf.st_ino, + 0, nullptr + }; + + auto it = dlhandles.find(&search); + if (it != dlhandles.end()) { + (*it)->refcount++; + return *it; + } + + void* real_handle = dlopen(filename, flags); + if (real_handle == nullptr) { + dlerror_storage = dlerror(); + return nullptr; + } + dl_wrap* wrap = new dl_wrap(); + wrap->st_dev = req.statbuf.st_dev; + wrap->st_ino = req.statbuf.st_ino; + wrap->refcount = 1; + wrap->real_handle = real_handle; + dlhandles.insert(wrap); + return wrap; +} + +int wrapped_dlclose(void* handle) { + Mutex::ScopedLock lock(dlhandles_mutex); + dl_wrap* wrap = static_cast(handle); + int ret = 0; + CHECK_GE(wrap->refcount, 1); + if (--wrap->refcount == 0) { + ret = dlclose(wrap->real_handle); + if (ret != 0) dlerror_storage = dlerror(); + dlhandles.erase(wrap); + delete wrap; + } + return ret; +} + +void* wrapped_dlsym(void* handle, const char* symbol) { + if (handle == RTLD_DEFAULT || handle == RTLD_NEXT) + return dlsym(handle, symbol); + dl_wrap* wrap = static_cast(handle); + return dlsym(wrap->real_handle, symbol); +} + +#define dlopen node::dlwrapper::wrapped_dlopen +#define dlerror node::dlwrapper::wrapped_dlerror +#define dlclose node::dlwrapper::wrapped_dlclose +#define dlsym node::dlwrapper::wrapped_dlsym + +} // namespace dlwrapper +} // namespace node + +#endif // _AIX + +#ifdef __linux__ +static bool libc_may_be_musl() { + static std::atomic_bool retval; // Cache the return value. + static std::atomic_bool has_cached_retval { false }; + if (has_cached_retval) return retval; + retval = dlsym(RTLD_DEFAULT, "gnu_get_libc_version") == nullptr; + has_cached_retval = true; + return retval; +} +#else // __linux__ +static bool libc_may_be_musl() { return false; } +#endif // __linux__ + namespace node { using v8::Context; @@ -110,7 +235,6 @@ using v8::Value; // Globals per process static node_module* modlist_internal; static node_module* modlist_linked; -static node_module* modlist_addon; static uv_once_t init_modpending_once = UV_ONCE_INIT; static uv_key_t thread_local_modpending; @@ -136,6 +260,57 @@ extern "C" void node_module_register(void* m) { namespace binding { +static struct global_handle_map_t { + public: + void set(void* handle, node_module* mod) { + CHECK_NE(handle, nullptr); + Mutex::ScopedLock lock(mutex_); + + map_[handle].module = mod; + // We need to store this flag internally to avoid a chicken-and-egg problem + // during cleanup. By the time we actually use the flag's value, + // the shared object has been unloaded, and its memory would be gone, + // making it impossible to access fields of `mod` -- + // unless `mod` *is* dynamically allocated, but we cannot know that + // without checking the flag. + map_[handle].wants_delete_module = mod->nm_flags & NM_F_DELETEME; + map_[handle].refcount++; + } + + node_module* get_and_increase_refcount(void* handle) { + CHECK_NE(handle, nullptr); + Mutex::ScopedLock lock(mutex_); + + auto it = map_.find(handle); + if (it == map_.end()) return nullptr; + it->second.refcount++; + return it->second.module; + } + + void erase(void* handle) { + CHECK_NE(handle, nullptr); + Mutex::ScopedLock lock(mutex_); + + auto it = map_.find(handle); + if (it == map_.end()) return; + CHECK_GE(it->second.refcount, 1); + if (--it->second.refcount == 0) { + if (it->second.wants_delete_module) + delete it->second.module; + map_.erase(handle); + } + } + + private: + Mutex mutex_; + struct Entry { + unsigned int refcount; + bool wants_delete_module; + node_module* module; + }; + std::unordered_map map_; +} global_handle_map; + DLib::DLib(const char* filename, int flags) : filename_(filename), flags_(flags), handle_(nullptr) {} @@ -149,7 +324,21 @@ bool DLib::Open() { void DLib::Close() { if (handle_ == nullptr) return; - dlclose(handle_); + + if (libc_may_be_musl()) { + // musl libc implements dlclose() as a no-op which returns 0. + // As a consequence, trying to re-load a previously closed addon at a later + // point will not call its static constructors, which Node.js uses. + // Therefore, when we may be using musl libc, we assume that the shared + // object exists indefinitely and keep it in our handle map. + return; + } + + int err = dlclose(handle_); + if (err == 0) { + if (has_entry_in_global_handle_map_) + global_handle_map.erase(handle_); + } handle_ = nullptr; } @@ -170,6 +359,8 @@ bool DLib::Open() { void DLib::Close() { if (handle_ == nullptr) return; + if (has_entry_in_global_handle_map_) + global_handle_map.erase(handle_); uv_dlclose(&lib_); handle_ = nullptr; } @@ -181,6 +372,16 @@ void* DLib::GetSymbolAddress(const char* name) { } #endif // !__POSIX__ +void DLib::SaveInGlobalHandleMap(node_module* mp) { + has_entry_in_global_handle_map_ = true; + global_handle_map.set(handle_, mp); +} + +node_module* DLib::GetSavedModuleFromGlobalHandleMap() { + has_entry_in_global_handle_map_ = true; + return global_handle_map.get_and_increase_refcount(handle_); +} + using InitializerCallback = void (*)(Local exports, Local module, Local context); @@ -235,12 +436,15 @@ void DLOpen(const FunctionCallbackInfo& args) { node::Utf8Value filename(env->isolate(), args[1]); // Cast env->TryLoadAddon(*filename, flags, [&](DLib* dlib) { + static Mutex dlib_load_mutex; + Mutex::ScopedLock lock(dlib_load_mutex); + const 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 = + node_module* mp = static_cast(uv_key_get(&thread_local_modpending)); uv_key_set(&thread_local_modpending, nullptr); @@ -257,17 +461,24 @@ void DLOpen(const FunctionCallbackInfo& args) { return false; } - if (mp == nullptr) { + if (mp != nullptr) { + mp->nm_dso_handle = dlib->handle_; + dlib->SaveInGlobalHandleMap(mp); + } else { if (auto callback = GetInitializerCallback(dlib)) { callback(exports, module, context); + return true; } else if (auto napi_callback = GetNapiInitializerCallback(dlib)) { napi_module_register_by_symbol(exports, module, context, napi_callback); + return true; } else { - dlib->Close(); - env->ThrowError("Module did not self-register."); - return false; + mp = dlib->GetSavedModuleFromGlobalHandleMap(); + if (mp == nullptr || mp->nm_context_register_func == nullptr) { + dlib->Close(); + env->ThrowError("Module did not self-register."); + return false; + } } - return true; } // -1 is used for N-API modules @@ -300,10 +511,8 @@ void DLOpen(const FunctionCallbackInfo& args) { } CHECK_EQ(mp->nm_flags & NM_F_BUILTIN, 0); - mp->nm_dso_handle = dlib->handle_; - mp->nm_link = modlist_addon; - modlist_addon = mp; - + // Do not keep the lock while running userland addon loading code. + Mutex::ScopedUnlock unlock(lock); 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) { diff --git a/src/node_binding.h b/src/node_binding.h index d73e18548ff47e..fcd7f34ac6a57a 100644 --- a/src/node_binding.h +++ b/src/node_binding.h @@ -18,6 +18,7 @@ enum { NM_F_BUILTIN = 1 << 0, // Unused. NM_F_LINKED = 1 << 1, NM_F_INTERNAL = 1 << 2, + NM_F_DELETEME = 1 << 3, }; #define NODE_MODULE_CONTEXT_AWARE_CPP(modname, regfunc, priv, flags) \ @@ -62,6 +63,8 @@ class DLib { bool Open(); void Close(); void* GetSymbolAddress(const char* name); + void SaveInGlobalHandleMap(node_module* mp); + node_module* GetSavedModuleFromGlobalHandleMap(); const std::string filename_; const int flags_; @@ -70,6 +73,7 @@ class DLib { #ifndef __POSIX__ uv_lib_t lib_; #endif + bool has_entry_in_global_handle_map_ = false; private: DISALLOW_COPY_AND_ASSIGN(DLib); diff --git a/test/addons/dlopen-ping-pong/test-worker.js b/test/addons/dlopen-ping-pong/test-worker.js new file mode 100644 index 00000000000000..feba6aa5eb0202 --- /dev/null +++ b/test/addons/dlopen-ping-pong/test-worker.js @@ -0,0 +1,20 @@ +'use strict'; +const common = require('../../common'); + +if (common.isWindows) + common.skip('dlopen global symbol loading is not supported on this os.'); + +const assert = require('assert'); +const { Worker } = require('worker_threads'); + +// Check that modules that are not declared as context-aware cannot be re-loaded +// from workers. + +const bindingPath = require.resolve(`./build/${common.buildType}/binding`); +require(bindingPath); + +new Worker(`require(${JSON.stringify(bindingPath)})`, { eval: true }) + .on('error', common.mustCall((err) => { + assert.strictEqual(err.constructor, Error); + assert.strictEqual(err.message, 'Module did not self-register.'); + })); diff --git a/test/addons/worker-addon/binding.cc b/test/addons/worker-addon/binding.cc index faf3ba8647e5bc..01c857c43ebcfc 100644 --- a/test/addons/worker-addon/binding.cc +++ b/test/addons/worker-addon/binding.cc @@ -21,7 +21,7 @@ struct statically_allocated { } ~statically_allocated() { assert(count == 0); - printf("dtor"); + printf("dtor "); } } var; diff --git a/test/addons/worker-addon/test.js b/test/addons/worker-addon/test.js index 7fb7c1a87aa903..ef158e98b14469 100644 --- a/test/addons/worker-addon/test.js +++ b/test/addons/worker-addon/test.js @@ -1,3 +1,4 @@ +// Flags: --experimental-report 'use strict'; const common = require('../../common'); const assert = require('assert'); @@ -6,21 +7,62 @@ const path = require('path'); const { Worker } = require('worker_threads'); const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`); -if (process.argv[2] === 'worker') { - new Worker(`require(${JSON.stringify(binding)});`, { eval: true }); - return; -} else if (process.argv[2] === 'main-thread') { - process.env.addExtraItemToEventLoop = 'yes'; - require(binding); - return; +switch (process.argv[2]) { + case 'both': + require(binding); + // fallthrough + case 'worker-twice': + case 'worker': + const worker = new Worker(`require(${JSON.stringify(binding)});`, { + eval: true + }); + if (process.argv[2] === 'worker-twice') { + worker.on('exit', common.mustCall(() => { + new Worker(`require(${JSON.stringify(binding)});`, { + eval: true + }); + })); + } + return; + case 'main-thread': + process.env.addExtraItemToEventLoop = 'yes'; + require(binding); + return; } -for (const test of ['worker', 'main-thread']) { +// Use process.report to figure out if we might be running under musl libc. +const glibc = JSON.parse(process.report.getReport()).header.glibcVersionRuntime; +assert(typeof glibc === 'string' || glibc === undefined, glibc); + +const libcMayBeMusl = common.isLinux && glibc === undefined; + +for (const { test, expected } of [ + { test: 'worker', expected: [ 'ctor cleanup dtor ' ] }, + { test: 'main-thread', expected: [ 'ctor cleanup dtor ' ] }, + // We always only have 1 instance of the shared object in memory, so + // 1 ctor and 1 dtor call. If we attach the module to 2 Environments, + // we expect 2 cleanup calls, otherwise one. + { test: 'both', expected: [ 'ctor cleanup cleanup dtor ' ] }, + { + test: 'worker-twice', + // In this case, we load and unload an addon, then load and unload again. + // musl doesn't support unloading, so the output may be missing + // a dtor + ctor pair. + expected: [ + 'ctor cleanup dtor ctor cleanup dtor ' + ].concat(libcMayBeMusl ? [ + 'ctor cleanup cleanup dtor ', + ] : []) + }, +]) { + console.log('spawning test', test); const proc = child_process.spawnSync(process.execPath, [ __filename, test ]); + process.stderr.write(proc.stderr.toString()); assert.strictEqual(proc.stderr.toString(), ''); - assert.strictEqual(proc.stdout.toString(), 'ctor cleanup dtor'); + assert(expected.includes(proc.stdout.toString()), + `${proc.stdout.toString()} is not included in ${expected}`); assert.strictEqual(proc.status, 0); } diff --git a/test/node-api/1_hello_world/test.js b/test/node-api/1_hello_world/test.js index d1d67d34f688c9..dd28e26a561295 100644 --- a/test/node-api/1_hello_world/test.js +++ b/test/node-api/1_hello_world/test.js @@ -1,6 +1,8 @@ 'use strict'; const common = require('../../common'); const assert = require('assert'); +const { Worker } = require('worker_threads'); + const bindingPath = require.resolve(`./build/${common.buildType}/binding`); const binding = require(bindingPath); assert.strictEqual(binding.hello(), 'world'); @@ -11,3 +13,10 @@ delete require.cache[bindingPath]; const rebinding = require(bindingPath); assert.strictEqual(rebinding.hello(), 'world'); assert.notStrictEqual(binding.hello, rebinding.hello); + +// Test that workers can load addons declared using NAPI_MODULE_INIT(). +new Worker(` +const { parentPort } = require('worker_threads'); +const msg = require(${JSON.stringify(bindingPath)}).hello(); +parentPort.postMessage(msg)`, { eval: true }) + .on('message', common.mustCall((msg) => assert.strictEqual(msg, 'world'))); diff --git a/test/node-api/test_worker_terminate/test.js b/test/node-api/test_worker_terminate/test.js index 2bfaab8e8eb0a9..7c7224c073dda3 100644 --- a/test/node-api/test_worker_terminate/test.js +++ b/test/node-api/test_worker_terminate/test.js @@ -4,6 +4,11 @@ const assert = require('assert'); const { Worker, isMainThread, workerData } = require('worker_threads'); if (isMainThread) { + // Load the addon in the main thread first. + // This checks that N-API addons can be loaded from multiple contexts + // when they are not loaded through NAPI_MODULE(). + require(`./build/${common.buildType}/test_worker_terminate`); + const counter = new Int32Array(new SharedArrayBuffer(4)); const worker = new Worker(__filename, { workerData: { counter } }); worker.on('exit', common.mustCall(() => { diff --git a/test/node-api/test_worker_terminate/test_worker_terminate.c b/test/node-api/test_worker_terminate/test_worker_terminate.c index a88d936293e2a8..517cae42037323 100644 --- a/test/node-api/test_worker_terminate/test_worker_terminate.c +++ b/test/node-api/test_worker_terminate/test_worker_terminate.c @@ -36,4 +36,6 @@ napi_value Init(napi_env env, napi_value exports) { return exports; } +// Do not start using NAPI_MODULE_INIT() here, so that we can test +// compatibility of Workers with NAPI_MODULE(). NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)