Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit d065e99

Browse filesBrowse files
Han5991aduh95
Han5991
authored and
aduh95
committed
lib: defer AbortSignal.any() following
Avoid registering AbortSignal.any() composites as dependants until they are actually observed. This fixes the long-lived source retention pattern from #62363 while preserving abort semantics through lazy refresh and follow paths. Also unregister fired timeout signals from the timeout finalization registry so timeout churn releases memory more promptly. PR-URL: #62367 Fixes: #62363 Refs: #54614 Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
1 parent 899d780 commit d065e99
Copy full SHA for d065e99

2 files changed

+116-28Lines changed: 116 additions & 28 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎lib/internal/abort_controller.js‎

Copy file name to clipboardExpand all lines: lib/internal/abort_controller.js
+86-27Lines changed: 86 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -85,17 +85,16 @@ function lazyMessageChannel() {
8585
}
8686

8787
const clearTimeoutRegistry = new SafeFinalizationRegistry(clearTimeout);
88-
const dependantSignalsCleanupRegistry = new SafeFinalizationRegistry((signalWeakRef) => {
89-
const signal = signalWeakRef.deref();
90-
if (signal === undefined) {
91-
return;
92-
}
93-
signal[kDependantSignals].forEach((ref) => {
94-
if (ref.deref() === undefined) {
95-
signal[kDependantSignals].delete(ref);
88+
const dependantSignalsCleanupRegistry = new SafeFinalizationRegistry(
89+
({ sourceSignalRef, dependantSignalRef, sourceSignalsCleanupToken }) => {
90+
sourceSignalsCleanupRegistry.unregister(sourceSignalsCleanupToken);
91+
92+
const sourceSignal = sourceSignalRef.deref();
93+
if (sourceSignal === undefined) {
94+
return;
9695
}
96+
sourceSignal[kDependantSignals].delete(dependantSignalRef);
9797
});
98-
});
9998

10099
const gcPersistentSignals = new SafeSet();
101100

@@ -117,6 +116,8 @@ const kCloneData = Symbol('kCloneData');
117116
const kTimeout = Symbol('kTimeout');
118117
const kMakeTransferable = Symbol('kMakeTransferable');
119118
const kComposite = Symbol('kComposite');
119+
const kFollowing = Symbol('kFollowing');
120+
const kResultSignalWeakRef = Symbol('kResultSignalWeakRef');
120121
const kSourceSignals = Symbol('kSourceSignals');
121122
const kDependantSignals = Symbol('kDependantSignals');
122123

@@ -136,6 +137,60 @@ function validateThisAbortSignal(obj) {
136137
throw new ERR_INVALID_THIS('AbortSignal');
137138
}
138139

140+
function refreshCompositeSignal(signal) {
141+
if (!signal[kComposite] || signal[kAborted] || !signal[kSourceSignals]?.size) {
142+
return;
143+
}
144+
145+
for (const sourceSignalWeakRef of signal[kSourceSignals]) {
146+
const sourceSignal = sourceSignalWeakRef.deref();
147+
if (sourceSignal === undefined) {
148+
signal[kSourceSignals].delete(sourceSignalWeakRef);
149+
continue;
150+
}
151+
152+
if (sourceSignal.aborted) {
153+
abortSignal(signal, sourceSignal.reason);
154+
return;
155+
}
156+
}
157+
}
158+
159+
function followCompositeSignal(signal) {
160+
if (signal[kFollowing] || signal[kAborted] || !signal[kSourceSignals]?.size) {
161+
return;
162+
}
163+
164+
const resultSignalWeakRef = signal[kResultSignalWeakRef] ??= new SafeWeakRef(signal);
165+
166+
for (const sourceSignalWeakRef of signal[kSourceSignals]) {
167+
const sourceSignal = sourceSignalWeakRef.deref();
168+
if (sourceSignal === undefined) {
169+
signal[kSourceSignals].delete(sourceSignalWeakRef);
170+
continue;
171+
}
172+
173+
if (sourceSignal.aborted) {
174+
abortSignal(signal, sourceSignal.reason);
175+
return;
176+
}
177+
178+
sourceSignal[kDependantSignals] ??= new SafeSet();
179+
sourceSignal[kDependantSignals].add(resultSignalWeakRef);
180+
dependantSignalsCleanupRegistry.register(signal, {
181+
sourceSignalRef: sourceSignalWeakRef,
182+
dependantSignalRef: resultSignalWeakRef,
183+
sourceSignalsCleanupToken: sourceSignalWeakRef,
184+
});
185+
sourceSignalsCleanupRegistry.register(sourceSignal, {
186+
sourceSignalRef: sourceSignalWeakRef,
187+
composedSignalRef: resultSignalWeakRef,
188+
}, sourceSignalWeakRef);
189+
}
190+
191+
signal[kFollowing] = true;
192+
}
193+
139194
// Because the AbortSignal timeout cannot be canceled, we don't want the
140195
// presence of the timer alone to keep the AbortSignal from being garbage
141196
// collected if it otherwise no longer accessible. We also don't want the
@@ -148,6 +203,7 @@ function setWeakAbortSignalTimeout(weakRef, delay) {
148203
const timeout = setTimeout(() => {
149204
const signal = weakRef.deref();
150205
if (signal !== undefined) {
206+
clearTimeoutRegistry.unregister(signal);
151207
gcPersistentSignals.delete(signal);
152208
abortSignal(
153209
signal,
@@ -198,6 +254,7 @@ class AbortSignal extends EventTarget {
198254
*/
199255
get aborted() {
200256
validateThisAbortSignal(this);
257+
refreshCompositeSignal(this);
201258
return !!this[kAborted];
202259
}
203260

@@ -206,11 +263,13 @@ class AbortSignal extends EventTarget {
206263
*/
207264
get reason() {
208265
validateThisAbortSignal(this);
266+
refreshCompositeSignal(this);
209267
return this[kReason];
210268
}
211269

212270
throwIfAborted() {
213271
validateThisAbortSignal(this);
272+
refreshCompositeSignal(this);
214273
if (this[kAborted]) {
215274
throw this[kReason];
216275
}
@@ -241,7 +300,8 @@ class AbortSignal extends EventTarget {
241300
signal[kTimeout] = true;
242301
clearTimeoutRegistry.register(
243302
signal,
244-
setWeakAbortSignalTimeout(new SafeWeakRef(signal), delay));
303+
setWeakAbortSignalTimeout(new SafeWeakRef(signal), delay),
304+
signal);
245305
return signal;
246306
}
247307

@@ -260,7 +320,6 @@ class AbortSignal extends EventTarget {
260320
return resultSignal;
261321
}
262322

263-
const resultSignalWeakRef = new SafeWeakRef(resultSignal);
264323
resultSignal[kSourceSignals] = new SafeSet();
265324

266325
// Track if we have any timeout signals
@@ -283,51 +342,51 @@ class AbortSignal extends EventTarget {
283342
return resultSignal;
284343
}
285344

286-
signal[kDependantSignals] ??= new SafeSet();
287345
if (!signal[kComposite]) {
288346
const signalWeakRef = new SafeWeakRef(signal);
289347
resultSignal[kSourceSignals].add(signalWeakRef);
290-
signal[kDependantSignals].add(resultSignalWeakRef);
291-
dependantSignalsCleanupRegistry.register(resultSignal, signalWeakRef);
292-
sourceSignalsCleanupRegistry.register(signal, {
293-
sourceSignalRef: signalWeakRef,
294-
composedSignalRef: resultSignalWeakRef,
295-
});
296348
} else if (!signal[kSourceSignals]) {
297349
continue;
298350
} else {
351+
refreshCompositeSignal(signal);
352+
if (signal.aborted) {
353+
abortSignal(resultSignal, signal.reason);
354+
return resultSignal;
355+
}
299356
for (const sourceSignalWeakRef of signal[kSourceSignals]) {
300357
const sourceSignal = sourceSignalWeakRef.deref();
301358
if (!sourceSignal) {
302359
continue;
303360
}
304-
assert(!sourceSignal.aborted);
305361
assert(!sourceSignal[kComposite]);
306362

363+
if (sourceSignal.aborted) {
364+
abortSignal(resultSignal, sourceSignal.reason);
365+
return resultSignal;
366+
}
367+
307368
if (resultSignal[kSourceSignals].has(sourceSignalWeakRef)) {
308369
continue;
309370
}
310371
resultSignal[kSourceSignals].add(sourceSignalWeakRef);
311-
sourceSignal[kDependantSignals].add(resultSignalWeakRef);
312-
dependantSignalsCleanupRegistry.register(resultSignal, sourceSignalWeakRef);
313-
sourceSignalsCleanupRegistry.register(signal, {
314-
sourceSignalRef: sourceSignalWeakRef,
315-
composedSignalRef: resultSignalWeakRef,
316-
});
317372
}
318373
}
319374
}
320375

321-
// If we have any timeout signals, add the composite signal to gcPersistentSignals
322376
if (hasTimeoutSignals && resultSignal[kSourceSignals].size > 0) {
323-
gcPersistentSignals.add(resultSignal);
377+
resultSignal[kTimeout] = true;
324378
}
325379

326380
return resultSignal;
327381
}
328382

329383
[kNewListener](size, type, listener, once, capture, passive, weak) {
330384
super[kNewListener](size, type, listener, once, capture, passive, weak);
385+
386+
if (this[kComposite] && type === 'abort' && !this.aborted && size === 1) {
387+
followCompositeSignal(this);
388+
}
389+
331390
const isTimeoutOrNonEmptyCompositeSignal = this[kTimeout] || (this[kComposite] && this[kSourceSignals]?.size);
332391
if (isTimeoutOrNonEmptyCompositeSignal &&
333392
type === 'abort' &&
Collapse file

‎test/parallel/test-abortsignal-drop-settled-signals.mjs‎

Copy file name to clipboardExpand all lines: test/parallel/test-abortsignal-drop-settled-signals.mjs
+30-1Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,9 @@ function makeSubsequentCalls(limit, done, holdReferences = false) {
2323
}
2424

2525
if (holdReferences) {
26-
retainedSignals.push(AbortSignal.any([ac.signal]));
26+
const signal = AbortSignal.any([ac.signal]);
27+
signal.addEventListener('abort', handler);
28+
retainedSignals.push(signal);
2729
} else {
2830
// Using a WeakRef to avoid retaining information that will interfere with the test
2931
signalRef = new WeakRef(AbortSignal.any([ac.signal]));
@@ -119,6 +121,27 @@ describe('when there is a long-lived signal', () => {
119121
done();
120122
}, true);
121123
});
124+
125+
it('does not keep retained dependent signals without listeners', (t, done) => {
126+
const ac = new AbortController();
127+
const retainedSignals = [];
128+
const kDependantSignals = Object.getOwnPropertySymbols(ac.signal).find(
129+
(s) => s.toString() === 'Symbol(kDependantSignals)'
130+
);
131+
132+
function run(iteration) {
133+
if (iteration > limit) {
134+
t.assert.strictEqual(ac.signal[kDependantSignals]?.size ?? 0, 0);
135+
done();
136+
return;
137+
}
138+
139+
retainedSignals.push(AbortSignal.any([ac.signal]));
140+
setImmediate(() => run(iteration + 1));
141+
}
142+
143+
run(1);
144+
});
122145
});
123146

124147
it('does not prevent source signal from being GCed if it is short-lived', (t, done) => {
@@ -134,10 +157,13 @@ it('does not prevent source signal from being GCed if it is short-lived', (t, do
134157

135158
it('drops settled dependent signals when signal is composite', (t, done) => {
136159
const controllers = Array.from({ length: 2 }, () => new AbortController());
160+
const handler = () => {};
137161

138162
// Using WeakRefs to avoid this test to retain information that will make the test fail
139163
const composedSignal1 = new WeakRef(AbortSignal.any([controllers[0].signal]));
140164
const composedSignalRef = new WeakRef(AbortSignal.any([composedSignal1.deref(), controllers[1].signal]));
165+
composedSignal1.deref().addEventListener('abort', handler);
166+
composedSignalRef.deref().addEventListener('abort', handler);
141167

142168
const kDependantSignals = Object.getOwnPropertySymbols(controllers[0].signal).find(
143169
(s) => s.toString() === 'Symbol(kDependantSignals)'
@@ -147,6 +173,9 @@ it('drops settled dependent signals when signal is composite', (t, done) => {
147173
t.assert.strictEqual(controllers[1].signal[kDependantSignals].size, 1);
148174

149175
setImmediate(mustCall(() => {
176+
composedSignal1.deref()?.removeEventListener('abort', handler);
177+
composedSignalRef.deref()?.removeEventListener('abort', handler);
178+
150179
globalThis.gc({ execution: 'async' }).then(async () => {
151180
await gcUntil('all signals are GCed', () => {
152181
const totalDependantSignals = Math.max(

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.