src: restrict unloading addons to Worker threads

Unloading native addons from the main thread was an (presumably unintended) significant breaking change, because addons may rely on their memory being available after an `Environment` exits. This patch only restricts this to Worker threads, at least for the time being, and thus matches the behaviour from #23319. PR-URL: #25577 Refs: #24861 Refs: #23319 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
nodejs · Jan 23, 2019 · ef1c639 · ef1c639
1 parent b070c99
commit ef1c639
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 6 deletions.
diff --git a/src/env.cc b/src/env.cc
@@ -276,9 +276,16 @@ 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();
+  // Do not unload addons on the main thread. Some addons need to retain memory
+  // beyond the Environment's lifetime, and unloading them early would break
+  // them; with Worker threads, we have the opportunity to be stricter.
+  // Also, since the main thread usually stops just before the process exits,
+  // this is far less relevant here.
+  if (!is_main_thread()) {
+    // Dereference all addons that were loaded into this environment.
+    for (binding::DLib& addon : loaded_addons_) {
+      addon.Close();
+    }
   }
 }
 

diff --git a/test/addons/worker-addon/binding.cc b/test/addons/worker-addon/binding.cc
@@ -3,6 +3,7 @@
 #include <stdio.h>
 #include <stdlib.h>
 #include <v8.h>
+#include <uv.h>
 
 using v8::Context;
 using v8::HandleScope;
@@ -41,6 +42,17 @@ void Initialize(Local<Object> exports,
       const_cast<void*>(static_cast<const void*>("cleanup")));
   node::AddEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr);
   node::RemoveEnvironmentCleanupHook(context->GetIsolate(), Dummy, nullptr);
+
+  if (getenv("addExtraItemToEventLoop") != nullptr) {
+    // Add an item to the event loop that we do not clean up in order to make
+    // sure that for the main thread, this addon's memory persists even after
+    // the Environment instance has been destroyed.
+    static uv_async_t extra_async;
+    uv_loop_t* loop = node::GetCurrentEventLoop(context->GetIsolate());
+    int err = uv_async_init(loop, &extra_async, [](uv_async_t*) {});
+    assert(err == 0);
+    uv_unref(reinterpret_cast<uv_handle_t*>(&extra_async));
+  }
 }
 
 NODE_MODULE_CONTEXT_AWARE(NODE_GYP_MODULE_NAME, Initialize)
diff --git a/test/addons/worker-addon/test.js b/test/addons/worker-addon/test.js
@@ -6,12 +6,19 @@ const path = require('path');
 const { Worker } = require('worker_threads');
 const binding = path.resolve(__dirname, `./build/${common.buildType}/binding`);
 
-if (process.argv[2] === 'child') {
+if (process.argv[2] === 'worker') {
   new Worker(`require(${JSON.stringify(binding)});`, { eval: true });
-} else {
+  return;
+} else if (process.argv[2] === 'main-thread') {
+  process.env.addExtraItemToEventLoop = 'yes';
+  require(binding);
+  return;
+}
+
+for (const test of ['worker', 'main-thread']) {
   const proc = child_process.spawnSync(process.execPath, [
     __filename,
-    'child'
+    test
   ]);
   assert.strictEqual(proc.stderr.toString(), '');
   assert.strictEqual(proc.stdout.toString(), 'ctor cleanup dtor');