domain: avoid circular memory references

addaleax · addaleax · commit 60c5099f4b6b · 2019-02-13T21:56:37.000+01:00
Avoid circular references that the JS engine cannot see through because it involves an `async id` ⇒ `domain` link. Using weak references is not a great solution, because it increases the domain module’s dependency on internals and the added calls into C++ may affect performance, but it seems like the least bad one. PR-URL: #25993 Fixes: #23862 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Refael Ackermann <refack@gmail.com>
diff --git a/lib/domain.js b/lib/domain.js
@@ -35,6 +35,10 @@ const {
 } = require('internal/errors').codes;
 const { createHook } = require('async_hooks');
 
+// 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];
@@ -53,7 +57,7 @@ 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);
+      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);
diff --git a/test/parallel/test-domain-async-id-map-leak.js b/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 &rarr; 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 &rarr; resource &rarr; async id &rArr; domain &rarr; 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();
