src: re-implement the C++ WeakReference class with JS WeakRef

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.

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

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,
 };

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',

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)                                                       \

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"
 

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"

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<Value>& args) {
   args.GetReturnValue().Set(args[0].As<ArrayBufferView>()->HasBuffer());
 }
 
-WeakReference::WeakReference(Realm* realm,
-                             Local<Object> object,
-                             Local<Object> target)
-    : WeakReference(realm, object, target, 0) {}
-
-WeakReference::WeakReference(Realm* realm,
-                             Local<Object> object,
-                             Local<Object> 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> 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<Object> 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<InternalFieldInfo>(type());
-  info->target = target_index_;
-  info->reference_count = reference_count_;
-  return info;
-}
-
-void WeakReference::Deserialize(Local<Context> context,
-                                Local<Object> holder,
-                                int index,
-                                InternalFieldInfoBase* info) {
-  DCHECK_IS_SNAPSHOT_SLOT(index);
-  HandleScope scope(context->GetIsolate());
-
-  InternalFieldInfo* weak_info = reinterpret_cast<InternalFieldInfo*>(info);
-  Local<Object> target;
-  if (weak_info->target != 0) {
-    target = context->GetDataFromSnapshotOnce<Object>(weak_info->target)
-                 .ToLocalChecked();
-  }
-  new WeakReference(
-      Realm::GetCurrent(context), holder, target, weak_info->reference_count);
-}
-
-void WeakReference::New(const FunctionCallbackInfo<Value>& args) {
-  Realm* realm = Realm::GetCurrent(args);
-  CHECK(args.IsConstructCall());
-  CHECK(args[0]->IsObject());
-  new WeakReference(realm, args.This(), args[0].As<Object>());
-}
-
-void WeakReference::Get(const FunctionCallbackInfo<Value>& args) {
-  WeakReference* weak_ref = Unwrap<WeakReference>(args.Holder());
-  Isolate* isolate = args.GetIsolate();
-  if (!weak_ref->target_.IsEmpty())
-    args.GetReturnValue().Set(weak_ref->target_.Get(isolate));
-}
-
-void WeakReference::IncRef(const FunctionCallbackInfo<Value>& args) {
-  WeakReference* weak_ref = Unwrap<WeakReference>(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<Value>& args) {
-  WeakReference* weak_ref = Unwrap<WeakReference>(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<Object> target,
                   env->should_abort_on_uncaught_toggle().GetJSArray())
             .FromJust());
 
-  Local<FunctionTemplate> 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",

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"

test/fixtures/snapshot/weak-reference-gc.js

@@ -1,20 +1,30 @@
 'use strict';
 
-const { internalBinding } = require('internal/test/binding');
-const { WeakReference } = internalBinding('util');
+const { WeakReference } = require('internal/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();
 });

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

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();