worker: fix premature addon unload

src/env-inl.h

@@ -280,6 +280,10 @@ inline uv_loop_t* Environment::event_loop() const {
   return isolate_data()->event_loop();
 }
 
+inline std::list<binding::DLib>& Environment::loaded_addons() {
+  return loaded_addons_;
+}
+
 #if HAVE_INSPECTOR
 inline bool Environment::is_in_inspector_console_call() const {
   return is_in_inspector_console_call_;

src/env.cc

@@ -1073,18 +1073,6 @@ Environment::~Environment() {
   TRACE_EVENT_NESTABLE_ASYNC_END0(
     TRACING_CATEGORY_NODE1(environment), "Environment", this);
 
-  // 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();
-    }
-  }
-
   delete external_memory_accounter_;
   if (cpu_profiler_) {
     for (auto& it : pending_profiles_) {

src/env.h

@@ -703,6 +703,7 @@ class Environment final : public MemoryRetainer {
   inline cppgc::AllocationHandle& cppgc_allocation_handle() const;
   inline v8::ExternalMemoryAccounter* external_memory_accounter() const;
   inline uv_loop_t* event_loop() const;
+  inline std::list<binding::DLib>& loaded_addons();
   void TryLoadAddon(const char* filename,
                     int flags,
                     const std::function<bool(binding::DLib*)>& was_loaded);

src/node_worker.cc

@@ -258,6 +258,11 @@ class WorkerThreadData {
     if (!loop_init_failed_) {
       CheckedUvLoopClose(&loop_);
     }
+
+    // Dereference all addons that were loaded into this environment.
+    for (binding::DLib& addon : loaded_addons_) {
+      addon.Close();
+    }
   }
 
   bool loop_is_usable() const { return !loop_init_failed_; }
@@ -266,6 +271,7 @@ class WorkerThreadData {
   Worker* const w_;
   uv_loop_t loop_;
   bool loop_init_failed_ = true;
+  std::list<binding::DLib> loaded_addons_;
   DeleteFnPtr<IsolateData, FreeIsolateData> isolate_data_;
   friend class Worker;
 };
@@ -322,6 +328,8 @@ void Worker::Run() {
       if (!env_) return;
       env_->set_can_call_into_js(false);
 
+      std::swap(data.loaded_addons_, env_->loaded_addons());
+
       {
         Mutex::ScopedLock lock(mutex_);
         stopped_ = true;

test/addons/worker-addon-exit/binding.cc

@@ -0,0 +1,99 @@
+#include <node.h>
+#include <node_object_wrap.h>
+
+using v8::Context;
+using v8::Function;
+using v8::FunctionCallbackInfo;
+using v8::FunctionTemplate;
+using v8::Isolate;
+using v8::Local;
+using v8::Number;
+using v8::Object;
+using v8::ObjectTemplate;
+using v8::String;
+using v8::Value;
+
+class MyObject : public node::ObjectWrap {
+ public:
+  static void Init(v8::Local<v8::Object> exports);
+  ~MyObject();
+
+ private:
+  explicit MyObject(double value = 0);
+
+  static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
+  static void PlusOne(const v8::FunctionCallbackInfo<v8::Value>& args);
+
+  double value_;
+};
+
+MyObject::MyObject(double value) : value_(value) {}
+
+MyObject::~MyObject() {}
+
+void MyObject::Init(Local<Object> exports) {
+  Isolate* isolate = Isolate::GetCurrent();
+  Local<Context> context = isolate->GetCurrentContext();
+
+  Local<ObjectTemplate> addon_data_tpl = ObjectTemplate::New(isolate);
+  addon_data_tpl->SetInternalFieldCount(1);  // 1 field for the MyObject::New()
+  Local<Object> addon_data =
+      addon_data_tpl->NewInstance(context).ToLocalChecked();
+
+  // Prepare constructor template
+  Local<FunctionTemplate> tpl = FunctionTemplate::New(isolate, New, addon_data);
+  tpl->SetClassName(String::NewFromUtf8(isolate, "MyObject").ToLocalChecked());
+  tpl->InstanceTemplate()->SetInternalFieldCount(1);
+
+  // Prototype
+  NODE_SET_PROTOTYPE_METHOD(tpl, "plusOne", PlusOne);
+
+  Local<Function> constructor = tpl->GetFunction(context).ToLocalChecked();
+  addon_data->SetInternalField(0, constructor);
+  exports
+      ->Set(context,
+            String::NewFromUtf8(isolate, "MyObject").ToLocalChecked(),
+            constructor)
+      .FromJust();
+}
+
+void MyObject::New(const FunctionCallbackInfo<Value>& args) {
+  Isolate* isolate = args.GetIsolate();
+  Local<Context> context = isolate->GetCurrentContext();
+
+  if (args.IsConstructCall()) {
+    // Invoked as constructor: `new MyObject(...)`
+    double value =
+        args[0]->IsUndefined() ? 0 : args[0]->NumberValue(context).FromMaybe(0);
+    MyObject* obj = new MyObject(value);
+    obj->Wrap(args.This());
+    args.GetReturnValue().Set(args.This());
+  } else {
+    // Invoked as plain function `MyObject(...)`, turn into construct call.
+    const int argc = 1;
+    Local<Value> argv[argc] = {args[0]};
+    Local<Function> cons = args.Data()
+                               .As<Object>()
+                               ->GetInternalField(0)
+                               .As<Value>()
+                               .As<Function>();
+    Local<Object> result =
+        cons->NewInstance(context, argc, argv).ToLocalChecked();
+    args.GetReturnValue().Set(result);
+  }
+}
+
+void MyObject::PlusOne(const FunctionCallbackInfo<Value>& args) {
+  Isolate* isolate = args.GetIsolate();
+
+  MyObject* obj = ObjectWrap::Unwrap<MyObject>(args.This());
+  obj->value_ += 1;
+
+  args.GetReturnValue().Set(Number::New(isolate, obj->value_));
+}
+
+void InitAll(Local<Object> exports) {
+  MyObject::Init(exports);
+}
+
+NODE_MODULE(NODE_GYP_MODULE_NAME, InitAll)

test/addons/worker-addon-exit/binding.gyp

@@ -0,0 +1,9 @@
+{
+  'targets': [
+    {
+      'target_name': 'binding',
+      'sources': [ 'binding.cc' ],
+      'includes': ['../common.gypi'],
+    }
+  ]
+}

test/addons/worker-addon-exit/test.js

@@ -0,0 +1,19 @@
+'use strict';
+const common = require('../../common');
+const { isMainThread, Worker } = require('worker_threads');
+
+if (!isMainThread) {
+  const addon = require(`./build/${common.buildType}/binding`);
+
+  // Create some garbage
+  const arr = [];
+  for (let i = 0; i < 1e5; i++) arr.push(`${i}`.repeat(100));
+
+  new addon.MyObject(10);
+
+  // Should not segfault
+  throw new Error('exit');
+} else {
+  const worker = new Worker(__filename);
+  worker.on('error', () => {});
+}