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.
nodejs · gabrielschulhof · Dec 6, 2018 · Dec 6, 2018 · Dec 6, 2018 · Dec 11, 2018
commit 005cbc9c269623a20bcdee635109f2bde82d1484
diff --git 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
@@ -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<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_;
 }

diff --git 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<std::string>& args,

diff --git a/src/env.h b/src/env.h
@@ -30,18 +30,20 @@
 #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"
 #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>
 
 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<bool(binding::DLib*)> 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<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_;

diff --git 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
@@ -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
@@ -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<void*>(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
@@ -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 <dlfcn.h>
-#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<void*>(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<Object> exports,
                                      Local<Value> module,
                                      Local<Context> context);
@@ -247,8 +175,8 @@ void DLOpen(const FunctionCallbackInfo<Value>& 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,37 +186,38 @@ void DLOpen(const FunctionCallbackInfo<Value>& args) {
   uv_key_set(&thread_local_modpending, nullptr);
 
   if (!is_opened) {
-    Local<String> errmsg = OneByteString(env->isolate(), dlib.errmsg_.c_str());
-    dlib.Close();
+    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());
 #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
   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)) {
+    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<Value>& 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<Value>& 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
@@ -3,8 +3,14 @@
 
 #if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 
+#if defined(__POSIX__)
+#include <dlfcn.h>
+#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<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-inl.h"
+
 #endif  // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS
 #endif  // SRC_NODE_BINDING_H_