From 49781f3a057e35a7695597b55fd002066585a918 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Sun, 17 Feb 2019 23:45:14 +0100 Subject: [PATCH 01/11] 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 --- doc/api/addons.md | 5 ++ doc/api/worker_threads.md | 4 +- src/node_api.cc | 2 +- src/node_binding.cc | 67 ++++++++++++++++--- src/node_binding.h | 1 + test/addons/dlopen-ping-pong/test-worker.js | 16 +++++ test/addons/worker-addon/test.js | 27 +++++--- test/node-api/1_hello_world/test.js | 9 +++ test/node-api/test_worker_terminate/test.js | 5 ++ .../test_worker_terminate.c | 2 + 10 files changed, 116 insertions(+), 22 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..f4fea1647640f2 100644 --- a/doc/api/worker_threads.md +++ b/doc/api/worker_threads.md @@ -360,11 +360,12 @@ Notable differences inside a Worker environment are: being invoked. - IPC channels from parent processes are not accessible. - The [`trace_events`][] module is not supported. +- Native add-ons can only be loaded from multiple threads if they fulfill + [certain conditions][Addons worker support]. Currently, the following differences also exist until they are addressed: - The [`inspector`][] module is not available yet. -- Native addons are not supported yet. Creating `Worker` instances inside of other `Worker`s is possible. @@ -602,6 +603,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..8936cb615145b2 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -110,7 +110,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 +135,48 @@ 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] = Entry { 1, mod }; + } + + 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.module->nm_flags & NM_F_DELETEME) + delete it->second.module; + map_.erase(handle); + } + } + + private: + Mutex mutex_; + struct Entry { + unsigned int refcount; + node_module* module; + }; + std::unordered_map map_; +} global_handle_map; + DLib::DLib(const char* filename, int flags) : filename_(filename), flags_(flags), handle_(nullptr) {} @@ -149,6 +190,7 @@ bool DLib::Open() { void DLib::Close() { if (handle_ == nullptr) return; + global_handle_map.erase(handle_); dlclose(handle_); handle_ = nullptr; } @@ -240,7 +282,7 @@ void DLOpen(const FunctionCallbackInfo& args) { // 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 +299,24 @@ void DLOpen(const FunctionCallbackInfo& args) { return false; } - if (mp == nullptr) { + if (mp != nullptr) { + mp->nm_dso_handle = dlib->handle_; + global_handle_map.set(dlib->handle_, 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 = global_handle_map.get_and_increase_refcount(dlib->handle_); + 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 +349,6 @@ 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; - 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..ff69f326524536 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) \ 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..0db6035d15dd09 --- /dev/null +++ b/test/addons/dlopen-ping-pong/test-worker.js @@ -0,0 +1,16 @@ +'use strict'; +const common = require('../../common'); +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/test.js b/test/addons/worker-addon/test.js index 7fb7c1a87aa903..666b83f42df4d3 100644 --- a/test/addons/worker-addon/test.js +++ b/test/addons/worker-addon/test.js @@ -6,21 +6,30 @@ 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': + 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']) { +for (const test of ['worker', 'main-thread', 'both']) { const proc = child_process.spawnSync(process.execPath, [ __filename, test ]); assert.strictEqual(proc.stderr.toString(), ''); - assert.strictEqual(proc.stdout.toString(), '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. + assert.strictEqual( + proc.stdout.toString(), + test === 'both' ? 'ctor cleanup cleanup dtor' : 'ctor cleanup dtor'); 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) From 28017abc7bc7f0d9e1ef3ba3f375a93237157ab7 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 18 Feb 2019 00:48:35 +0100 Subject: [PATCH 02/11] fixup! worker: improve integration with native addons --- src/node_binding.cc | 27 +++++++++++++++++++++++---- src/node_binding.h | 3 +++ 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/node_binding.cc b/src/node_binding.cc index 8936cb615145b2..5202497b0a2960 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -141,7 +141,8 @@ static struct global_handle_map_t { CHECK_NE(handle, nullptr); Mutex::ScopedLock lock(mutex_); - map_[handle] = Entry { 1, mod }; + map_[handle].module = mod; + map_[handle].refcount++; } node_module* get_and_increase_refcount(void* handle) { @@ -190,7 +191,8 @@ bool DLib::Open() { void DLib::Close() { if (handle_ == nullptr) return; - global_handle_map.erase(handle_); + if (has_entry_in_global_handle_map_) + global_handle_map.erase(handle_); dlclose(handle_); handle_ = nullptr; } @@ -212,6 +214,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; } @@ -223,6 +227,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); @@ -277,6 +291,9 @@ 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 @@ -301,7 +318,7 @@ void DLOpen(const FunctionCallbackInfo& args) { if (mp != nullptr) { mp->nm_dso_handle = dlib->handle_; - global_handle_map.set(dlib->handle_, mp); + dlib->SaveInGlobalHandleMap(mp); } else { if (auto callback = GetInitializerCallback(dlib)) { callback(exports, module, context); @@ -310,7 +327,7 @@ void DLOpen(const FunctionCallbackInfo& args) { napi_module_register_by_symbol(exports, module, context, napi_callback); return true; } else { - mp = global_handle_map.get_and_increase_refcount(dlib->handle_); + mp = dlib->GetSavedModuleFromGlobalHandleMap(); if (mp == nullptr || mp->nm_context_register_func == nullptr) { dlib->Close(); env->ThrowError("Module did not self-register."); @@ -349,6 +366,8 @@ void DLOpen(const FunctionCallbackInfo& args) { } CHECK_EQ(mp->nm_flags & NM_F_BUILTIN, 0); + // 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 ff69f326524536..fcd7f34ac6a57a 100644 --- a/src/node_binding.h +++ b/src/node_binding.h @@ -63,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_; @@ -71,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); From b89e762e94c5b2b82d21e36bfe7d9ba06d1a983e Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Mon, 18 Feb 2019 09:20:43 +0100 Subject: [PATCH 03/11] fixup! worker: improve integration with native addons --- doc/api/worker_threads.md | 4 ---- 1 file changed, 4 deletions(-) diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md index f4fea1647640f2..8b9ceb5c39eafc 100644 --- a/doc/api/worker_threads.md +++ b/doc/api/worker_threads.md @@ -363,10 +363,6 @@ Notable differences inside a Worker environment are: - Native add-ons can only be loaded from multiple threads if they fulfill [certain conditions][Addons worker support]. -Currently, the following differences also exist until they are addressed: - -- The [`inspector`][] module is not available yet. - Creating `Worker` instances inside of other `Worker`s is possible. Like [Web Workers][] and the [`cluster` module][], two-way communication can be From 170629f28b807c3de0b8d4af4b280d85aacb6dd5 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 19 Feb 2019 20:28:32 +0100 Subject: [PATCH 04/11] fixup! worker: improve integration with native addons --- doc/api/worker_threads.md | 1 - 1 file changed, 1 deletion(-) diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md index 8b9ceb5c39eafc..54bd66a6ea683e 100644 --- a/doc/api/worker_threads.md +++ b/doc/api/worker_threads.md @@ -577,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 From 2fb648c2b6b4bda5b4e3bc6c3164f8c7ac3243d4 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Tue, 19 Feb 2019 22:44:58 +0100 Subject: [PATCH 05/11] fixup! worker: improve integration with native addons --- test/addons/dlopen-ping-pong/test-worker.js | 4 ++++ test/addons/worker-addon/test.js | 2 ++ 2 files changed, 6 insertions(+) diff --git a/test/addons/dlopen-ping-pong/test-worker.js b/test/addons/dlopen-ping-pong/test-worker.js index 0db6035d15dd09..feba6aa5eb0202 100644 --- a/test/addons/dlopen-ping-pong/test-worker.js +++ b/test/addons/dlopen-ping-pong/test-worker.js @@ -1,5 +1,9 @@ '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'); diff --git a/test/addons/worker-addon/test.js b/test/addons/worker-addon/test.js index 666b83f42df4d3..562158d2d3505f 100644 --- a/test/addons/worker-addon/test.js +++ b/test/addons/worker-addon/test.js @@ -20,10 +20,12 @@ switch (process.argv[2]) { } for (const test of ['worker', 'main-thread', 'both']) { + 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(), ''); // 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, From 404301d5c88d70e06626789e25a34e4b18079c1f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 20 Feb 2019 13:18:39 +0100 Subject: [PATCH 06/11] fixup! worker: improve integration with native addons --- test/addons/worker-addon/binding.cc | 2 +- test/addons/worker-addon/test.js | 28 ++++++++++++++++++++++------ 2 files changed, 23 insertions(+), 7 deletions(-) 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 562158d2d3505f..a0236883e893d2 100644 --- a/test/addons/worker-addon/test.js +++ b/test/addons/worker-addon/test.js @@ -10,8 +10,18 @@ switch (process.argv[2]) { case 'both': require(binding); // fallthrough + case 'worker-twice': case 'worker': - new Worker(`require(${JSON.stringify(binding)});`, { eval: true }); + 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'; @@ -19,7 +29,16 @@ switch (process.argv[2]) { return; } -for (const test of ['worker', 'main-thread', 'both']) { +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 ' }, + // In this case, we load and unload an addon, then load and unload again. + { test: 'worker-twice', expected: 'ctor cleanup dtor ctor cleanup dtor ' }, +]) { console.log('spawning test', test); const proc = child_process.spawnSync(process.execPath, [ __filename, @@ -27,11 +46,8 @@ for (const test of ['worker', 'main-thread', 'both']) { ]); process.stderr.write(proc.stderr.toString()); assert.strictEqual(proc.stderr.toString(), ''); - // 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. assert.strictEqual( proc.stdout.toString(), - test === 'both' ? 'ctor cleanup cleanup dtor' : 'ctor cleanup dtor'); + expected); assert.strictEqual(proc.status, 0); } From 2f97646ef0180e5eee35872d946c7200c2b6f5f8 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 20 Feb 2019 13:26:58 +0100 Subject: [PATCH 07/11] fixup! worker: improve integration with native addons --- src/node_binding.cc | 108 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/src/node_binding.cc b/src/node_binding.cc index 5202497b0a2960..11367c3ba46c89 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -96,6 +96,114 @@ 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; + 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) { + 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 + namespace node { using v8::Context; From 859610f9ebca1d37280312a15820c61066dd1e75 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 20 Feb 2019 15:58:25 +0100 Subject: [PATCH 08/11] fixup! worker: improve integration with native addons --- src/node_binding.cc | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/src/node_binding.cc b/src/node_binding.cc index 11367c3ba46c89..e8a077898203b1 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -180,6 +180,7 @@ 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(); @@ -190,6 +191,8 @@ int wrapped_dlclose(void* handle) { } 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); } @@ -299,9 +302,17 @@ bool DLib::Open() { void DLib::Close() { if (handle_ == nullptr) return; - if (has_entry_in_global_handle_map_) - global_handle_map.erase(handle_); - dlclose(handle_); + int err = dlclose(handle_); + + if (err == 0) { + // musl libc implements dlclose() as a no-op which returns 1. + // 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 dlclose() fails, we assume that the shared object + // still exists and keep it in our handle map. + if (has_entry_in_global_handle_map_) + global_handle_map.erase(handle_); + } handle_ = nullptr; } From d1b8fd987a785104bef81e0de20677cb38a7c02b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 20 Feb 2019 16:29:10 +0100 Subject: [PATCH 09/11] fixup! worker: improve integration with native addons --- src/node_binding.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/node_binding.cc b/src/node_binding.cc index e8a077898203b1..8ddc265e52477a 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -253,6 +253,13 @@ static struct global_handle_map_t { 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++; } @@ -274,7 +281,7 @@ static struct global_handle_map_t { if (it == map_.end()) return; CHECK_GE(it->second.refcount, 1); if (--it->second.refcount == 0) { - if (it->second.module->nm_flags & NM_F_DELETEME) + if (it->second.wants_delete_module) delete it->second.module; map_.erase(handle); } @@ -284,6 +291,7 @@ static struct global_handle_map_t { Mutex mutex_; struct Entry { unsigned int refcount; + bool wants_delete_module; node_module* module; }; std::unordered_map map_; From 73ddc807aa5b3bfe44086c6d4f7bafce36b9ed7f Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 20 Feb 2019 16:42:59 +0100 Subject: [PATCH 10/11] fixup! worker: improve integration with native addons --- test/addons/worker-addon/test.js | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/test/addons/worker-addon/test.js b/test/addons/worker-addon/test.js index a0236883e893d2..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'); @@ -29,15 +30,30 @@ switch (process.argv[2]) { return; } +// 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 ' }, + { 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 ' }, - // In this case, we load and unload an addon, then load and unload again. - { test: 'worker-twice', expected: 'ctor cleanup dtor ctor cleanup dtor ' }, + { 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, [ @@ -46,8 +62,7 @@ for (const { test, expected } of [ ]); process.stderr.write(proc.stderr.toString()); assert.strictEqual(proc.stderr.toString(), ''); - assert.strictEqual( - proc.stdout.toString(), - expected); + assert(expected.includes(proc.stdout.toString()), + `${proc.stdout.toString()} is not included in ${expected}`); assert.strictEqual(proc.status, 0); } From 9c0f9d3dbd31ad1316e1849e69e0f9de6336f980 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 20 Feb 2019 17:17:23 +0100 Subject: [PATCH 11/11] fixup! worker: improve integration with native addons --- src/node_binding.cc | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/node_binding.cc b/src/node_binding.cc index 8ddc265e52477a..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) @@ -207,6 +208,19 @@ void* wrapped_dlsym(void* handle, const char* symbol) { #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; @@ -310,14 +324,18 @@ bool DLib::Open() { void DLib::Close() { if (handle_ == nullptr) return; - int err = dlclose(handle_); - if (err == 0) { - // musl libc implements dlclose() as a no-op which returns 1. + 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 dlclose() fails, we assume that the shared object - // still exists and keep it in our handle map. + // 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_); }