From 86f7b923f507d4a2574baa40b06f45fd070f2a12 Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Tue, 26 Nov 2024 13:05:11 -0300 Subject: [PATCH 1/5] lib: settle signals when controller's signal is GCed --- lib/internal/abort_controller.js | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index b812f588c23e99..4da7ecc7b2fe11 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -95,8 +95,25 @@ const dependantSignalsCleanupRegistry = new SafeFinalizationRegistry((signalWeak } }); }); + const gcPersistentSignals = new SafeSet(); +const finalizer = new SafeFinalizationRegistry(({ sourceSignalRef, composedSignalRef }) => { + // TODO: remove ref from source signal + // TODO: remove composed signal from gcPersistentSignals + const composedSignal = composedSignalRef.deref(); + if (composedSignal !== undefined) { + composedSignal[kSourceSignals].delete(sourceSignalRef); + gcPersistentSignals.delete(composedSignal); + } + + // TODO: remove ref from dependant signal + const sourceSignal = sourceSignalRef.deref(); + if (sourceSignal !== undefined) { + sourceSignal[kDependantSignals].delete(composedSignalRef); + } +}); + const kAborted = Symbol('kAborted'); const kReason = Symbol('kReason'); const kCloneData = Symbol('kCloneData'); @@ -258,6 +275,9 @@ class AbortSignal extends EventTarget { resultSignal[kSourceSignals].add(signalWeakRef); signal[kDependantSignals].add(resultSignalWeakRef); dependantSignalsCleanupRegistry.register(resultSignal, signalWeakRef); + // when the source signal - coming from the controller - is gced, we need to remove it from the dependant + // signals of the composite signal + finalizer.register(signal, { sourceSignalRef: signalWeakRef, composedSignalRef: resultSignalWeakRef}); } else if (!signal[kSourceSignals]) { continue; } else { @@ -293,6 +313,7 @@ class AbortSignal extends EventTarget { // listener, then we don't want it to be gc'd while the listener // is attached and the timer still hasn't fired. So, we retain a // strong ref that is held for as long as the listener is registered. + gcPersistentSignals.add(this); } } @@ -434,6 +455,7 @@ class AbortController { */ get signal() { this.#signal ??= new AbortSignal(kDontThrowSymbol); + return this.#signal; } From 643efe16723422502c97b7ce9a0b56920d581814 Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Tue, 26 Nov 2024 13:57:18 -0300 Subject: [PATCH 2/5] test: test clean up settled signals --- lib/internal/abort_controller.js | 9 ++-- .../test-abortsignal-drop-settled-signals.mjs | 46 +++++++++++++++++++ 2 files changed, 51 insertions(+), 4 deletions(-) diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index 4da7ecc7b2fe11..2514a8d0932aa2 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -99,15 +99,16 @@ const dependantSignalsCleanupRegistry = new SafeFinalizationRegistry((signalWeak const gcPersistentSignals = new SafeSet(); const finalizer = new SafeFinalizationRegistry(({ sourceSignalRef, composedSignalRef }) => { - // TODO: remove ref from source signal - // TODO: remove composed signal from gcPersistentSignals const composedSignal = composedSignalRef.deref(); if (composedSignal !== undefined) { composedSignal[kSourceSignals].delete(sourceSignalRef); - gcPersistentSignals.delete(composedSignal); + + if (composedSignal[kSourceSignals].size === 0) { + // This signal will no longer abort. There's no need to keep it in the gcPersistentSignals set. + gcPersistentSignals.delete(composedSignal); + } } - // TODO: remove ref from dependant signal const sourceSignal = sourceSignalRef.deref(); if (sourceSignal !== undefined) { sourceSignal[kDependantSignals].delete(composedSignalRef); diff --git a/test/parallel/test-abortsignal-drop-settled-signals.mjs b/test/parallel/test-abortsignal-drop-settled-signals.mjs index 728002b51d30d5..c4d8fedaa503ad 100644 --- a/test/parallel/test-abortsignal-drop-settled-signals.mjs +++ b/test/parallel/test-abortsignal-drop-settled-signals.mjs @@ -64,6 +64,38 @@ function runShortLivedSourceSignal(limit, done) { run(1); }; +function runWithOrphanListeners(limit, done) { + let composedSignalRef; + const composedSignalRefs = []; + const handler = () => { }; + + function run(iteration) { + const ac = new AbortController(); + if (iteration > limit) { + setImmediate(() => { + global.gc(); + setImmediate(() => { + global.gc(); + + done(composedSignalRefs); + }); + }); + return; + } + + composedSignalRef = new WeakRef(AbortSignal.any([ac.signal])); + composedSignalRef.deref().addEventListener('abort', handler); + + composedSignalRefs.push(composedSignalRef); + + setImmediate(() => { + run(iteration + 1); + }); + } + + run(1); +} + const limit = 10_000; describe('when there is a long-lived signal', () => { @@ -120,3 +152,17 @@ it('drops settled dependant signals when signal is composite', (t, done) => { }); }); }); + +it('drops settled signals even when there are listeners', (t, done) => { + runWithOrphanListeners(limit, (signalRefs) => { + setImmediate(() => { + global.gc(); + + const unGCedSignals = [...signalRefs].filter((ref) => ref.deref()); + + t.assert.strictEqual(unGCedSignals.length, 0); + + done(); + }); + }); +}); From e1522128218b0995b15b3169484715a4d12d636e Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Tue, 26 Nov 2024 14:03:29 -0300 Subject: [PATCH 3/5] fixup: remove unneeded comments --- lib/internal/abort_controller.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index 2514a8d0932aa2..5d114edf9ecb9e 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -276,9 +276,7 @@ class AbortSignal extends EventTarget { resultSignal[kSourceSignals].add(signalWeakRef); signal[kDependantSignals].add(resultSignalWeakRef); dependantSignalsCleanupRegistry.register(resultSignal, signalWeakRef); - // when the source signal - coming from the controller - is gced, we need to remove it from the dependant - // signals of the composite signal - finalizer.register(signal, { sourceSignalRef: signalWeakRef, composedSignalRef: resultSignalWeakRef}); + finalizer.register(signal, { sourceSignalRef: signalWeakRef, composedSignalRef: resultSignalWeakRef }); } else if (!signal[kSourceSignals]) { continue; } else { From 9d3074e5faf27b822fddd1dd9a6e8500ab481ec8 Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Tue, 26 Nov 2024 15:25:36 -0300 Subject: [PATCH 4/5] chore: remove unused statements --- lib/internal/abort_controller.js | 6 ------ 1 file changed, 6 deletions(-) diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index 5d114edf9ecb9e..515845f85d7d12 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -108,11 +108,6 @@ const finalizer = new SafeFinalizationRegistry(({ sourceSignalRef, composedSigna gcPersistentSignals.delete(composedSignal); } } - - const sourceSignal = sourceSignalRef.deref(); - if (sourceSignal !== undefined) { - sourceSignal[kDependantSignals].delete(composedSignalRef); - } }); const kAborted = Symbol('kAborted'); @@ -312,7 +307,6 @@ class AbortSignal extends EventTarget { // listener, then we don't want it to be gc'd while the listener // is attached and the timer still hasn't fired. So, we retain a // strong ref that is held for as long as the listener is registered. - gcPersistentSignals.add(this); } } From c11785e76d5841c1b1ebad90b7d53ca6b1ddd01c Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Thu, 28 Nov 2024 15:15:40 -0300 Subject: [PATCH 5/5] lib: handle composed signals finalization --- lib/internal/abort_controller.js | 11 +++++++++-- .../test-abortsignal-drop-settled-signals.mjs | 17 +++++++++++++---- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index 515845f85d7d12..27a1294320e8a0 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -98,7 +98,7 @@ const dependantSignalsCleanupRegistry = new SafeFinalizationRegistry((signalWeak const gcPersistentSignals = new SafeSet(); -const finalizer = new SafeFinalizationRegistry(({ sourceSignalRef, composedSignalRef }) => { +const sourceSignalsCleanupRegistry = new SafeFinalizationRegistry(({ sourceSignalRef, composedSignalRef }) => { const composedSignal = composedSignalRef.deref(); if (composedSignal !== undefined) { composedSignal[kSourceSignals].delete(sourceSignalRef); @@ -271,7 +271,10 @@ class AbortSignal extends EventTarget { resultSignal[kSourceSignals].add(signalWeakRef); signal[kDependantSignals].add(resultSignalWeakRef); dependantSignalsCleanupRegistry.register(resultSignal, signalWeakRef); - finalizer.register(signal, { sourceSignalRef: signalWeakRef, composedSignalRef: resultSignalWeakRef }); + sourceSignalsCleanupRegistry.register(signal, { + sourceSignalRef: signalWeakRef, + composedSignalRef: resultSignalWeakRef, + }); } else if (!signal[kSourceSignals]) { continue; } else { @@ -289,6 +292,10 @@ class AbortSignal extends EventTarget { resultSignal[kSourceSignals].add(sourceSignalWeakRef); sourceSignal[kDependantSignals].add(resultSignalWeakRef); dependantSignalsCleanupRegistry.register(resultSignal, sourceSignalWeakRef); + sourceSignalsCleanupRegistry.register(signal, { + sourceSignalRef: sourceSignalWeakRef, + composedSignalRef: resultSignalWeakRef, + }); } } } diff --git a/test/parallel/test-abortsignal-drop-settled-signals.mjs b/test/parallel/test-abortsignal-drop-settled-signals.mjs index c4d8fedaa503ad..f829fb0a9173fa 100644 --- a/test/parallel/test-abortsignal-drop-settled-signals.mjs +++ b/test/parallel/test-abortsignal-drop-settled-signals.mjs @@ -86,7 +86,10 @@ function runWithOrphanListeners(limit, done) { composedSignalRef = new WeakRef(AbortSignal.any([ac.signal])); composedSignalRef.deref().addEventListener('abort', handler); - composedSignalRefs.push(composedSignalRef); + const otherComposedSignalRef = new WeakRef(AbortSignal.any([composedSignalRef.deref()])); + otherComposedSignalRef.deref().addEventListener('abort', handler); + + composedSignalRefs.push(composedSignalRef, otherComposedSignalRef); setImmediate(() => { run(iteration + 1); @@ -157,12 +160,18 @@ it('drops settled signals even when there are listeners', (t, done) => { runWithOrphanListeners(limit, (signalRefs) => { setImmediate(() => { global.gc(); + setImmediate(() => { + global.gc(); // One more call needed to clean up the deeper composed signals + setImmediate(() => { + global.gc(); // One more call needed to clean up the deeper composed signals - const unGCedSignals = [...signalRefs].filter((ref) => ref.deref()); + const unGCedSignals = [...signalRefs].filter((ref) => ref.deref()); - t.assert.strictEqual(unGCedSignals.length, 0); + t.assert.strictEqual(unGCedSignals.length, 0); - done(); + done(); + }); + }); }); }); });