From 93417ac99521f0164dfacbbc0f7d30806d1ec0e3 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Thu, 7 Feb 2019 21:19:07 +0100
Subject: domain: avoid circular memory references
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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: https://github.com/nodejs/node/pull/25993
Fixes: https://github.com/nodejs/node/issues/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>
---
 lib/domain.js                                  | 13 +++++++---
 test/parallel/test-domain-async-id-map-leak.js | 36 ++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 3 deletions(-)
 create mode 100644 test/parallel/test-domain-async-id-map-leak.js

diff --git a/lib/domain.js b/lib/domain.js
index 8d96683312..031350746c 100644
--- 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,20 +57,22 @@ 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;
     }
   },
   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) {
@@ -167,6 +173,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
new file mode 100644
index 0000000000..e720241841
--- /dev/null
+++ 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 → 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();
-- 
cgit v1.2.3