Skip to content

Commit

Permalink
diagnostics_channel: fix ref counting bug when reaching zero
Browse files Browse the repository at this point in the history
PR-URL: nodejs/node#47520
Reviewed-By: Robert Nagy <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
  • Loading branch information
sercher committed Apr 24, 2024
1 parent eacf986 commit 5b0d5cb
Show file tree
Hide file tree
Showing 2 changed files with 38 additions and 21 deletions.
52 changes: 31 additions & 21 deletions graal-nodejs/lib/diagnostics_channel.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const {
ArrayPrototypeIndexOf,
ArrayPrototypePush,
ArrayPrototypeSplice,
SafeFinalizationRegistry,
ObjectGetPrototypeOf,
ObjectSetPrototypeOf,
Promise,
Expand All @@ -28,14 +29,29 @@ const { triggerUncaughtException } = internalBinding('errors');

const { WeakReference } = internalBinding('util');

function decRef(channel) {
if (channels.get(channel.name).decRef() === 0) {
channels.delete(channel.name);
// 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.
class WeakRefMap extends SafeMap {
#finalizers = new SafeFinalizationRegistry((key) => {
this.delete(key);
});

set(key, value) {
this.#finalizers.register(value, key);
return super.set(key, new WeakReference(value));
}
}

function incRef(channel) {
channels.get(channel.name).incRef();
get(key) {
return super.get(key)?.get();
}

incRef(key) {
return super.get(key)?.incRef();
}

decRef(key) {
return super.get(key)?.decRef();
}
}

function markActive(channel) {
Expand Down Expand Up @@ -80,7 +96,7 @@ class ActiveChannel {
subscribe(subscription) {
validateFunction(subscription, 'subscription');
ArrayPrototypePush(this._subscribers, subscription);
incRef(this);
channels.incRef(this.name);
}

unsubscribe(subscription) {
Expand All @@ -89,15 +105,15 @@ class ActiveChannel {

ArrayPrototypeSplice(this._subscribers, index, 1);

decRef(this);
channels.decRef(this.name);
maybeMarkInactive(this);

return true;
}

bindStore(store, transform) {
const replacing = this._stores.has(store);
if (!replacing) incRef(this);
if (!replacing) channels.incRef(this.name);
this._stores.set(store, transform);
}

Expand All @@ -108,7 +124,7 @@ class ActiveChannel {

this._stores.delete(store);

decRef(this);
channels.decRef(this.name);
maybeMarkInactive(this);

return true;
Expand Down Expand Up @@ -153,7 +169,7 @@ class Channel {
this._stores = undefined;
this.name = name;

channels.set(name, new WeakReference(this));
channels.set(name, this);
}

static [SymbolHasInstance](instance) {
Expand Down Expand Up @@ -191,12 +207,10 @@ class Channel {
}
}

const channels = new SafeMap();
const channels = new WeakRefMap();

function channel(name) {
let channel;
const ref = channels.get(name);
if (ref) channel = ref.get();
const channel = channels.get(name);
if (channel) return channel;

if (typeof name !== 'string' && typeof name !== 'symbol') {
Expand All @@ -215,12 +229,8 @@ function unsubscribe(name, subscription) {
}

function hasSubscribers(name) {
let channel;
const ref = channels.get(name);
if (ref) channel = ref.get();
if (!channel) {
return false;
}
const channel = channels.get(name);
if (!channel) return false;

return channel.hasSubscribers;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,3 +42,10 @@ assert.ok(!dc.unsubscribe(name, subscriber));
assert.throws(() => {
dc.subscribe(name, null);
}, { code: 'ERR_INVALID_ARG_TYPE' });

// Reaching zero subscribers should not delete from the channels map as there
// will be no more weakref to incRef if another subscribe happens while the
// channel object itself exists.
channel.subscribe(subscriber);
channel.unsubscribe(subscriber);
channel.subscribe(subscriber);

0 comments on commit 5b0d5cb

Please sign in to comment.