From ebaae7762d925310fd8d9c316d434736c55af69b Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 7 Feb 2019 21:18:25 +0100 Subject: [PATCH 1/4] src: add WeakReference utility Add a simple `WeakReference` utility that we can use until the language provides something on its own. --- src/node_util.cc | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/src/node_util.cc b/src/node_util.cc index 2a7d90cfe2b661..bc8f6345e6d1b5 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -1,6 +1,7 @@ #include "node_errors.h" #include "node_watchdog.h" #include "util.h" +#include "base_object-inl.h" namespace node { namespace util { @@ -10,6 +11,7 @@ using v8::Array; using v8::Boolean; using v8::Context; using v8::Function; +using v8::FunctionTemplate; using v8::FunctionCallbackInfo; using v8::IndexFilter; using v8::Integer; @@ -181,6 +183,37 @@ void EnqueueMicrotask(const FunctionCallbackInfo& args) { isolate->EnqueueMicrotask(args[0].As()); } +class WeakReference : public BaseObject { + public: + WeakReference(Environment* env, Local object, Local target) + : BaseObject(env, object) { + MakeWeak(); + target_.Reset(env->isolate(), target); + target_.SetWeak(); + } + + static void New(const FunctionCallbackInfo& args) { + Environment* env = Environment::GetCurrent(args); + CHECK(args.IsConstructCall()); + CHECK(args[0]->IsObject()); + new WeakReference(env, args.This(), args[0].As()); + } + + static void Get(const FunctionCallbackInfo& args) { + WeakReference* weak_ref = Unwrap(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 target_; +}; + void Initialize(Local target, Local unused, Local context, @@ -241,6 +274,16 @@ void Initialize(Local target, should_abort_on_uncaught_toggle, env->should_abort_on_uncaught_toggle().GetJSArray()) .FromJust()); + + Local weak_ref_string = + FIXED_ONE_BYTE_STRING(env->isolate(), "WeakReference"); + Local 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 From 6081b5cb729c22b3658f81ff7e8fc592970503b2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Thu, 7 Feb 2019 21:19:07 +0100 Subject: [PATCH 2/4] 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. Fixes: https://github.com/nodejs/node/issues/23862 --- lib/domain.js | 11 ++++-- .../parallel/test-domain-async-id-map-leak.js | 36 +++++++++++++++++++ 2 files changed, 44 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 8d966833125f9d..82a256fa412411 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,20 @@ 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(); + 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 +171,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 00000000000000..e720241841d130 --- /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(); From fa56b3542b7d405ef0d734e0616dcf0ceee6bae2 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 8 Feb 2019 14:02:22 +0100 Subject: [PATCH 3/4] fixup! src: add WeakReference utility --- src/node_util.cc | 2 +- .../test-internal-util-weakreference.js | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-internal-util-weakreference.js diff --git a/src/node_util.cc b/src/node_util.cc index bc8f6345e6d1b5..4cca0cbb72aed0 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -11,8 +11,8 @@ using v8::Array; using v8::Boolean; using v8::Context; using v8::Function; -using v8::FunctionTemplate; using v8::FunctionCallbackInfo; +using v8::FunctionTemplate; using v8::IndexFilter; using v8::Integer; using v8::Isolate; diff --git a/test/parallel/test-internal-util-weakreference.js b/test/parallel/test-internal-util-weakreference.js new file mode 100644 index 00000000000000..b48b34fe2309ea --- /dev/null +++ b/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); +}); From 30449e8409b3d02f9a72412216de9c9346193e1a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 8 Feb 2019 14:05:18 +0100 Subject: [PATCH 4/4] fixup! domain: avoid circular memory references --- lib/domain.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/domain.js b/lib/domain.js index 82a256fa412411..031350746cc5b8 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -64,6 +64,8 @@ const asyncHook = createHook({ before(asyncId) { const current = pairing.get(asyncId); if (current !== undefined) { // enter domain for this cb + // 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(); } },