summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAnna Henningsen <anna@addaleax.net>2019-02-07 21:19:07 +0100
committerDaniel Bevenius <daniel.bevenius@gmail.com>2019-02-12 05:38:18 +0100
commit93417ac99521f0164dfacbbc0f7d30806d1ec0e3 (patch)
treed414e9ee589c518352f74c5dbf5cd1d46e8bcca5
parent86799326077611d694f37753ee3ab6903397f893 (diff)
downloadandroid-node-v8-93417ac99521f0164dfacbbc0f7d30806d1ec0e3.tar.gz
android-node-v8-93417ac99521f0164dfacbbc0f7d30806d1ec0e3.tar.bz2
android-node-v8-93417ac99521f0164dfacbbc0f7d30806d1ec0e3.zip
domain: avoid circular memory references
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>
-rw-r--r--lib/domain.js13
-rw-r--r--test/parallel/test-domain-async-id-map-leak.js36
2 files changed, 46 insertions, 3 deletions
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();