diff options
author | Anna Henningsen <anna@addaleax.net> | 2019-02-07 21:19:07 +0100 |
---|---|---|
committer | Daniel Bevenius <daniel.bevenius@gmail.com> | 2019-02-12 05:38:18 +0100 |
commit | 93417ac99521f0164dfacbbc0f7d30806d1ec0e3 (patch) | |
tree | d414e9ee589c518352f74c5dbf5cd1d46e8bcca5 | |
parent | 86799326077611d694f37753ee3ab6903397f893 (diff) | |
download | android-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.js | 13 | ||||
-rw-r--r-- | test/parallel/test-domain-async-id-map-leak.js | 36 |
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(); |