src: unload addons when environment quits

This is an alternative to #23319
which attaches the loaded addons to the environment and closes them
when the environment is destroyed.
PR-URL: #24861
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

Author: Gabriel Schulhof

src/env-inl.h

@@ -396,6 +396,16 @@ inline uv_loop_t* Environment::event_loop() const {
 returnisolate_data()->event_loop();
 }
 
+inlinevoidEnvironment::TryLoadAddon(
+constchar* filename,
+int flags,
+ std::function<bool(binding::DLib*)> 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_;
 }

src/env.cc

@@ -276,6 +276,11 @@ Environment::~Environment() {
 
 TRACE_EVENT_NESTABLE_ASYNC_END0(
 TRACING_CATEGORY_NODE1(environment), "Environment", this);
+
+// Dereference all addons that were loaded into this environment.
+for (binding::DLib& addon : loaded_addons_) {
+ addon.Close();
+ }
 }
 
 voidEnvironment::Start(const std::vector<std::string>& args,

src/env.h

@@ -30,18 +30,20 @@
 #endif
 #include"handle_wrap.h"
 #include"node.h"
+#include"node_binding.h"
 #include"node_http2_state.h"
 #include"node_options.h"
 #include"req_wrap.h"
 #include"util.h"
 #include"uv.h"
 #include"v8.h"
 
-#include<list>
 #include<stdint.h>
-#include<vector>
+#include<functional>
+#include<list>
 #include<unordered_map>
 #include<unordered_set>
+#include<vector>
 
 structnghttp2_rcbuf;
 
@@ -636,6 +638,9 @@ class Environment {
 inline v8::Isolate* isolate() const;
 inlineuv_loop_t* event_loop() const;
 inlineuint32_twatched_providers() const;
+inlinevoidTryLoadAddon(constchar* filename,
+int flags,
+ std::function<bool(binding::DLib*)> was_loaded);
 
 staticinline Environment* from_timer_handle(uv_timer_t* handle);
 inlineuv_timer_t* timer_handle();
@@ -922,6 +927,7 @@ class Environment {
 inlinevoidThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
 const char* errmsg);
 
+ std::list<binding::DLib> loaded_addons_;
  v8::Isolate* const isolate_;
  IsolateData* const isolate_data_;
 uv_timer_t timer_handle_;

src/node_binding.cc

@@ -2,10 +2,6 @@
 #include"node_internals.h"
 #include"node_native_module.h"
 
-#if defined(__POSIX__)
-#include<dlfcn.h>
-#endif
-
 #if HAVE_OPENSSL
 #defineNODE_BUILTIN_OPENSSL_MODULES(V) V(crypto) V(tls_wrap)
 #else
@@ -126,31 +122,8 @@ extern "C" void node_module_register(void* m) {
 
 namespacebinding {
 
-classDLib {
-public:
-#ifdef __POSIX__
-staticconstintkDefaultFlags = RTLD_LAZY;
-#else
-staticconstintkDefaultFlags = 0;
-#endif
-
-inlineDLib(constchar* filename, int flags)
- : filename_(filename), flags_(flags), handle_(nullptr) {}
-
-inlineboolOpen();
-inlinevoidClose();
-inlinevoid* GetSymbolAddress(constchar* name);
-
-const std::string filename_;
-constint flags_;
- std::string errmsg_;
-void* handle_;
-#ifndef __POSIX__
-uv_lib_t lib_;
-#endif
-private:
-DISALLOW_COPY_AND_ASSIGN(DLib);
-};
+DLib::DLib(constchar* filename, int flags)
+ : filename_(filename), flags_(flags), handle_(nullptr) {}
 
 #ifdef __POSIX__
 boolDLib::Open() {
@@ -247,87 +220,92 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
  }
 
  node::Utf8Value filename(env->isolate(), args[1]); // Cast
- DLib dlib(*filename, 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<node_module*>(uv_key_get(&thread_local_modpending));
-uv_key_set(&thread_local_modpending, nullptr);
-
-if (!is_opened) {
- Local<String> errmsg = OneByteString(env->isolate(), dlib.errmsg_.c_str());
- dlib.Close();
+ env->TryLoadAddon(*filename, flags, [&](DLib* dlib) {
+constbool 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<node_module*>(uv_key_get(&thread_local_modpending));
+uv_key_set(&thread_local_modpending, nullptr);
+
+if (!is_opened) {
+ Local<String> 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;
- }
+env->isolate()->ThrowException(Exception::Error(errmsg));
+returnfalse;
+}
 
-if (mp == nullptr) {
-if (auto callback = GetInitializerCallback(&dlib)) {
-callback(exports, module, context);
- } elseif (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)) {
+callback(exports, module, context);
+ } elseif (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.");
+returnfalse;
+ }
+returntrue;
  }
-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);
-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);
+returntrue;
+ }
+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);
+returnfalse;
+ }
+if (mp->nm_flags & NM_F_BUILTIN) {
+ dlib->Close();
+ env->ThrowError("Built-in module self-registered.");
+returnfalse;
  }
-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;
+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);
- } elseif (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;
- }
+if (mp->nm_context_register_func != nullptr) {
+ mp->nm_context_register_func(exports, module, context, mp->nm_priv);
+ } elseif (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.");
+returnfalse;
+ }
+
+returntrue;
+ });
 
 // Tell coverity that 'handle' should not be freed when we return.
 // coverity[leaked_storage]

src/node_binding.h

@@ -3,8 +3,14 @@
 
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
+#if defined(__POSIX__)
+#include<dlfcn.h>
+#endif
+
 #include"node.h"
+#defineNAPI_EXPERIMENTAL
 #include"node_api.h"
+#include"util.h"
 #include"uv.h"
 #include"v8.h"
 
@@ -46,6 +52,32 @@ extern bool node_is_initialized;
 
 namespacebinding {
 
+classDLib {
+public:
+#ifdef __POSIX__
+staticconstintkDefaultFlags = RTLD_LAZY;
+#else
+staticconstintkDefaultFlags = 0;
+#endif
+
+DLib(constchar* filename, int flags);
+
+boolOpen();
+voidClose();
+void* GetSymbolAddress(constchar* name);
+
+const std::string filename_;
+constint flags_;
+ std::string errmsg_;
+void* handle_;
+#ifndef __POSIX__
+uv_lib_t lib_;
+#endif
+
+private:
+DISALLOW_COPY_AND_ASSIGN(DLib);
+};
+
 // Call _register<module_name> 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<v8::Value>& args);
 
 } // namespace node
 
+#include"node_binding.h"
+
 #endif// defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 #endif// SRC_NODE_BINDING_H_

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);
+
 newWorker(`
  const binding = require(${JSON.stringify(bindingPath)});
 

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)
+#defineNODE_C_DTOR(fn) \
+NODE_CTOR_PREFIXvoid __cdecl fn(void); \
+__declspec(dllexport, allocate(".CRT$XPU")) \
+ void (__cdecl*fn ## _)(void) = fn; \
+NODE_CTOR_PREFIXvoid __cdecl fn(void)
+#else
+#defineNODE_C_DTOR(fn) \
+NODE_CTOR_PREFIXvoidfn(void) __attribute__((destructor)); \
+NODE_CTOR_PREFIXvoidfn(void)
+#endif
+
 staticchar cookie[] = "yum yum";
 staticint at_exit_cb1_called = 0;
 staticint at_exit_cb2_called = 0;
@@ -27,7 +40,7 @@ static void at_exit_cb2(void* arg) {
  at_exit_cb2_called++;
 }
 
-staticvoidsanity_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<Object> 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)