domain: use strong reference to domain while active

When an uncaught exception is thrown inside a domain, the domain is removed from the stack as of 43a5170. This means that it might not be kept alive as an object anymore, and may be garbage collected before the `after()` hook can run, which tries to exit it as well. Resolve that by making references to the domain strong while it is active. Fixes: #28275 PR-URL: #28313 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Rich Trott <[email protected]>
nodejs · Aug 2, 2019 · 727ffe4 · 727ffe4
1 parent ae56a23
commit 727ffe4
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 1 deletion.
diff --git a/lib/domain.js b/lib/domain.js
@@ -73,13 +73,18 @@ const asyncHook = createHook({
     if (current !== undefined) { // Enter domain for this cb
       // We will get the domain through current.get(), because the resource
       // object's .domain property makes sure it is not garbage collected.
+      // However, we do need to make the reference to the domain non-weak,
+      // so that it cannot be garbage collected before the after() hook.
+      current.incRef();
       current.get().enter();
     }
   },
   after(asyncId) {
     const current = pairing.get(asyncId);
     if (current !== undefined) { // Exit domain for this cb
-      current.get().exit();
+      const domain = current.get();
+      current.decRef();
+      domain.exit();
     }
   },
   destroy(asyncId) {

diff --git a/src/node_util.cc b/src/node_util.cc
@@ -189,12 +189,26 @@ class WeakReference : public BaseObject {
       args.GetReturnValue().Set(weak_ref->target_.Get(isolate));
   }
 
+  static void IncRef(const FunctionCallbackInfo<Value>& args) {
+    WeakReference* weak_ref = Unwrap<WeakReference>(args.Holder());
+    if (weak_ref->reference_count_ == 0) weak_ref->target_.ClearWeak();
+    weak_ref->reference_count_++;
+  }
+
+  static void DecRef(const FunctionCallbackInfo<Value>& args) {
+    WeakReference* weak_ref = Unwrap<WeakReference>(args.Holder());
+    CHECK_GE(weak_ref->reference_count_, 1);
+    weak_ref->reference_count_--;
+    if (weak_ref->reference_count_ == 0) weak_ref->target_.SetWeak();
+  }
+
   SET_MEMORY_INFO_NAME(WeakReference)
   SET_SELF_SIZE(WeakReference)
   SET_NO_MEMORY_INFO()
 
  private:
   Global<Object> target_;
+  uint64_t reference_count_ = 0;
 };
 
 static void GuessHandleType(const FunctionCallbackInfo<Value>& args) {
@@ -294,6 +308,8 @@ void Initialize(Local<Object> target,
   weak_ref->InstanceTemplate()->SetInternalFieldCount(1);
   weak_ref->SetClassName(weak_ref_string);
   env->SetProtoMethod(weak_ref, "get", WeakReference::Get);
+  env->SetProtoMethod(weak_ref, "incRef", WeakReference::IncRef);
+  env->SetProtoMethod(weak_ref, "decRef", WeakReference::DecRef);
   target->Set(context, weak_ref_string,
               weak_ref->GetFunction(context).ToLocalChecked()).Check();
 

diff --git a/test/parallel/test-domain-error-types.js b/test/parallel/test-domain-error-types.js
@@ -1,3 +1,4 @@
+// Flags: --gc-interval=100 --stress-compaction
 'use strict';
 
 const common = require('../common');
@@ -6,6 +7,8 @@ const domain = require('domain');
 
 // This test is similar to test-domain-multiple-errors, but uses a new domain
 // for each errors.
+// The test flags are not essential, but serve as a way to verify that
+// https://github.com/nodejs/node/issues/28275 is fixed in debug mode.
 
 for (const something of [
   42, null, undefined, false, () => {}, 'string', Symbol('foo')