From 00dc205e64217835cf786163f5fe7aeacd139511 Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Thu, 10 Oct 2024 17:21:35 -0300 Subject: [PATCH 01/12] lib: remove settled dependant signals when they are GCed --- lib/internal/abort_controller.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index 48ac99a14f33aa..b5dba958256459 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -84,6 +84,13 @@ function lazyMessageChannel() { } const clearTimeoutRegistry = new SafeFinalizationRegistry(clearTimeout); +const finalizers = new SafeFinalizationRegistry((signal) => { + signal[kDependantSignals].forEach((ref) => { + if (!ref.deref()) { + signal[kDependantSignals].delete(ref); + } + }); +}); const gcPersistentSignals = new SafeSet(); const kAborted = Symbol('kAborted'); @@ -235,13 +242,17 @@ class AbortSignal extends EventTarget { } const resultSignalWeakRef = new SafeWeakRef(resultSignal); resultSignal[kSourceSignals] = new SafeSet(); + + for (let i = 0; i < signalsArray.length; i++) { const signal = signalsArray[i]; + finalizers.register(resultSignal, signal); if (signal.aborted) { abortSignal(resultSignal, signal.reason); return resultSignal; } signal[kDependantSignals] ??= new SafeSet(); + if (!signal[kComposite]) { resultSignal[kSourceSignals].add(new SafeWeakRef(signal)); signal[kDependantSignals].add(resultSignalWeakRef); From 220915bbadaad8033950d170105a33a0d16e823f Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Thu, 10 Oct 2024 18:37:47 -0300 Subject: [PATCH 02/12] test: test settled signals dropping --- .../test-abortsignal-drop-settled-signals.mjs | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 test/parallel/test-abortsignal-drop-settled-signals.mjs diff --git a/test/parallel/test-abortsignal-drop-settled-signals.mjs b/test/parallel/test-abortsignal-drop-settled-signals.mjs new file mode 100644 index 00000000000000..87d972e4915772 --- /dev/null +++ b/test/parallel/test-abortsignal-drop-settled-signals.mjs @@ -0,0 +1,64 @@ +// Flags: --expose_gc + +import * as common from '../common/index.mjs'; + +if (common.isASan) { + common.skip('ASan messes with memory measurements'); +} + +import { describe, it } from 'node:test'; + +const makeSubseSequentCalls = (limit, done, holdReferences = false) => { + const ac = new AbortController(); + const x = []; + let dependantSymbol; + + let i = 0; + function run() { + if (!holdReferences) { + AbortSignal.any([ac.signal]); + } else { + x.push(AbortSignal.any([ac.signal])); + } + + if (!dependantSymbol) { + const kDependantSignals = Object.getOwnPropertySymbols(ac.signal).filter( + (s) => s.toString() === 'Symbol(kDependantSignals)' + )[0]; + dependantSymbol ??= kDependantSignals; + } + + if (++i >= limit) { + global.gc(); + done(ac.signal[dependantSymbol].size); + return; + } + setImmediate(run); + } + return run(); +}; + +const limit = 50000; + +describe('when there is a long-lived signal', () => { + describe('and dependant signals are Ced', () => { + it('drops settled signals', (t, done) => { + makeSubseSequentCalls(limit, (totalDependantSignals) => { + // We're unable to assert how many signals are dropped (since it depens on gc), but we can assert that some are. + t.assert.equal(totalDependantSignals < limit, true); + + done(); + }); + }); + }); + + describe('and dependant signals are still valid references', () => { + it('keeps all dependant signals', (t, done) => { + makeSubseSequentCalls(limit, (totalDependantSignals) => { + t.assert.equal(totalDependantSignals, limit); + + done(); + }, true); + }); + }); +}); From 85ac22a8e613ed1c96e2969bcb4d24151981f09d Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Thu, 10 Oct 2024 21:59:08 -0300 Subject: [PATCH 03/12] fixup: typo --- lib/internal/abort_controller.js | 3 -- .../test-abortsignal-drop-settled-signals.mjs | 34 ++++++++----------- 2 files changed, 15 insertions(+), 22 deletions(-) diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index b5dba958256459..f3f22502140172 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -242,8 +242,6 @@ class AbortSignal extends EventTarget { } const resultSignalWeakRef = new SafeWeakRef(resultSignal); resultSignal[kSourceSignals] = new SafeSet(); - - for (let i = 0; i < signalsArray.length; i++) { const signal = signalsArray[i]; finalizers.register(resultSignal, signal); @@ -252,7 +250,6 @@ class AbortSignal extends EventTarget { return resultSignal; } signal[kDependantSignals] ??= new SafeSet(); - if (!signal[kComposite]) { resultSignal[kSourceSignals].add(new SafeWeakRef(signal)); signal[kDependantSignals].add(resultSignalWeakRef); diff --git a/test/parallel/test-abortsignal-drop-settled-signals.mjs b/test/parallel/test-abortsignal-drop-settled-signals.mjs index 87d972e4915772..0d652815c4f59e 100644 --- a/test/parallel/test-abortsignal-drop-settled-signals.mjs +++ b/test/parallel/test-abortsignal-drop-settled-signals.mjs @@ -8,17 +8,17 @@ if (common.isASan) { import { describe, it } from 'node:test'; -const makeSubseSequentCalls = (limit, done, holdReferences = false) => { +const makeSubsequentCalls = (limit, done, holdReferences = false) => { const ac = new AbortController(); - const x = []; + const retainedSignals = []; let dependantSymbol; let i = 0; function run() { - if (!holdReferences) { - AbortSignal.any([ac.signal]); + if (holdReferences) { + retainedSignals.push(AbortSignal.any([ac.signal])); } else { - x.push(AbortSignal.any([ac.signal])); + AbortSignal.any([ac.signal]); } if (!dependantSymbol) { @@ -41,24 +41,20 @@ const makeSubseSequentCalls = (limit, done, holdReferences = false) => { const limit = 50000; describe('when there is a long-lived signal', () => { - describe('and dependant signals are Ced', () => { - it('drops settled signals', (t, done) => { - makeSubseSequentCalls(limit, (totalDependantSignals) => { - // We're unable to assert how many signals are dropped (since it depens on gc), but we can assert that some are. - t.assert.equal(totalDependantSignals < limit, true); + it('drops settled signals', (t, done) => { + makeSubsequentCalls(limit, (totalDependantSignals) => { + // We're unable to assert how many signals are dropped (since it depends on gc), but we can assert that some are. + t.assert.equal(totalDependantSignals < limit, true); - done(); - }); + done(); }); }); - describe('and dependant signals are still valid references', () => { - it('keeps all dependant signals', (t, done) => { - makeSubseSequentCalls(limit, (totalDependantSignals) => { - t.assert.equal(totalDependantSignals, limit); + it('keeps all dependant signals', (t, done) => { + makeSubsequentCalls(limit, (totalDependantSignals) => { + t.assert.equal(totalDependantSignals, limit); - done(); - }, true); - }); + done(); + }, true); }); }); From 9a7c5ce1776ddd0b486eaec1bf2a5b62530ea9ca Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Fri, 11 Oct 2024 00:04:54 -0300 Subject: [PATCH 04/12] fix: avoid holding strong reference on finalizers --- lib/internal/abort_controller.js | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index f3f22502140172..ee832bedbd32bc 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -84,7 +84,11 @@ function lazyMessageChannel() { } const clearTimeoutRegistry = new SafeFinalizationRegistry(clearTimeout); -const finalizers = new SafeFinalizationRegistry((signal) => { +const finalizers = new SafeFinalizationRegistry((signalWeakRef) => { + const signal = signalWeakRef.deref(); + if (!signal) { + return; + } signal[kDependantSignals].forEach((ref) => { if (!ref.deref()) { signal[kDependantSignals].delete(ref); @@ -244,14 +248,15 @@ class AbortSignal extends EventTarget { resultSignal[kSourceSignals] = new SafeSet(); for (let i = 0; i < signalsArray.length; i++) { const signal = signalsArray[i]; - finalizers.register(resultSignal, signal); if (signal.aborted) { abortSignal(resultSignal, signal.reason); return resultSignal; } + const signalWeakRef = new SafeWeakRef(signal); + finalizers.register(resultSignal, signalWeakRef); signal[kDependantSignals] ??= new SafeSet(); if (!signal[kComposite]) { - resultSignal[kSourceSignals].add(new SafeWeakRef(signal)); + resultSignal[kSourceSignals].add(signalWeakRef); signal[kDependantSignals].add(resultSignalWeakRef); } else if (!signal[kSourceSignals]) { continue; From 4b3ec8e50e05d0d647b203382ff53dc6edcc9f17 Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Fri, 11 Oct 2024 01:12:53 -0300 Subject: [PATCH 05/12] test: improve tests --- .../test-abortsignal-drop-settled-signals.mjs | 42 ++++++++++--------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/test/parallel/test-abortsignal-drop-settled-signals.mjs b/test/parallel/test-abortsignal-drop-settled-signals.mjs index 0d652815c4f59e..a540f737768ee0 100644 --- a/test/parallel/test-abortsignal-drop-settled-signals.mjs +++ b/test/parallel/test-abortsignal-drop-settled-signals.mjs @@ -13,8 +13,14 @@ const makeSubsequentCalls = (limit, done, holdReferences = false) => { const retainedSignals = []; let dependantSymbol; - let i = 0; - function run() { + // let i = 0; + function run(iteration) { + if (iteration > limit) { + global.gc(); + done(ac.signal, dependantSymbol); + return; + } + if (holdReferences) { retainedSignals.push(AbortSignal.any([ac.signal])); } else { @@ -25,34 +31,30 @@ const makeSubsequentCalls = (limit, done, holdReferences = false) => { const kDependantSignals = Object.getOwnPropertySymbols(ac.signal).filter( (s) => s.toString() === 'Symbol(kDependantSignals)' )[0]; - dependantSymbol ??= kDependantSignals; + dependantSymbol = kDependantSignals; } - if (++i >= limit) { - global.gc(); - done(ac.signal[dependantSymbol].size); - return; - } - setImmediate(run); + setImmediate(() => run(iteration + 1)); } - return run(); + + run(1); }; -const limit = 50000; +const limit = 10_000; describe('when there is a long-lived signal', () => { - it('drops settled signals', (t, done) => { - makeSubsequentCalls(limit, (totalDependantSignals) => { - // We're unable to assert how many signals are dropped (since it depends on gc), but we can assert that some are. - t.assert.equal(totalDependantSignals < limit, true); - - done(); + it('drops settled dependant signals', (t, done) => { + makeSubsequentCalls(limit, (signal, depandantSignalsKey) => { + setImmediate(() => { + t.assert.equal(signal[depandantSignalsKey].size, 0); + done(); + }); }); }); - it('keeps all dependant signals', (t, done) => { - makeSubsequentCalls(limit, (totalDependantSignals) => { - t.assert.equal(totalDependantSignals, limit); + it('keeps all active dependant signals', (t, done) => { + makeSubsequentCalls(limit, (signal, depandantSignalsKey) => { + t.assert.equal(signal[depandantSignalsKey].size, limit); done(); }, true); From 87ce721e10c9cda29f1b41152b870092969f24d9 Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Fri, 11 Oct 2024 10:32:19 -0300 Subject: [PATCH 06/12] lib: optmize finalizers logic --- lib/internal/abort_controller.js | 6 ++++-- test/parallel/test-abortsignal-drop-settled-signals.mjs | 5 ++--- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index ee832bedbd32bc..ffcbddaba11748 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -252,10 +252,10 @@ class AbortSignal extends EventTarget { abortSignal(resultSignal, signal.reason); return resultSignal; } - const signalWeakRef = new SafeWeakRef(signal); - finalizers.register(resultSignal, signalWeakRef); signal[kDependantSignals] ??= new SafeSet(); if (!signal[kComposite]) { + const signalWeakRef = new SafeWeakRef(signal); + finalizers.register(resultSignal, signalWeakRef); resultSignal[kSourceSignals].add(signalWeakRef); signal[kDependantSignals].add(resultSignalWeakRef); } else if (!signal[kSourceSignals]) { @@ -274,6 +274,8 @@ class AbortSignal extends EventTarget { } resultSignal[kSourceSignals].add(sourceSignal); sourceSignalRef[kDependantSignals].add(resultSignalWeakRef); + // Here, sourceSignal is a WeakRef + finalizers.register(resultSignal, sourceSignal); } } } diff --git a/test/parallel/test-abortsignal-drop-settled-signals.mjs b/test/parallel/test-abortsignal-drop-settled-signals.mjs index a540f737768ee0..dbcc8d1cff8887 100644 --- a/test/parallel/test-abortsignal-drop-settled-signals.mjs +++ b/test/parallel/test-abortsignal-drop-settled-signals.mjs @@ -13,7 +13,6 @@ const makeSubsequentCalls = (limit, done, holdReferences = false) => { const retainedSignals = []; let dependantSymbol; - // let i = 0; function run(iteration) { if (iteration > limit) { global.gc(); @@ -28,9 +27,9 @@ const makeSubsequentCalls = (limit, done, holdReferences = false) => { } if (!dependantSymbol) { - const kDependantSignals = Object.getOwnPropertySymbols(ac.signal).filter( + const kDependantSignals = Object.getOwnPropertySymbols(ac.signal).find( (s) => s.toString() === 'Symbol(kDependantSignals)' - )[0]; + ); dependantSymbol = kDependantSignals; } From 3fb376583e42ee6193f4703057d0b8538f8ec9f0 Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Fri, 11 Oct 2024 10:57:37 -0300 Subject: [PATCH 07/12] test: ensure short-lived signals are GCed --- .../test-abortsignal-drop-settled-signals.mjs | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/test/parallel/test-abortsignal-drop-settled-signals.mjs b/test/parallel/test-abortsignal-drop-settled-signals.mjs index dbcc8d1cff8887..cae1434552c931 100644 --- a/test/parallel/test-abortsignal-drop-settled-signals.mjs +++ b/test/parallel/test-abortsignal-drop-settled-signals.mjs @@ -39,6 +39,26 @@ const makeSubsequentCalls = (limit, done, holdReferences = false) => { run(1); }; +const runShortLivedSourceSignal = (limit, done) => { + const signalRefs = new Set(); + + function run(iteration) { + if (iteration > limit) { + global.gc(); + done(signalRefs); + return; + } + + const ac = new AbortController(); + signalRefs.add(new WeakRef(ac.signal)); + AbortSignal.any([ac.signal]); + + setImmediate(() => run(iteration + 1)); + } + + run(1); +}; + const limit = 10_000; describe('when there is a long-lived signal', () => { @@ -59,3 +79,16 @@ describe('when there is a long-lived signal', () => { }, true); }); }); + +describe('when there is a short-lived signal', () => { + it('does not prevent source signal from being GCed', (t, done) => { + runShortLivedSourceSignal(limit, (signalRefs) => { + setImmediate(() => { + const unGCedSignals = [...signalRefs].filter((ref) => ref.deref()); + + t.assert.equal(unGCedSignals, 0); + done(); + }); + }); + }); +}); From ba52b6b3dd2b47a048282dcbbb67c979f0e31abd Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Fri, 11 Oct 2024 11:03:10 -0300 Subject: [PATCH 08/12] refactor: rename variables for improving readability --- lib/internal/abort_controller.js | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index ffcbddaba11748..0f8bfda1d3eede 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -261,21 +261,20 @@ class AbortSignal extends EventTarget { } else if (!signal[kSourceSignals]) { continue; } else { - for (const sourceSignal of signal[kSourceSignals]) { - const sourceSignalRef = sourceSignal.deref(); - if (!sourceSignalRef) { + for (const sourceSignalWeakRef of signal[kSourceSignals]) { + const sourceSignal = sourceSignalWeakRef.deref(); + if (!sourceSignal) { continue; } - assert(!sourceSignalRef.aborted); - assert(!sourceSignalRef[kComposite]); + assert(!sourceSignal.aborted); + assert(!sourceSignal[kComposite]); - if (resultSignal[kSourceSignals].has(sourceSignal)) { + if (resultSignal[kSourceSignals].has(sourceSignalWeakRef)) { continue; } - resultSignal[kSourceSignals].add(sourceSignal); - sourceSignalRef[kDependantSignals].add(resultSignalWeakRef); - // Here, sourceSignal is a WeakRef - finalizers.register(resultSignal, sourceSignal); + resultSignal[kSourceSignals].add(sourceSignalWeakRef); + sourceSignal[kDependantSignals].add(resultSignalWeakRef); + finalizers.register(resultSignal, sourceSignalWeakRef); } } } From 5f0939fcc6cd3632281356da9a52b7409e2dba7a Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Fri, 11 Oct 2024 12:12:36 -0300 Subject: [PATCH 09/12] test: improve signal dropping test --- .../test-abortsignal-drop-settled-signals.mjs | 49 +++++++++++++++++-- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/test/parallel/test-abortsignal-drop-settled-signals.mjs b/test/parallel/test-abortsignal-drop-settled-signals.mjs index cae1434552c931..bce4afebb148ba 100644 --- a/test/parallel/test-abortsignal-drop-settled-signals.mjs +++ b/test/parallel/test-abortsignal-drop-settled-signals.mjs @@ -9,21 +9,29 @@ if (common.isASan) { import { describe, it } from 'node:test'; const makeSubsequentCalls = (limit, done, holdReferences = false) => { + let dependantSymbol; + let signalRef; const ac = new AbortController(); const retainedSignals = []; - let dependantSymbol; + const handler = () => {}; function run(iteration) { if (iteration > limit) { - global.gc(); - done(ac.signal, dependantSymbol); + // This setImmediate is necessary to ensure that in the last iteration the remaining signal is GCed (if not + // retained) + setImmediate(() => { + global.gc(); + done(ac.signal, dependantSymbol); + }); return; } if (holdReferences) { retainedSignals.push(AbortSignal.any([ac.signal])); } else { - AbortSignal.any([ac.signal]); + // Using a WeakRef to avoid retaining information that will interfere with the test + signalRef = new WeakRef(AbortSignal.any([ac.signal])); + signalRef.deref().addEventListener('abort', handler); } if (!dependantSymbol) { @@ -33,7 +41,12 @@ const makeSubsequentCalls = (limit, done, holdReferences = false) => { dependantSymbol = kDependantSignals; } - setImmediate(() => run(iteration + 1)); + setImmediate(() => { + // Removing the event listener at some moment in the future + // Which will then allow the signal to be GCed + signalRef?.deref()?.removeEventListener('abort', handler); + run(iteration + 1); + }); } run(1); @@ -92,3 +105,29 @@ describe('when there is a short-lived signal', () => { }); }); }); + +// describe('when provided signal is composed', () => { +// it('drops settled dependant signals', (t, done) => { +// const controllers = Array.from({ length: 2 }, () => new AbortController()); +// const composedSignal1 = AbortSignal.any([controllers[0].signal]); +// const composedSignalRef = new WeakRef(AbortSignal.any([composedSignal1, controllers[1].signal])); + +// global.gc(); + +// setImmediate(() => { +// // Object.getOwnPropertySymbols(composedSignal1).forEach((s) => { +// // console.log(s, composedSignal1[s]); +// // }); + +// // console.log('signal 2 ====') + +// const composedSignal2 = composedSignalRef.deref(); + +// Object.getOwnPropertySymbols(composedSignal2).forEach((s) => { +// console.log(s, composedSignal2[s]); +// }); +// done(); +// }); + +// }); +// }); From d3cc655e1f4ba5fa548f95c22532d83621f369a1 Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Fri, 11 Oct 2024 14:21:22 -0300 Subject: [PATCH 10/12] test: test settled signals when AbortSignal.any receives a composite signal --- .../test-abortsignal-drop-settled-signals.mjs | 41 +++++++++---------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/test/parallel/test-abortsignal-drop-settled-signals.mjs b/test/parallel/test-abortsignal-drop-settled-signals.mjs index bce4afebb148ba..b286a6fdc721a0 100644 --- a/test/parallel/test-abortsignal-drop-settled-signals.mjs +++ b/test/parallel/test-abortsignal-drop-settled-signals.mjs @@ -13,7 +13,7 @@ const makeSubsequentCalls = (limit, done, holdReferences = false) => { let signalRef; const ac = new AbortController(); const retainedSignals = []; - const handler = () => {}; + const handler = () => { }; function run(iteration) { if (iteration > limit) { @@ -106,28 +106,27 @@ describe('when there is a short-lived signal', () => { }); }); -// describe('when provided signal is composed', () => { -// it('drops settled dependant signals', (t, done) => { -// const controllers = Array.from({ length: 2 }, () => new AbortController()); -// const composedSignal1 = AbortSignal.any([controllers[0].signal]); -// const composedSignalRef = new WeakRef(AbortSignal.any([composedSignal1, controllers[1].signal])); +it('drops settled dependant signals when signal is composite', (t, done) => { + const controllers = Array.from({ length: 2 }, () => new AbortController()); + const composedSignal1 = AbortSignal.any([controllers[0].signal]); + const composedSignalRef = new WeakRef(AbortSignal.any([composedSignal1, controllers[1].signal])); -// global.gc(); + const kDependantSignals = Object.getOwnPropertySymbols(controllers[0].signal).find( + (s) => s.toString() === 'Symbol(kDependantSignals)' + ); -// setImmediate(() => { -// // Object.getOwnPropertySymbols(composedSignal1).forEach((s) => { -// // console.log(s, composedSignal1[s]); -// // }); + setImmediate(() => { + global.gc(); -// // console.log('signal 2 ====') + t.assert.equal(composedSignalRef.deref(), undefined); + t.assert.equal(controllers[0].signal[kDependantSignals].size, 2); + t.assert.equal(controllers[1].signal[kDependantSignals].size, 1); -// const composedSignal2 = composedSignalRef.deref(); - -// Object.getOwnPropertySymbols(composedSignal2).forEach((s) => { -// console.log(s, composedSignal2[s]); -// }); -// done(); -// }); + setImmediate(() => { + t.assert.equal(controllers[0].signal[kDependantSignals].size, 0); + t.assert.equal(controllers[1].signal[kDependantSignals].size, 0); -// }); -// }); + done(); + }); + }); +}); From c1a1c2ce32db4632009d1d56ef50ab3cfa33cf40 Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Fri, 11 Oct 2024 14:25:54 -0300 Subject: [PATCH 11/12] refactor: remove unneeded describe --- .../test-abortsignal-drop-settled-signals.mjs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/test/parallel/test-abortsignal-drop-settled-signals.mjs b/test/parallel/test-abortsignal-drop-settled-signals.mjs index b286a6fdc721a0..c9ac36c425c8e9 100644 --- a/test/parallel/test-abortsignal-drop-settled-signals.mjs +++ b/test/parallel/test-abortsignal-drop-settled-signals.mjs @@ -93,15 +93,13 @@ describe('when there is a long-lived signal', () => { }); }); -describe('when there is a short-lived signal', () => { - it('does not prevent source signal from being GCed', (t, done) => { - runShortLivedSourceSignal(limit, (signalRefs) => { - setImmediate(() => { - const unGCedSignals = [...signalRefs].filter((ref) => ref.deref()); +it('does not prevent source signal from being GCed if it is short-lived', (t, done) => { + runShortLivedSourceSignal(limit, (signalRefs) => { + setImmediate(() => { + const unGCedSignals = [...signalRefs].filter((ref) => ref.deref()); - t.assert.equal(unGCedSignals, 0); - done(); - }); + t.assert.equal(unGCedSignals, 0); + done(); }); }); }); From c74940249bd567b4f1e1219d44d6aa0e646ca7e9 Mon Sep 17 00:00:00 2001 From: Edy Silva Date: Mon, 14 Oct 2024 11:05:12 -0300 Subject: [PATCH 12/12] chore: address pr's comments --- lib/internal/abort_controller.js | 10 ++--- .../test-abortsignal-drop-settled-signals.mjs | 38 ++++++++----------- 2 files changed, 20 insertions(+), 28 deletions(-) diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index 0f8bfda1d3eede..25293a7c2e56f7 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -84,13 +84,13 @@ function lazyMessageChannel() { } const clearTimeoutRegistry = new SafeFinalizationRegistry(clearTimeout); -const finalizers = new SafeFinalizationRegistry((signalWeakRef) => { +const dependantSignalsCleanupRegistry = new SafeFinalizationRegistry((signalWeakRef) => { const signal = signalWeakRef.deref(); - if (!signal) { + if (signal === undefined) { return; } signal[kDependantSignals].forEach((ref) => { - if (!ref.deref()) { + if (ref.deref() === undefined) { signal[kDependantSignals].delete(ref); } }); @@ -255,9 +255,9 @@ class AbortSignal extends EventTarget { signal[kDependantSignals] ??= new SafeSet(); if (!signal[kComposite]) { const signalWeakRef = new SafeWeakRef(signal); - finalizers.register(resultSignal, signalWeakRef); resultSignal[kSourceSignals].add(signalWeakRef); signal[kDependantSignals].add(resultSignalWeakRef); + dependantSignalsCleanupRegistry.register(resultSignal, signalWeakRef); } else if (!signal[kSourceSignals]) { continue; } else { @@ -274,7 +274,7 @@ class AbortSignal extends EventTarget { } resultSignal[kSourceSignals].add(sourceSignalWeakRef); sourceSignal[kDependantSignals].add(resultSignalWeakRef); - finalizers.register(resultSignal, sourceSignalWeakRef); + dependantSignalsCleanupRegistry.register(resultSignal, sourceSignalWeakRef); } } } diff --git a/test/parallel/test-abortsignal-drop-settled-signals.mjs b/test/parallel/test-abortsignal-drop-settled-signals.mjs index c9ac36c425c8e9..728002b51d30d5 100644 --- a/test/parallel/test-abortsignal-drop-settled-signals.mjs +++ b/test/parallel/test-abortsignal-drop-settled-signals.mjs @@ -1,14 +1,9 @@ // Flags: --expose_gc - -import * as common from '../common/index.mjs'; - -if (common.isASan) { - common.skip('ASan messes with memory measurements'); -} - +// +import '../common/index.mjs'; import { describe, it } from 'node:test'; -const makeSubsequentCalls = (limit, done, holdReferences = false) => { +function makeSubsequentCalls(limit, done, holdReferences = false) { let dependantSymbol; let signalRef; const ac = new AbortController(); @@ -34,12 +29,9 @@ const makeSubsequentCalls = (limit, done, holdReferences = false) => { signalRef.deref().addEventListener('abort', handler); } - if (!dependantSymbol) { - const kDependantSignals = Object.getOwnPropertySymbols(ac.signal).find( - (s) => s.toString() === 'Symbol(kDependantSignals)' - ); - dependantSymbol = kDependantSignals; - } + dependantSymbol ??= Object.getOwnPropertySymbols(ac.signal).find( + (s) => s.toString() === 'Symbol(kDependantSignals)' + ); setImmediate(() => { // Removing the event listener at some moment in the future @@ -52,7 +44,7 @@ const makeSubsequentCalls = (limit, done, holdReferences = false) => { run(1); }; -const runShortLivedSourceSignal = (limit, done) => { +function runShortLivedSourceSignal(limit, done) { const signalRefs = new Set(); function run(iteration) { @@ -78,7 +70,7 @@ describe('when there is a long-lived signal', () => { it('drops settled dependant signals', (t, done) => { makeSubsequentCalls(limit, (signal, depandantSignalsKey) => { setImmediate(() => { - t.assert.equal(signal[depandantSignalsKey].size, 0); + t.assert.strictEqual(signal[depandantSignalsKey].size, 0); done(); }); }); @@ -86,7 +78,7 @@ describe('when there is a long-lived signal', () => { it('keeps all active dependant signals', (t, done) => { makeSubsequentCalls(limit, (signal, depandantSignalsKey) => { - t.assert.equal(signal[depandantSignalsKey].size, limit); + t.assert.strictEqual(signal[depandantSignalsKey].size, limit); done(); }, true); @@ -98,7 +90,7 @@ it('does not prevent source signal from being GCed if it is short-lived', (t, do setImmediate(() => { const unGCedSignals = [...signalRefs].filter((ref) => ref.deref()); - t.assert.equal(unGCedSignals, 0); + t.assert.strictEqual(unGCedSignals.length, 0); done(); }); }); @@ -116,13 +108,13 @@ it('drops settled dependant signals when signal is composite', (t, done) => { setImmediate(() => { global.gc(); - t.assert.equal(composedSignalRef.deref(), undefined); - t.assert.equal(controllers[0].signal[kDependantSignals].size, 2); - t.assert.equal(controllers[1].signal[kDependantSignals].size, 1); + t.assert.strictEqual(composedSignalRef.deref(), undefined); + t.assert.strictEqual(controllers[0].signal[kDependantSignals].size, 2); + t.assert.strictEqual(controllers[1].signal[kDependantSignals].size, 1); setImmediate(() => { - t.assert.equal(controllers[0].signal[kDependantSignals].size, 0); - t.assert.equal(controllers[1].signal[kDependantSignals].size, 0); + t.assert.strictEqual(controllers[0].signal[kDependantSignals].size, 0); + t.assert.strictEqual(controllers[1].signal[kDependantSignals].size, 0); done(); });