From c991280c889986ed2f044070e93977a2f24e285c Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Wed, 5 Dec 2018 17:23:56 -0800 Subject: [PATCH 1/5] test,doc: add tests and docs for addon unloading Originally from portions of https://github.com/nodejs/node/pull/23319/. --- doc/api/addons.md | 18 ++++++++++ test/addons/hello-world/test-worker.js | 14 ++++++++ test/addons/worker-addon/binding.cc | 46 ++++++++++++++++++++++++++ test/addons/worker-addon/binding.gyp | 9 +++++ test/addons/worker-addon/test.js | 21 ++++++++++++ 5 files changed, 108 insertions(+) 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/doc/api/addons.md b/doc/api/addons.md index d993c72d346495..9bc75c7dfc15d1 100644 --- a/doc/api/addons.md +++ b/doc/api/addons.md @@ -234,6 +234,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 @@ -1349,6 +1366,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/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..1fb85ae230eb5f --- /dev/null +++ b/test/addons/worker-addon/binding.cc @@ -0,0 +1,46 @@ +#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_CONTEXT_AWARE(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..2f4bb512fc6571 --- /dev/null +++ b/test/addons/worker-addon/test.js @@ -0,0 +1,21 @@ +// 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, [ + '--experimental-worker', + __filename, + 'child' + ]); + assert.strictEqual(proc.stderr.toString(), ''); + assert.strictEqual(proc.stdout.toString(), 'ctor cleanup dtor'); + assert.strictEqual(proc.status, 0); +} From 005cbc9c269623a20bcdee635109f2bde82d1484 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Wed, 5 Dec 2018 17:24:49 -0800 Subject: [PATCH 2/5] src: unload addons when environment quits This is an alternative to https://github.com/nodejs/node/pull/23319 which attaches the loaded addons to the environment and closes them when the environment is destroyed. --- node.gyp | 1 + src/env-inl.h | 9 ++ src/env.cc | 5 ++ src/env.h | 10 ++- src/node.cc | 2 +- src/node_api.cc | 2 +- src/node_binding-inl.h | 59 ++++++++++++ src/node_binding.cc | 114 +++++------------------- src/node_binding.h | 34 +++++++ src/node_internals.h | 2 +- test/abort/test-addon-uv-handle-leak.js | 10 +++ test/addons/uv-handle-leak/binding.cc | 5 +- 12 files changed, 154 insertions(+), 99 deletions(-) create mode 100644 src/node_binding-inl.h diff --git a/node.gyp b/node.gyp index 7e8867e971aec3..265ca01945a46e 100644 --- a/node.gyp +++ b/node.gyp @@ -421,6 +421,7 @@ 'src/node_api.h', 'src/node_api_types.h', 'src/node_binding.h', + 'src/node_binding-inl.h', 'src/node_buffer.h', 'src/node_constants.h', 'src/node_context_data.h', diff --git a/src/env-inl.h b/src/env-inl.h index ba5704ed2e9574..96c8f533478b3e 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -396,6 +396,15 @@ inline uv_loop_t* Environment::event_loop() const { return isolate_data()->event_loop(); } +inline void Environment::TryLoadAddon(const char* filename, + int flags, + std::function was_loaded) { + loaded_addons_.emplace_back(filename, flags); + if (!was_loaded(&loaded_addons_.back())) { + loaded_addons_.pop_back(); + } +} + inline Environment::AsyncHooks* Environment::async_hooks() { return &async_hooks_; } diff --git a/src/env.cc b/src/env.cc index bdb3c1ea247e21..8e28cdeb2143b6 100644 --- a/src/env.cc +++ b/src/env.cc @@ -267,6 +267,11 @@ Environment::~Environment() { TRACE_EVENT_NESTABLE_ASYNC_END0( TRACING_CATEGORY_NODE1(environment), "Environment", this); + + // Dereference all addons that were loaded into this environment. + for (auto& addon : loaded_addons_) { + addon.Close(); + } } void Environment::Start(const std::vector& args, diff --git a/src/env.h b/src/env.h index 64593a0025c3b6..67ea5f2b6247ea 100644 --- a/src/env.h +++ b/src/env.h @@ -30,6 +30,7 @@ #endif #include "handle_wrap.h" #include "node.h" +#include "node_binding-inl.h" #include "node_http2_state.h" #include "node_options.h" #include "req_wrap.h" @@ -37,11 +38,12 @@ #include "uv.h" #include "v8.h" -#include #include -#include +#include +#include #include #include +#include struct nghttp2_rcbuf; @@ -636,6 +638,9 @@ class Environment { inline v8::Isolate* isolate() const; inline uv_loop_t* event_loop() const; inline uint32_t watched_providers() const; + inline void TryLoadAddon(const char* filename, + int flags, + std::function was_loaded); static inline Environment* from_timer_handle(uv_timer_t* handle); inline uv_timer_t* timer_handle(); @@ -921,6 +926,7 @@ class Environment { inline void ThrowError(v8::Local (*fun)(v8::Local), const char* errmsg); + std::list loaded_addons_; v8::Isolate* const isolate_; IsolateData* const isolate_data_; uv_timer_t timer_handle_; diff --git a/src/node.cc b/src/node.cc index ad0afd437dd4b5..e0db703d8890d3 100644 --- a/src/node.cc +++ b/src/node.cc @@ -19,7 +19,7 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE // USE OR OTHER DEALINGS IN THE SOFTWARE. -#include "node_binding.h" +#include "node_binding-inl.h" #include "node_buffer.h" #include "node_constants.h" #include "node_context_data.h" diff --git a/src/node_api.cc b/src/node_api.cc index 7d843c08f5a69d..894d10b61e1d04 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -3,7 +3,7 @@ #define NAPI_EXPERIMENTAL #include "js_native_api_v8.h" #include "node_api.h" -#include "node_binding.h" +#include "node_binding-inl.h" #include "node_errors.h" #include "node_internals.h" diff --git a/src/node_binding-inl.h b/src/node_binding-inl.h new file mode 100644 index 00000000000000..7d2703072f0fb8 --- /dev/null +++ b/src/node_binding-inl.h @@ -0,0 +1,59 @@ +#ifndef SRC_NODE_BINDING_INL_H_ +#define SRC_NODE_BINDING_INL_H_ + +#include "node_binding.h" + +namespace node { + +namespace binding { + +inline DLib::DLib(const char* filename, int flags): + filename_(filename), flags_(flags), handle_(nullptr) {} + +#ifdef __POSIX__ + inline bool DLib::Open() { + handle_ = dlopen(filename_.c_str(), flags_); + if (handle_ != nullptr) return true; + errmsg_ = dlerror(); + return false; + } + + inline void DLib::Close() { + if (handle_ == nullptr) return; + dlclose(handle_); + handle_ = nullptr; + } + + inline void* DLib::GetSymbolAddress(const char* name) { + return dlsym(handle_, name); + } +#else // !__POSIX__ + inline bool DLib::Open() { + int ret = uv_dlopen(filename_.c_str(), &lib_); + if (ret == 0) { + handle_ = static_cast(lib_.handle); + return true; + } + errmsg_ = uv_dlerror(&lib_); + uv_dlclose(&lib_); + return false; + } + + inline void DLib::Close() { + if (handle_ == nullptr) return; + uv_dlclose(&lib_); + handle_ = nullptr; + } + + inline void* DLib::GetSymbolAddress(const char* name) { + void* address; + if (0 == uv_dlsym(&lib_, name, &address)) return address; + return nullptr; + } +#endif // !__POSIX__ + +} // end of namespace binding + +} // end of namespace node + +#endif // SRC_NODE_BINDING_INL_H_ diff --git a/src/node_binding.cc b/src/node_binding.cc index 6d70d0a501e335..0653f40e80edb3 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -1,11 +1,7 @@ -#include "node_binding.h" +#include "node_binding-inl.h" #include "node_internals.h" #include "node_native_module.h" -#if defined(__POSIX__) -#include -#endif - #if HAVE_OPENSSL #define NODE_BUILTIN_OPENSSL_MODULES(V) V(crypto) V(tls_wrap) #else @@ -126,74 +122,6 @@ extern "C" void node_module_register(void* m) { namespace binding { -class DLib { - public: -#ifdef __POSIX__ - static const int kDefaultFlags = RTLD_LAZY; -#else - static const int kDefaultFlags = 0; -#endif - - inline DLib(const char* filename, int flags) - : filename_(filename), flags_(flags), handle_(nullptr) {} - - inline bool Open(); - inline void Close(); - inline void* GetSymbolAddress(const char* name); - - const std::string filename_; - const int flags_; - std::string errmsg_; - void* handle_; -#ifndef __POSIX__ - uv_lib_t lib_; -#endif - private: - DISALLOW_COPY_AND_ASSIGN(DLib); -}; - -#ifdef __POSIX__ -bool DLib::Open() { - handle_ = dlopen(filename_.c_str(), flags_); - if (handle_ != nullptr) return true; - errmsg_ = dlerror(); - return false; -} - -void DLib::Close() { - if (handle_ == nullptr) return; - dlclose(handle_); - handle_ = nullptr; -} - -void* DLib::GetSymbolAddress(const char* name) { - return dlsym(handle_, name); -} -#else // !__POSIX__ -bool DLib::Open() { - int ret = uv_dlopen(filename_.c_str(), &lib_); - if (ret == 0) { - handle_ = static_cast(lib_.handle); - return true; - } - errmsg_ = uv_dlerror(&lib_); - uv_dlclose(&lib_); - return false; -} - -void DLib::Close() { - if (handle_ == nullptr) return; - uv_dlclose(&lib_); - handle_ = nullptr; -} - -void* DLib::GetSymbolAddress(const char* name) { - void* address; - if (0 == uv_dlsym(&lib_, name, &address)) return address; - return nullptr; -} -#endif // !__POSIX__ - using InitializerCallback = void (*)(Local exports, Local module, Local context); @@ -247,8 +175,8 @@ void DLOpen(const FunctionCallbackInfo& args) { } node::Utf8Value filename(env->isolate(), args[1]); // Cast - DLib dlib(*filename, flags); - bool is_opened = dlib.Open(); + env->TryLoadAddon(*filename, flags, [&](DLib* dlib) { + 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 @@ -258,27 +186,28 @@ void DLOpen(const FunctionCallbackInfo& args) { uv_key_set(&thread_local_modpending, nullptr); if (!is_opened) { - Local errmsg = OneByteString(env->isolate(), dlib.errmsg_.c_str()); - dlib.Close(); + Local errmsg = OneByteString(env->isolate(), dlib->errmsg_.c_str()); + dlib->Close(); #ifdef _WIN32 // 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; + return false; } if (mp == nullptr) { - if (auto callback = GetInitializerCallback(&dlib)) { + if (auto callback = GetInitializerCallback(dlib)) { callback(exports, module, context); - } else if (auto napi_callback = GetNapiInitializerCallback(&dlib)) { + } else if (auto napi_callback = GetNapiInitializerCallback(dlib)) { napi_module_register_by_symbol(exports, module, context, napi_callback); } else { - dlib.Close(); + dlib->Close(); env->ThrowError("Module did not self-register."); + return false; } - return; + return true; } // -1 is used for N-API modules @@ -286,9 +215,9 @@ void DLOpen(const FunctionCallbackInfo& args) { // 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)) { + if (auto callback = GetInitializerCallback(dlib)) { callback(exports, module, context); - return; + return true; } char errmsg[1024]; snprintf(errmsg, @@ -305,17 +234,17 @@ void DLOpen(const FunctionCallbackInfo& args) { // NOTE: `mp` is allocated inside of the shared library's memory, calling // `dlclose` will deallocate it - dlib.Close(); + dlib->Close(); env->ThrowError(errmsg); - return; + return false; } if (mp->nm_flags & NM_F_BUILTIN) { - dlib.Close(); + dlib->Close(); env->ThrowError("Built-in module self-registered."); - return; + return false; } - mp->nm_dso_handle = dlib.handle_; + mp->nm_dso_handle = dlib->handle_; mp->nm_link = modlist_addon; modlist_addon = mp; @@ -324,11 +253,14 @@ void DLOpen(const FunctionCallbackInfo& args) { } else if (mp->nm_register_func != nullptr) { mp->nm_register_func(exports, module, mp->nm_priv); } else { - dlib.Close(); + dlib->Close(); env->ThrowError("Module has no declared entry point."); - return; + return false; } + return true; + }); + // Tell coverity that 'handle' should not be freed when we return. // coverity[leaked_storage] } diff --git a/src/node_binding.h b/src/node_binding.h index 743c82accd09b8..8d56dcacfdb19e 100644 --- a/src/node_binding.h +++ b/src/node_binding.h @@ -3,8 +3,14 @@ #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS +#if defined(__POSIX__) +#include +#endif + #include "node.h" +#define NAPI_EXPERIMENTAL #include "node_api.h" +#include "util.h" #include "uv.h" #include "v8.h" @@ -46,6 +52,32 @@ extern bool node_is_initialized; namespace binding { +class DLib { + public: +#ifdef __POSIX__ + static const int kDefaultFlags = RTLD_LAZY; +#else + static const int kDefaultFlags = 0; +#endif + + DLib(const char* filename, int flags); + + bool Open(); + void Close(); + void* GetSymbolAddress(const char* name); + + const std::string filename_; + const int flags_; + std::string errmsg_; + void* handle_; +#ifndef __POSIX__ + uv_lib_t lib_; +#endif + + private: + DISALLOW_COPY_AND_ASSIGN(DLib); +}; + // Call _register functions for all of // the built-in modules. Because built-in modules don't // use the __attribute__((constructor)). Need to @@ -60,5 +92,7 @@ void DLOpen(const v8::FunctionCallbackInfo& args); } // namespace node +#include "node_binding-inl.h" + #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #endif // SRC_NODE_BINDING_H_ diff --git a/src/node_internals.h b/src/node_internals.h index 1d43d4b1419b9c..803172b8570908 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -26,7 +26,7 @@ #include "env-inl.h" #include "node.h" -#include "node_binding.h" +#include "node_binding-inl.h" #include "node_mutex.h" #include "node_persistent.h" #include "tracing/trace_event.h" diff --git a/test/abort/test-addon-uv-handle-leak.js b/test/abort/test-addon-uv-handle-leak.js index 96608f8dda95fd..b67c5f7136af6e 100644 --- a/test/abort/test-addon-uv-handle-leak.js +++ b/test/abort/test-addon-uv-handle-leak.js @@ -18,6 +18,16 @@ if (!fs.existsSync(bindingPath)) common.skip('binding not built yet'); if (process.argv[2] === 'child') { + + // The worker thread loads and then unloads `bindingPath`. Because of this the + // symbols in `bindingPath` are lost when the worker thread quits, but the + // number of open handles in the worker thread's event loop is assessed in the + // main thread afterwards, and the names of the callbacks associated with the + // open handles is retrieved at that time as well. Thus, we require + // `bindingPath` here so that the symbols and their names survive the life + // cycle of the worker thread. + require(bindingPath); + new Worker(` const binding = require(${JSON.stringify(bindingPath)}); diff --git a/test/addons/uv-handle-leak/binding.cc b/test/addons/uv-handle-leak/binding.cc index c2e5f0bf27bf16..221a1284323171 100644 --- a/test/addons/uv-handle-leak/binding.cc +++ b/test/addons/uv-handle-leak/binding.cc @@ -41,8 +41,7 @@ void LeakHandle(const FunctionCallbackInfo& args) { uv_unref(reinterpret_cast(leaked_timer)); } -void Initialize(v8::Local exports) { +// This module gets loaded multiple times in some tests so it must support that. +NODE_MODULE_INIT(/*exports, module, context*/) { NODE_SET_METHOD(exports, "leakHandle", LeakHandle); } - -NODE_MODULE(NODE_GYP_MODULE_NAME, Initialize) From df7aa7afeac635618660329b47244be6f6d9209e Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Thu, 6 Dec 2018 11:35:38 -0800 Subject: [PATCH 3/5] squash! whitespace fixes --- src/env-inl.h | 7 +- src/node_binding-inl.h | 78 +++++++++++----------- src/node_binding.cc | 147 +++++++++++++++++++++-------------------- 3 files changed, 117 insertions(+), 115 deletions(-) diff --git a/src/env-inl.h b/src/env-inl.h index 96c8f533478b3e..ecbf39cd0b15b8 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -396,9 +396,10 @@ inline uv_loop_t* Environment::event_loop() const { return isolate_data()->event_loop(); } -inline void Environment::TryLoadAddon(const char* filename, - int flags, - std::function was_loaded) { +inline void Environment::TryLoadAddon( + const char* filename, + int flags, + std::function was_loaded) { loaded_addons_.emplace_back(filename, flags); if (!was_loaded(&loaded_addons_.back())) { loaded_addons_.pop_back(); diff --git a/src/node_binding-inl.h b/src/node_binding-inl.h index 7d2703072f0fb8..0c74236969be1e 100644 --- a/src/node_binding-inl.h +++ b/src/node_binding-inl.h @@ -7,49 +7,49 @@ namespace node { namespace binding { -inline DLib::DLib(const char* filename, int flags): - filename_(filename), flags_(flags), handle_(nullptr) {} +inline DLib::DLib(const char* filename, int flags) + : filename_(filename), flags_(flags), handle_(nullptr) {} #ifdef __POSIX__ - inline bool DLib::Open() { - handle_ = dlopen(filename_.c_str(), flags_); - if (handle_ != nullptr) return true; - errmsg_ = dlerror(); - return false; - } - - inline void DLib::Close() { - if (handle_ == nullptr) return; - dlclose(handle_); - handle_ = nullptr; - } - - inline void* DLib::GetSymbolAddress(const char* name) { - return dlsym(handle_, name); - } +inline bool DLib::Open() { + handle_ = dlopen(filename_.c_str(), flags_); + if (handle_ != nullptr) return true; + errmsg_ = dlerror(); + return false; +} + +inline void DLib::Close() { + if (handle_ == nullptr) return; + dlclose(handle_); + handle_ = nullptr; +} + +inline void* DLib::GetSymbolAddress(const char* name) { + return dlsym(handle_, name); +} #else // !__POSIX__ - inline bool DLib::Open() { - int ret = uv_dlopen(filename_.c_str(), &lib_); - if (ret == 0) { - handle_ = static_cast(lib_.handle); - return true; - } - errmsg_ = uv_dlerror(&lib_); - uv_dlclose(&lib_); - return false; - } - - inline void DLib::Close() { - if (handle_ == nullptr) return; - uv_dlclose(&lib_); - handle_ = nullptr; - } - - inline void* DLib::GetSymbolAddress(const char* name) { - void* address; - if (0 == uv_dlsym(&lib_, name, &address)) return address; - return nullptr; +inline bool DLib::Open() { + int ret = uv_dlopen(filename_.c_str(), &lib_); + if (ret == 0) { + handle_ = static_cast(lib_.handle); + return true; } + errmsg_ = uv_dlerror(&lib_); + uv_dlclose(&lib_); + return false; +} + +inline void DLib::Close() { + if (handle_ == nullptr) return; + uv_dlclose(&lib_); + handle_ = nullptr; +} + +inline void* DLib::GetSymbolAddress(const char* name) { + void* address; + if (0 == uv_dlsym(&lib_, name, &address)) return address; + return nullptr; +} #endif // !__POSIX__ } // end of namespace binding diff --git a/src/node_binding.cc b/src/node_binding.cc index 0653f40e80edb3..79d1c42a67e081 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -176,89 +176,90 @@ void DLOpen(const FunctionCallbackInfo& args) { node::Utf8Value filename(env->isolate(), args[1]); // Cast env->TryLoadAddon(*filename, flags, [&](DLib* dlib) { - 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 = - 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()); - dlib->Close(); + 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 = + 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()); + dlib->Close(); #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 false; - } - - 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."); + env->isolate()->ThrowException(Exception::Error(errmsg)); return false; } - return true; - } - // -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); + 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."); + return false; + } return true; } - 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 false; - } - if (mp->nm_flags & NM_F_BUILTIN) { - dlib->Close(); - env->ThrowError("Built-in module self-registered."); - return false; - } - mp->nm_dso_handle = dlib->handle_; - mp->nm_link = modlist_addon; - modlist_addon = mp; + // -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); + return true; + } + 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 false; + } + if (mp->nm_flags & NM_F_BUILTIN) { + dlib->Close(); + env->ThrowError("Built-in module self-registered."); + return 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 false; - } + mp->nm_dso_handle = dlib->handle_; + mp->nm_link = modlist_addon; + modlist_addon = mp; - return true; + 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 false; + } + + return true; }); // Tell coverity that 'handle' should not be freed when we return. From 584f6434d4472156d0a70624318a3c48327b3f05 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Mon, 10 Dec 2018 23:43:37 -0800 Subject: [PATCH 4/5] review suggestions --- node.gyp | 1 - src/env.cc | 2 +- src/env.h | 2 +- src/node.cc | 2 +- src/node_api.cc | 2 +- src/node_binding-inl.h | 59 ------------------------------------------ src/node_binding.cc | 47 ++++++++++++++++++++++++++++++++- src/node_binding.h | 2 +- src/node_internals.h | 2 +- 9 files changed, 52 insertions(+), 67 deletions(-) delete mode 100644 src/node_binding-inl.h diff --git a/node.gyp b/node.gyp index 265ca01945a46e..7e8867e971aec3 100644 --- a/node.gyp +++ b/node.gyp @@ -421,7 +421,6 @@ 'src/node_api.h', 'src/node_api_types.h', 'src/node_binding.h', - 'src/node_binding-inl.h', 'src/node_buffer.h', 'src/node_constants.h', 'src/node_context_data.h', diff --git a/src/env.cc b/src/env.cc index 8e28cdeb2143b6..66ce46b58426fa 100644 --- a/src/env.cc +++ b/src/env.cc @@ -269,7 +269,7 @@ Environment::~Environment() { TRACING_CATEGORY_NODE1(environment), "Environment", this); // Dereference all addons that were loaded into this environment. - for (auto& addon : loaded_addons_) { + for (binding::DLib& addon : loaded_addons_) { addon.Close(); } } diff --git a/src/env.h b/src/env.h index 67ea5f2b6247ea..475c6063a3b0f4 100644 --- a/src/env.h +++ b/src/env.h @@ -30,7 +30,7 @@ #endif #include "handle_wrap.h" #include "node.h" -#include "node_binding-inl.h" +#include "node_binding.h" #include "node_http2_state.h" #include "node_options.h" #include "req_wrap.h" diff --git a/src/node.cc b/src/node.cc index e0db703d8890d3..ad0afd437dd4b5 100644 --- a/src/node.cc +++ b/src/node.cc @@ -19,7 +19,7 @@ // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE // USE OR OTHER DEALINGS IN THE SOFTWARE. -#include "node_binding-inl.h" +#include "node_binding.h" #include "node_buffer.h" #include "node_constants.h" #include "node_context_data.h" diff --git a/src/node_api.cc b/src/node_api.cc index 894d10b61e1d04..7d843c08f5a69d 100644 --- a/src/node_api.cc +++ b/src/node_api.cc @@ -3,7 +3,7 @@ #define NAPI_EXPERIMENTAL #include "js_native_api_v8.h" #include "node_api.h" -#include "node_binding-inl.h" +#include "node_binding.h" #include "node_errors.h" #include "node_internals.h" diff --git a/src/node_binding-inl.h b/src/node_binding-inl.h deleted file mode 100644 index 0c74236969be1e..00000000000000 --- a/src/node_binding-inl.h +++ /dev/null @@ -1,59 +0,0 @@ -#ifndef SRC_NODE_BINDING_INL_H_ -#define SRC_NODE_BINDING_INL_H_ - -#include "node_binding.h" - -namespace node { - -namespace binding { - -inline DLib::DLib(const char* filename, int flags) - : filename_(filename), flags_(flags), handle_(nullptr) {} - -#ifdef __POSIX__ -inline bool DLib::Open() { - handle_ = dlopen(filename_.c_str(), flags_); - if (handle_ != nullptr) return true; - errmsg_ = dlerror(); - return false; -} - -inline void DLib::Close() { - if (handle_ == nullptr) return; - dlclose(handle_); - handle_ = nullptr; -} - -inline void* DLib::GetSymbolAddress(const char* name) { - return dlsym(handle_, name); -} -#else // !__POSIX__ -inline bool DLib::Open() { - int ret = uv_dlopen(filename_.c_str(), &lib_); - if (ret == 0) { - handle_ = static_cast(lib_.handle); - return true; - } - errmsg_ = uv_dlerror(&lib_); - uv_dlclose(&lib_); - return false; -} - -inline void DLib::Close() { - if (handle_ == nullptr) return; - uv_dlclose(&lib_); - handle_ = nullptr; -} - -inline void* DLib::GetSymbolAddress(const char* name) { - void* address; - if (0 == uv_dlsym(&lib_, name, &address)) return address; - return nullptr; -} -#endif // !__POSIX__ - -} // end of namespace binding - -} // end of namespace node - -#endif // SRC_NODE_BINDING_INL_H_ diff --git a/src/node_binding.cc b/src/node_binding.cc index 79d1c42a67e081..85d7f98452c132 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -1,4 +1,4 @@ -#include "node_binding-inl.h" +#include "node_binding.h" #include "node_internals.h" #include "node_native_module.h" @@ -122,6 +122,51 @@ extern "C" void node_module_register(void* m) { namespace binding { +DLib::DLib(const char* filename, int flags) + : filename_(filename), flags_(flags), handle_(nullptr) {} + +#ifdef __POSIX__ +bool DLib::Open() { + handle_ = dlopen(filename_.c_str(), flags_); + if (handle_ != nullptr) return true; + errmsg_ = dlerror(); + return false; +} + +void DLib::Close() { + if (handle_ == nullptr) return; + dlclose(handle_); + handle_ = nullptr; +} + +void* DLib::GetSymbolAddress(const char* name) { + return dlsym(handle_, name); +} +#else // !__POSIX__ +bool DLib::Open() { + int ret = uv_dlopen(filename_.c_str(), &lib_); + if (ret == 0) { + handle_ = static_cast(lib_.handle); + return true; + } + errmsg_ = uv_dlerror(&lib_); + uv_dlclose(&lib_); + return false; +} + +void DLib::Close() { + if (handle_ == nullptr) return; + uv_dlclose(&lib_); + handle_ = nullptr; +} + +void* DLib::GetSymbolAddress(const char* name) { + void* address; + if (0 == uv_dlsym(&lib_, name, &address)) return address; + return nullptr; +} +#endif // !__POSIX__ + using InitializerCallback = void (*)(Local exports, Local module, Local context); diff --git a/src/node_binding.h b/src/node_binding.h index 8d56dcacfdb19e..7d79dae80d8e39 100644 --- a/src/node_binding.h +++ b/src/node_binding.h @@ -92,7 +92,7 @@ void DLOpen(const v8::FunctionCallbackInfo& args); } // namespace node -#include "node_binding-inl.h" +#include "node_binding.h" #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS #endif // SRC_NODE_BINDING_H_ diff --git a/src/node_internals.h b/src/node_internals.h index 803172b8570908..1d43d4b1419b9c 100644 --- a/src/node_internals.h +++ b/src/node_internals.h @@ -26,7 +26,7 @@ #include "env-inl.h" #include "node.h" -#include "node_binding-inl.h" +#include "node_binding.h" #include "node_mutex.h" #include "node_persistent.h" #include "tracing/trace_event.h" From 0b5d5975ba630386463a95493621f16c28929118 Mon Sep 17 00:00:00 2001 From: Gabriel Schulhof Date: Mon, 17 Dec 2018 13:38:45 -0800 Subject: [PATCH 5/5] replace atexit(3) with library destructor --- test/addons/at-exit/binding.cc | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/test/addons/at-exit/binding.cc b/test/addons/at-exit/binding.cc index 4dd9b0f304758f..3a27bcd7c30756 100644 --- a/test/addons/at-exit/binding.cc +++ b/test/addons/at-exit/binding.cc @@ -9,6 +9,19 @@ using v8::Isolate; using v8::Local; using v8::Object; +#if defined(_MSC_VER) +#pragma section(".CRT$XPU", read) +#define NODE_C_DTOR(fn) \ + NODE_CTOR_PREFIX void __cdecl fn(void); \ + __declspec(dllexport, allocate(".CRT$XPU")) \ + void (__cdecl*fn ## _)(void) = fn; \ + NODE_CTOR_PREFIX void __cdecl fn(void) +#else +#define NODE_C_DTOR(fn) \ + NODE_CTOR_PREFIX void fn(void) __attribute__((destructor)); \ + NODE_CTOR_PREFIX void fn(void) +#endif + static char cookie[] = "yum yum"; static int at_exit_cb1_called = 0; static int at_exit_cb2_called = 0; @@ -27,7 +40,7 @@ static void at_exit_cb2(void* arg) { at_exit_cb2_called++; } -static void sanity_check(void) { +NODE_C_DTOR(sanity_check) { assert(at_exit_cb1_called == 1); assert(at_exit_cb2_called == 2); } @@ -36,7 +49,6 @@ void init(Local exports) { AtExit(at_exit_cb1, exports->GetIsolate()); AtExit(at_exit_cb2, cookie); AtExit(at_exit_cb2, cookie); - atexit(sanity_check); } NODE_MODULE(NODE_GYP_MODULE_NAME, init)