From 08396744d46ceeca5aecc64ed96742bd68db8f70 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 7 Aug 2023 16:24:06 +0200 Subject: [PATCH 1/3] test: make WeakReference tests robust Previously we assume that the objects are GC'ed after one global.gc() returns, which is not necessarily always the case. Use gcUntil() to run GC multiple times if they are not GC'ed in the first time around. --- test/fixtures/snapshot/weak-reference-gc.js | 21 ++++++++++++++----- .../parallel/test-domain-async-id-map-leak.js | 17 +++++++++++---- .../test-internal-util-weakreference.js | 12 ++++++----- 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/test/fixtures/snapshot/weak-reference-gc.js b/test/fixtures/snapshot/weak-reference-gc.js index d8bfdf95d177..8dada530e77c 100644 --- a/test/fixtures/snapshot/weak-reference-gc.js +++ b/test/fixtures/snapshot/weak-reference-gc.js @@ -5,16 +5,27 @@ const { WeakReference } = internalBinding('util'); const { setDeserializeMainFunction } = require('v8').startupSnapshot -const assert = require('assert'); let obj = { hello: 'world' }; const ref = new WeakReference(obj); +let gcCount = 0; +let maxGC = 10; -setDeserializeMainFunction(() => { - obj = null; +function run() { globalThis.gc(); - setImmediate(() => { - assert.strictEqual(ref.get(), undefined); + gcCount++; + if (ref.get() === undefined) { + return; + } else if (gcCount < maxGC) { + run(); + } else { + throw new Error(`Reference is still around after ${maxGC} GC`); + } }); +} + +setDeserializeMainFunction(() => { + obj = null; + run(); }); diff --git a/test/parallel/test-domain-async-id-map-leak.js b/test/parallel/test-domain-async-id-map-leak.js index 8c03aa940125..12e93ef3594e 100644 --- a/test/parallel/test-domain-async-id-map-leak.js +++ b/test/parallel/test-domain-async-id-map-leak.js @@ -13,6 +13,8 @@ const isEnumerable = Function.call.bind(Object.prototype.propertyIsEnumerable); // See: https://github.com/nodejs/node/issues/23862 let d = domain.create(); +let resourceGCed = false; let domainGCed = false; let + emitterGCed = false; d.run(() => { const resource = new async_hooks.AsyncResource('TestResource'); const emitter = new EventEmitter(); @@ -30,10 +32,17 @@ d.run(() => { // 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() }); + onGC(resource, { ongc: common.mustCall(() => { resourceGCed = true; }) }); + onGC(d, { ongc: common.mustCall(() => { domainGCed = true; }) }); + onGC(emitter, { ongc: common.mustCall(() => { emitterGCed = true; }) }); }); d = null; -global.gc(); + +async function main() { + await common.gcUntil( + 'All objects garbage collected', + () => resourceGCed && domainGCed && emitterGCed); +} + +main(); diff --git a/test/parallel/test-internal-util-weakreference.js b/test/parallel/test-internal-util-weakreference.js index b48b34fe2309..75a00176bb09 100644 --- a/test/parallel/test-internal-util-weakreference.js +++ b/test/parallel/test-internal-util-weakreference.js @@ -1,6 +1,6 @@ // Flags: --expose-internals --expose-gc 'use strict'; -require('../common'); +const common = require('../common'); const assert = require('assert'); const { internalBinding } = require('internal/test/binding'); const { WeakReference } = internalBinding('util'); @@ -9,9 +9,11 @@ let obj = { hello: 'world' }; const ref = new WeakReference(obj); assert.strictEqual(ref.get(), obj); -setImmediate(() => { +async function main() { obj = null; - global.gc(); + await common.gcUntil( + 'Reference is garbage collected', + () => ref.get() === undefined); +} - assert.strictEqual(ref.get(), undefined); -}); +main(); From dcc533095c5727f36c25c69ec3c6178896d3db56 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 7 Aug 2023 16:28:56 +0200 Subject: [PATCH 2/3] lib: implement WeakReference on top of JS WeakRef The C++ implementation can now be done entirely in JS using WeakRef. Re-implement it in JS instead to simplify the code. --- lib/diagnostics_channel.js | 2 +- lib/domain.js | 3 +- lib/internal/util.js | 34 +++++++++++++++++++ test/fixtures/snapshot/weak-reference-gc.js | 3 +- test/fixtures/snapshot/weak-reference.js | 3 +- .../test-internal-util-weakreference.js | 3 +- 6 files changed, 39 insertions(+), 9 deletions(-) diff --git a/lib/diagnostics_channel.js b/lib/diagnostics_channel.js index dae0e930a395..10d35054f565 100644 --- a/lib/diagnostics_channel.js +++ b/lib/diagnostics_channel.js @@ -28,7 +28,7 @@ const { const { triggerUncaughtException } = internalBinding('errors'); -const { WeakReference } = internalBinding('util'); +const { WeakReference } = require('internal/util'); // Can't delete when weakref count reaches 0 as it could increment again. // Only GC can be used as a valid time to clean up the channels map. diff --git a/lib/domain.js b/lib/domain.js index 51565795d720..7da672a36915 100644 --- a/lib/domain.js +++ b/lib/domain.js @@ -52,9 +52,8 @@ const { const { createHook } = require('async_hooks'); const { useDomainTrampoline } = require('internal/async_hooks'); -// TODO(addaleax): Use a non-internal solution for this. const kWeak = Symbol('kWeak'); -const { WeakReference } = internalBinding('util'); +const { WeakReference } = require('internal/util'); // Overwrite process.domain with a getter/setter that will allow for more // effective optimizations diff --git a/lib/internal/util.js b/lib/internal/util.js index 1e1a647e6938..3586084ba7b8 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -33,6 +33,7 @@ const { SafeMap, SafeSet, SafeWeakMap, + SafeWeakRef, StringPrototypeReplace, StringPrototypeToLowerCase, StringPrototypeToUpperCase, @@ -797,6 +798,38 @@ function guessHandleType(fd) { return handleTypes[type]; } +class WeakReference { + #weak = null; + #strong = null; + #refCount = 0; + constructor(object) { + this.#weak = new SafeWeakRef(object); + } + + incRef() { + this.#refCount++; + if (this.#refCount === 1) { + const derefed = this.#weak.deref(); + if (derefed !== undefined) { + this.#strong = derefed; + } + } + return this.#refCount; + } + + decRef() { + this.#refCount--; + if (this.#refCount === 0) { + this.#strong = null; + } + return this.#refCount; + } + + get() { + return this.#weak.deref(); + } +} + module.exports = { getLazy, assertCrypto, @@ -855,4 +888,5 @@ module.exports = { kEnumerableProperty, setOwnProperty, pendingDeprecate, + WeakReference, }; diff --git a/test/fixtures/snapshot/weak-reference-gc.js b/test/fixtures/snapshot/weak-reference-gc.js index 8dada530e77c..b6af6c46e382 100644 --- a/test/fixtures/snapshot/weak-reference-gc.js +++ b/test/fixtures/snapshot/weak-reference-gc.js @@ -1,7 +1,6 @@ 'use strict'; -const { internalBinding } = require('internal/test/binding'); -const { WeakReference } = internalBinding('util'); +const { WeakReference } = require('internal/util'); const { setDeserializeMainFunction } = require('v8').startupSnapshot diff --git a/test/fixtures/snapshot/weak-reference.js b/test/fixtures/snapshot/weak-reference.js index 214d52fee185..1aefc6a1c071 100644 --- a/test/fixtures/snapshot/weak-reference.js +++ b/test/fixtures/snapshot/weak-reference.js @@ -1,7 +1,6 @@ 'use strict'; -const { internalBinding } = require('internal/test/binding'); -const { WeakReference } = internalBinding('util'); +const { WeakReference } = require('internal/util'); const { setDeserializeMainFunction } = require('v8').startupSnapshot diff --git a/test/parallel/test-internal-util-weakreference.js b/test/parallel/test-internal-util-weakreference.js index 75a00176bb09..ef3c0943b1f8 100644 --- a/test/parallel/test-internal-util-weakreference.js +++ b/test/parallel/test-internal-util-weakreference.js @@ -2,8 +2,7 @@ 'use strict'; const common = require('../common'); const assert = require('assert'); -const { internalBinding } = require('internal/test/binding'); -const { WeakReference } = internalBinding('util'); +const { WeakReference } = require('internal/util'); let obj = { hello: 'world' }; const ref = new WeakReference(obj); From c859da0aeb8f0afa13643d580b446e9447310c7f Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 7 Aug 2023 17:04:56 +0200 Subject: [PATCH 3/3] src: remove C++ WeakReference implementation --- node.gyp | 1 - src/base_object_types.h | 1 - src/inspector/node_string.cc | 1 - src/node_snapshotable.cc | 1 - src/node_util.cc | 119 ----------------------------------- src/node_util.h | 52 --------------- src/util.cc | 1 - 7 files changed, 176 deletions(-) delete mode 100644 src/node_util.h diff --git a/node.gyp b/node.gyp index 896f4c881a50..063349540086 100644 --- a/node.gyp +++ b/node.gyp @@ -253,7 +253,6 @@ 'src/node_stat_watcher.h', 'src/node_union_bytes.h', 'src/node_url.h', - 'src/node_util.h', 'src/node_version.h', 'src/node_v8.h', 'src/node_v8_platform-inl.h', diff --git a/src/base_object_types.h b/src/base_object_types.h index bb7a0e064b0b..97ae94f10aa7 100644 --- a/src/base_object_types.h +++ b/src/base_object_types.h @@ -29,7 +29,6 @@ namespace node { // SET_OBJECT_ID(), the second argument should match the C++ class // name. #define SERIALIZABLE_NON_BINDING_TYPES(V) \ - V(util_weak_reference, util::WeakReference) // Helper list of all binding data wrapper types. #define BINDING_TYPES(V) \ diff --git a/src/inspector/node_string.cc b/src/inspector/node_string.cc index 6b59cd73f974..0f780f46c8eb 100644 --- a/src/inspector/node_string.cc +++ b/src/inspector/node_string.cc @@ -1,6 +1,5 @@ #include "node_string.h" #include "node/inspector/protocol/Protocol.h" -#include "node_util.h" #include "simdutf.h" #include "util-inl.h" diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index af85ea10a941..da66bab7ea31 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -22,7 +22,6 @@ #include "node_process.h" #include "node_snapshot_builder.h" #include "node_url.h" -#include "node_util.h" #include "node_v8.h" #include "node_v8_platform-inl.h" #include "timers.h" diff --git a/src/node_util.cc b/src/node_util.cc index c10cf98372d7..9b3ba203a3a8 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -1,4 +1,3 @@ -#include "node_util.h" #include "base_object-inl.h" #include "node_errors.h" #include "node_external_reference.h" @@ -17,8 +16,6 @@ using v8::CFunction; using v8::Context; using v8::External; using v8::FunctionCallbackInfo; -using v8::FunctionTemplate; -using v8::HandleScope; using v8::IndexFilter; using v8::Integer; using v8::Isolate; @@ -201,109 +198,6 @@ void ArrayBufferViewHasBuffer(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(args[0].As()->HasBuffer()); } -WeakReference::WeakReference(Realm* realm, - Local object, - Local target) - : WeakReference(realm, object, target, 0) {} - -WeakReference::WeakReference(Realm* realm, - Local object, - Local target, - uint64_t reference_count) - : SnapshotableObject(realm, object, type_int), - reference_count_(reference_count) { - MakeWeak(); - if (!target.IsEmpty()) { - target_.Reset(realm->isolate(), target); - if (reference_count_ == 0) { - target_.SetWeak(); - } - } -} - -bool WeakReference::PrepareForSerialization(Local context, - v8::SnapshotCreator* creator) { - if (target_.IsEmpty()) { - target_index_ = 0; - return true; - } - - // Users can still hold strong references to target in addition to the - // reference that we manage here, and they could expect that the referenced - // object remains the same as long as that external strong reference - // is alive. Since we have no way to know if there is any other reference - // keeping the target alive, the best we can do to maintain consistency is to - // simply save a reference to the target in the snapshot (effectively making - // it strong) during serialization, and restore it during deserialization. - // If there's no known counted reference from our side, we'll make the - // reference here weak upon deserialization so that it can be GC'ed if users - // do not hold additional references to it. - Local target = target_.Get(context->GetIsolate()); - target_index_ = creator->AddData(context, target); - DCHECK_NE(target_index_, 0); - target_.Reset(); - return true; -} - -InternalFieldInfoBase* WeakReference::Serialize(int index) { - DCHECK_IS_SNAPSHOT_SLOT(index); - InternalFieldInfo* info = - InternalFieldInfoBase::New(type()); - info->target = target_index_; - info->reference_count = reference_count_; - return info; -} - -void WeakReference::Deserialize(Local context, - Local holder, - int index, - InternalFieldInfoBase* info) { - DCHECK_IS_SNAPSHOT_SLOT(index); - HandleScope scope(context->GetIsolate()); - - InternalFieldInfo* weak_info = reinterpret_cast(info); - Local target; - if (weak_info->target != 0) { - target = context->GetDataFromSnapshotOnce(weak_info->target) - .ToLocalChecked(); - } - new WeakReference( - Realm::GetCurrent(context), holder, target, weak_info->reference_count); -} - -void WeakReference::New(const FunctionCallbackInfo& args) { - Realm* realm = Realm::GetCurrent(args); - CHECK(args.IsConstructCall()); - CHECK(args[0]->IsObject()); - new WeakReference(realm, args.This(), args[0].As()); -} - -void WeakReference::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)); -} - -void WeakReference::IncRef(const FunctionCallbackInfo& args) { - WeakReference* weak_ref = Unwrap(args.Holder()); - weak_ref->reference_count_++; - if (weak_ref->target_.IsEmpty()) return; - if (weak_ref->reference_count_ == 1) weak_ref->target_.ClearWeak(); - args.GetReturnValue().Set( - v8::Number::New(args.GetIsolate(), weak_ref->reference_count_)); -} - -void WeakReference::DecRef(const FunctionCallbackInfo& args) { - WeakReference* weak_ref = Unwrap(args.Holder()); - CHECK_GE(weak_ref->reference_count_, 1); - weak_ref->reference_count_--; - if (weak_ref->target_.IsEmpty()) return; - if (weak_ref->reference_count_ == 0) weak_ref->target_.SetWeak(); - args.GetReturnValue().Set( - v8::Number::New(args.GetIsolate(), weak_ref->reference_count_)); -} - static uint32_t GetUVHandleTypeCode(const uv_handle_type type) { // TODO(anonrig): We can use an enum here and then create the array in the // binding, which will remove the hard-coding in C++ and JS land. @@ -391,10 +285,6 @@ void RegisterExternalReferences(ExternalReferenceRegistry* registry) { registry->Register(GetExternalValue); registry->Register(Sleep); registry->Register(ArrayBufferViewHasBuffer); - registry->Register(WeakReference::New); - registry->Register(WeakReference::Get); - registry->Register(WeakReference::IncRef); - registry->Register(WeakReference::DecRef); registry->Register(GuessHandleType); registry->Register(FastGuessHandleType); registry->Register(fast_guess_handle_type_.GetTypeInfo()); @@ -508,15 +398,6 @@ void Initialize(Local target, env->should_abort_on_uncaught_toggle().GetJSArray()) .FromJust()); - Local weak_ref = - NewFunctionTemplate(isolate, WeakReference::New); - weak_ref->InstanceTemplate()->SetInternalFieldCount( - WeakReference::kInternalFieldCount); - SetProtoMethod(isolate, weak_ref, "get", WeakReference::Get); - SetProtoMethod(isolate, weak_ref, "incRef", WeakReference::IncRef); - SetProtoMethod(isolate, weak_ref, "decRef", WeakReference::DecRef); - SetConstructorFunction(context, target, "WeakReference", weak_ref); - SetFastMethodNoSideEffect(context, target, "guessHandleType", diff --git a/src/node_util.h b/src/node_util.h deleted file mode 100644 index 715686856db8..000000000000 --- a/src/node_util.h +++ /dev/null @@ -1,52 +0,0 @@ - -#ifndef SRC_NODE_UTIL_H_ -#define SRC_NODE_UTIL_H_ - -#if defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS -#include "base_object.h" -#include "node_snapshotable.h" -#include "v8.h" - -namespace node { -namespace util { - -class WeakReference : public SnapshotableObject { - public: - SERIALIZABLE_OBJECT_METHODS() - - SET_OBJECT_ID(util_weak_reference) - - WeakReference(Realm* realm, - v8::Local object, - v8::Local target); - static void New(const v8::FunctionCallbackInfo& args); - static void Get(const v8::FunctionCallbackInfo& args); - static void IncRef(const v8::FunctionCallbackInfo& args); - static void DecRef(const v8::FunctionCallbackInfo& args); - - SET_MEMORY_INFO_NAME(WeakReference) - SET_SELF_SIZE(WeakReference) - SET_NO_MEMORY_INFO() - - struct InternalFieldInfo : public node::InternalFieldInfoBase { - SnapshotIndex target; - uint64_t reference_count; - }; - - private: - WeakReference(Realm* realm, - v8::Local object, - v8::Local target, - uint64_t reference_count); - v8::Global target_; - uint64_t reference_count_ = 0; - - SnapshotIndex target_index_ = 0; // 0 means target_ is not snapshotted -}; - -} // namespace util -} // namespace node - -#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS - -#endif // SRC_NODE_UTIL_H_ diff --git a/src/util.cc b/src/util.cc index 8140c177490c..76a61aef5926 100644 --- a/src/util.cc +++ b/src/util.cc @@ -27,7 +27,6 @@ #include "node_buffer.h" #include "node_errors.h" #include "node_internals.h" -#include "node_util.h" #include "node_v8_platform-inl.h" #include "string_bytes.h" #include "uv.h"