diff --git a/lib/internal/abort_controller.js b/lib/internal/abort_controller.js index a24b5b556e1a5e..717c31f4f7a624 100644 --- a/lib/internal/abort_controller.js +++ b/lib/internal/abort_controller.js @@ -387,24 +387,23 @@ class AbortSignal extends EventTarget { followCompositeSignal(this); } - const isTimeoutOrNonEmptyCompositeSignal = this[kTimeout] || (this[kComposite] && this[kSourceSignals]?.size); - if (isTimeoutOrNonEmptyCompositeSignal && - type === 'abort' && + if (type === 'abort' && !this.aborted && !weak && size === 1) { - // If this is a timeout signal, or a non-empty composite signal, and we're adding a non-weak abort - // 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. + // Per the DOM spec's "AbortSignal garbage collection" section, an + // AbortSignal with registered event listeners must not be garbage + // collected while those listeners are attached. Retain a strong ref + // for as long as at least one non-weak `abort` listener is registered. + // This also covers timeout and non-empty composite signals, which were + // previously the only signals retained here. gcPersistentSignals.add(this); } } [kRemoveListener](size, type, listener, capture) { super[kRemoveListener](size, type, listener, capture); - const isTimeoutOrNonEmptyCompositeSignal = this[kTimeout] || (this[kComposite] && this[kSourceSignals]?.size); - if (isTimeoutOrNonEmptyCompositeSignal && type === 'abort' && size === 0) { + if (type === 'abort' && size === 0) { gcPersistentSignals.delete(this); } } diff --git a/test/parallel/test-abortsignal-gc-with-listener.js b/test/parallel/test-abortsignal-gc-with-listener.js new file mode 100644 index 00000000000000..70f44914f09c7a --- /dev/null +++ b/test/parallel/test-abortsignal-gc-with-listener.js @@ -0,0 +1,45 @@ +// Flags: --no-warnings --expose-gc --expose-internals +'use strict'; +require('../common'); + +const assert = require('assert'); + +const { + test, +} = require('node:test'); + +const { setTimeout: sleep } = require('timers/promises'); + +// Regression test for https://github.com/nodejs/node/issues/55428 +// When a signal "follows" another signal (a listener on the source signal +// aborts a dependent signal), the dependent signal must remain alive while the +// relationship exists, per the DOM spec's "AbortSignal garbage collection" +// section. This mirrors what `fetch`/`Request` does: the request's signal +// follows the user-provided signal, and it must keep firing its own listeners +// even after the `Request` object has been garbage collected. + +test('a following AbortSignal must survive gc', async () => { + const parent = new AbortController(); + let fired = false; + + { + const dependent = new AbortController(); + // The dependent signal "follows" parent: when parent aborts, dependent + // aborts too. This is the same pattern used by `Request`. + parent.signal.addEventListener('abort', () => dependent.abort(), { once: true }); + dependent.signal.addEventListener('abort', () => { fired = true; }); + // `dependent` goes out of scope here; only `parent` is reachable. + } + + await sleep(10); + globalThis.gc(); + + // Aborting the parent must still propagate to, and fire listeners on, the + // dependent signal even though the dependent controller is no longer + // referenced from JS. + parent.abort(); + + await sleep(10); + assert.strictEqual(fired, true, + 'listener on the following signal should fire after gc of its controller'); +});