[v10.x] domain: avoid circular memory references

lib/domain.js

@@ -35,7 +35,11 @@ const {
 } = require('internal/errors').codes;
 const { createHook } = require('async_hooks');
 
-// overwrite process.domain with a getter/setter that will allow for more
+// TODO(addaleax): Use a non-internal solution for this.
+const kWeak = Symbol('kWeak');
+const { WeakReference } = internalBinding('util');
+
+// Overwrite process.domain with a getter/setter that will allow for more
 // effective optimizations
 var _domain = [null];
 Object.defineProperty(process, 'domain', {
@@ -52,8 +56,8 @@ const pairing = new Map();
 const asyncHook = createHook({
   init(asyncId, type, triggerAsyncId, resource) {
     if (process.domain !== null && process.domain !== undefined) {
-      // if this operation is created while in a domain, let's mark it
-      pairing.set(asyncId, process.domain);
+      // If this operation is created while in a domain, let's mark it
+      pairing.set(asyncId, process.domain[kWeak]);
       resource.domain = process.domain;
       if (resource.promise !== undefined &&
           resource.promise instanceof Promise) {
@@ -67,13 +71,15 @@ const asyncHook = createHook({
   before(asyncId) {
     const current = pairing.get(asyncId);
     if (current !== undefined) { // enter domain for this cb
-      current.enter();
+      // We will get the domain through current.get(), because the resource
+      // object's .domain property makes sure it is not garbage collected.
+      current.get().enter();
     }
   },
   after(asyncId) {
     const current = pairing.get(asyncId);
     if (current !== undefined) { // exit domain for this cb
-      current.exit();
+      current.get().exit();
     }
   },
   destroy(asyncId) {
@@ -174,6 +180,7 @@ class Domain extends EventEmitter {
     super();
 
     this.members = [];
+    this[kWeak] = new WeakReference(this);
     asyncHook.enable();
 
     this.on('removeListener', updateExceptionCapture);

src/node_util.cc

@@ -1,5 +1,6 @@
 #include "node_internals.h"
 #include "node_watchdog.h"
+#include "base_object-inl.h"
 
 namespace node {
 namespace util {
@@ -9,8 +10,10 @@ using v8::Array;
 using v8::Boolean;
 using v8::Context;
 using v8::FunctionCallbackInfo;
+using v8::FunctionTemplate;
 using v8::IndexFilter;
 using v8::Integer;
+using v8::Isolate;
 using v8::KeyCollectionMode;
 using v8::Local;
 using v8::NewStringType;
@@ -178,6 +181,37 @@ void SafeGetenv(const FunctionCallbackInfo<Value>& args) {
             NewStringType::kNormal).ToLocalChecked());
 }
 
+class WeakReference : public BaseObject {
+ public:
+  WeakReference(Environment* env, Local<Object> object, Local<Object> target)
+    : BaseObject(env, object) {
+    MakeWeak();
+    target_.Reset(env->isolate(), target);
+    target_.SetWeak();
+  }
+
+  static void New(const FunctionCallbackInfo<Value>& args) {
+    Environment* env = Environment::GetCurrent(args);
+    CHECK(args.IsConstructCall());
+    CHECK(args[0]->IsObject());
+    new WeakReference(env, args.This(), args[0].As<Object>());
+  }
+
+  static void Get(const FunctionCallbackInfo<Value>& args) {
+    WeakReference* weak_ref = Unwrap<WeakReference>(args.Holder());
+    Isolate* isolate = args.GetIsolate();
+    if (!weak_ref->target_.IsEmpty())
+      args.GetReturnValue().Set(weak_ref->target_.Get(isolate));
+  }
+
+  SET_MEMORY_INFO_NAME(WeakReference)
+  SET_SELF_SIZE(WeakReference)
+  SET_NO_MEMORY_INFO()
+
+ private:
+  Persistent<Object> target_;
+};
+
 void Initialize(Local<Object> target,
                 Local<Value> unused,
                 Local<Context> context) {
@@ -235,6 +269,16 @@ void Initialize(Local<Object> target,
   target->Set(context,
               FIXED_ONE_BYTE_STRING(env->isolate(), "propertyFilter"),
               constants).FromJust();
+
+  Local<String> weak_ref_string =
+      FIXED_ONE_BYTE_STRING(env->isolate(), "WeakReference");
+  Local<FunctionTemplate> weak_ref =
+      env->NewFunctionTemplate(WeakReference::New);
+  weak_ref->InstanceTemplate()->SetInternalFieldCount(1);
+  weak_ref->SetClassName(weak_ref_string);
+  env->SetProtoMethod(weak_ref, "get", WeakReference::Get);
+  target->Set(context, weak_ref_string,
+              weak_ref->GetFunction(context).ToLocalChecked()).FromJust();
 }
 
 }  // namespace util

test/parallel/test-domain-async-id-map-leak.js

@@ -0,0 +1,36 @@
+// Flags: --expose-gc
+'use strict';
+const common = require('../common');
+const onGC = require('../common/ongc');
+const assert = require('assert');
+const async_hooks = require('async_hooks');
+const domain = require('domain');
+const EventEmitter = require('events');
+
+// This test makes sure that the (async id → domain) map which is part of the
+// domain module does not get in the way of garbage collection.
+// See: https://github.com/nodejs/node/issues/23862
+
+let d = domain.create();
+d.run(() => {
+  const resource = new async_hooks.AsyncResource('TestResource');
+  const emitter = new EventEmitter();
+
+  d.remove(emitter);
+  d.add(emitter);
+
+  emitter.linkToResource = resource;
+  assert.strictEqual(emitter.domain, d);
+  assert.strictEqual(resource.domain, d);
+
+  // This would otherwise be a circular chain now:
+  // emitter → resource → async id ⇒ domain → emitter.
+  // Make sure that all of these objects are released:
+
+  onGC(resource, { ongc: common.mustCall() });
+  onGC(d, { ongc: common.mustCall() });
+  onGC(emitter, { ongc: common.mustCall() });
+});
+
+d = null;
+global.gc();

test/parallel/test-internal-util-weakreference.js

@@ -0,0 +1,17 @@
+// Flags: --expose-internals --expose-gc
+'use strict';
+require('../common');
+const assert = require('assert');
+const { internalBinding } = require('internal/test/binding');
+const { WeakReference } = internalBinding('util');
+
+let obj = { hello: 'world' };
+const ref = new WeakReference(obj);
+assert.strictEqual(ref.get(), obj);
+
+setImmediate(() => {
+  obj = null;
+  global.gc();
+
+  assert.strictEqual(ref.get(), undefined);
+});