Skip to content

Commit 157d274

Browse files
Stephen BelangerRafaelGSS
Stephen Belanger
authored andcommitted
diagnostics_channel: fix ref counting bug when reaching zero subscribers
PR-URL: #47520 Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
1 parent 947925d commit 157d274

File tree

2 files changed

+38
-21
lines changed

2 files changed

+38
-21
lines changed

lib/diagnostics_channel.js

+31-21
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ const {
55
ArrayPrototypeIndexOf,
66
ArrayPrototypePush,
77
ArrayPrototypeSplice,
8+
SafeFinalizationRegistry,
89
ObjectGetPrototypeOf,
910
ObjectSetPrototypeOf,
1011
Promise,
@@ -29,14 +30,29 @@ const { triggerUncaughtException } = internalBinding('errors');
2930

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

32-
function decRef(channel) {
33-
if (channels.get(channel.name).decRef() === 0) {
34-
channels.delete(channel.name);
33+
// Can't delete when weakref count reaches 0 as it could increment again.
34+
// Only GC can be used as a valid time to clean up the channels map.
35+
class WeakRefMap extends SafeMap {
36+
#finalizers = new SafeFinalizationRegistry((key) => {
37+
this.delete(key);
38+
});
39+
40+
set(key, value) {
41+
this.#finalizers.register(value, key);
42+
return super.set(key, new WeakReference(value));
3543
}
36-
}
3744

38-
function incRef(channel) {
39-
channels.get(channel.name).incRef();
45+
get(key) {
46+
return super.get(key)?.get();
47+
}
48+
49+
incRef(key) {
50+
return super.get(key)?.incRef();
51+
}
52+
53+
decRef(key) {
54+
return super.get(key)?.decRef();
55+
}
4056
}
4157

4258
function markActive(channel) {
@@ -81,7 +97,7 @@ class ActiveChannel {
8197
subscribe(subscription) {
8298
validateFunction(subscription, 'subscription');
8399
ArrayPrototypePush(this._subscribers, subscription);
84-
incRef(this);
100+
channels.incRef(this.name);
85101
}
86102

87103
unsubscribe(subscription) {
@@ -90,15 +106,15 @@ class ActiveChannel {
90106

91107
ArrayPrototypeSplice(this._subscribers, index, 1);
92108

93-
decRef(this);
109+
channels.decRef(this.name);
94110
maybeMarkInactive(this);
95111

96112
return true;
97113
}
98114

99115
bindStore(store, transform) {
100116
const replacing = this._stores.has(store);
101-
if (!replacing) incRef(this);
117+
if (!replacing) channels.incRef(this.name);
102118
this._stores.set(store, transform);
103119
}
104120

@@ -109,7 +125,7 @@ class ActiveChannel {
109125

110126
this._stores.delete(store);
111127

112-
decRef(this);
128+
channels.decRef(this.name);
113129
maybeMarkInactive(this);
114130

115131
return true;
@@ -154,7 +170,7 @@ class Channel {
154170
this._stores = undefined;
155171
this.name = name;
156172

157-
channels.set(name, new WeakReference(this));
173+
channels.set(name, this);
158174
}
159175

160176
static [SymbolHasInstance](instance) {
@@ -192,12 +208,10 @@ class Channel {
192208
}
193209
}
194210

195-
const channels = new SafeMap();
211+
const channels = new WeakRefMap();
196212

197213
function channel(name) {
198-
let channel;
199-
const ref = channels.get(name);
200-
if (ref) channel = ref.get();
214+
const channel = channels.get(name);
201215
if (channel) return channel;
202216

203217
if (typeof name !== 'string' && typeof name !== 'symbol') {
@@ -216,12 +230,8 @@ function unsubscribe(name, subscription) {
216230
}
217231

218232
function hasSubscribers(name) {
219-
let channel;
220-
const ref = channels.get(name);
221-
if (ref) channel = ref.get();
222-
if (!channel) {
223-
return false;
224-
}
233+
const channel = channels.get(name);
234+
if (!channel) return false;
225235

226236
return channel.hasSubscribers;
227237
}

test/parallel/test-diagnostics-channel-pub-sub.js

+7
Original file line numberDiff line numberDiff line change
@@ -42,3 +42,10 @@ assert.ok(!dc.unsubscribe(name, subscriber));
4242
assert.throws(() => {
4343
dc.subscribe(name, null);
4444
}, { code: 'ERR_INVALID_ARG_TYPE' });
45+
46+
// Reaching zero subscribers should not delete from the channels map as there
47+
// will be no more weakref to incRef if another subscribe happens while the
48+
// channel object itself exists.
49+
channel.subscribe(subscriber);
50+
channel.unsubscribe(subscriber);
51+
channel.subscribe(subscriber);

0 commit comments

Comments
 (0)