Repository: sindresorhus/got (ts)

Issue:
Resource leak when (re-)using an abort signal
#### Describe the bug

 - Node.js version: v16.17.1
 - OS & version: Debian 5.10.140-1

When re-using an abort controller for multiple got requests a significant amount of memory is retained by the `AbortSignal` through got. The main reason for this (I think) is that the `Request` instance adds an event listener for the `abort`event on the passed signal where the event handler refers back to the request. The event handler is never removed so the signal retains a reference to the `Request` instance for as long as it lives.

#### Actual behavior

The code to reproduce below downloads and discards a 1MB file in a loop. The loop exists when the abort signal is aborted via ctrl-c. The same abort signal is passed to got to cancel the in-progress download. The heap profile screenshot below shows how a large chunk of memory is retained via the callback context of an event listener coming from here:
https://github.com/sindresorhus/got/blob/main/source/core/index.ts#L254

![screenshot_20221010_221207](https://user-images.githubusercontent.com/5262541/194835921-dfcdfaff-55d6-447e-81f0-65c56264c7e8.png)

#### Expected behavior

Memory is released after the request completes.

#### Code to reproduce

```js
import got from 'got';

const abortController = new AbortController();

process.on('SIGINT', function () {
  abortController.abort();
});

async function main() {
  while (!abortController.signal.aborted) {
    await got.get(
      'https://ia902805.us.archive.org/14/items/1mbFile/1mb.mp4',
      {
        signal: abortController.signal,
      }
    ).buffer();
  }
}

main().then(() => console.log('Bye')).catch((e) => {
  if (e.constructor.name !== 'AbortError') {
    console.error(e);
  }
});
```

#### Checklist

- [*] I have read the documentation.
- [*] I have tried my code with the latest version of ~~Node.js~~ and Got.


Pull request description:
Fix resource leak by removing abort event listeners on destroy
This change removes the event listener callback that is added to the passed `AbortSignal` when the *got* instance is destroyed.

I'm not entirely sure that this fix covers all clean up points. I skipped the clean up on the redirect which I think is correct but would appreciate confirmation.

Also I'd be open to adding a test to cover this but any pointer on how to test this would be appreciated!

Fixes #2160

#### Checklist

- [*] I have read the documentation.
- [*] I have included a pull request description of my changes.
- [x] I have included some tests.


Patch:
diff --git a/source/core/index.ts b/source/core/index.ts
index 0a054fd..bb3c59a 100644
--- a/source/core/index.ts
+++ b/source/core/index.ts
@@ -176,6 +176,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 	private _triggerRead: boolean;
 	declare private _jobs: Array<() => void>;
 	private _cancelTimeouts: () => void;
+	private readonly _removeListeners: () => void;
 	private _nativeResponse?: IncomingMessageWithTimings;
 	private _flushed: boolean;
 	private _aborted: boolean;
@@ -199,6 +200,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 		this._unproxyEvents = noop;
 		this._triggerRead = false;
 		this._cancelTimeouts = noop;
+		this._removeListeners = noop;
 		this._jobs = [];
 		this._flushed = false;
 		this._requestInitialized = false;
@@ -247,14 +249,6 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 			return;
 		}
 
-		if (this.options.signal?.aborted) {
-			this.destroy(new AbortError(this));
-		}
-
-		this.options.signal?.addEventListener('abort', () => {
-			this.destroy(new AbortError(this));
-		});
-
 		// Important! If you replace `body` in a handler with another stream, make sure it's readable first.
 		// The below is run only once.
 		const {body} = this.options;
@@ -271,6 +265,21 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 				}
 			});
 		}
+
+		if (this.options.signal) {
+			const abort = () => {
+				this.destroy(new AbortError(this));
+			};
+
+			if (this.options.signal.aborted) {
+				abort();
+			} else {
+				this.options.signal.addEventListener('abort', abort);
+				this._removeListeners = () => {
+					this.options.signal.removeEventListener('abort', abort);
+				};
+			}
+		}
 	}
 
 	async flush() {
@@ -508,6 +517,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 		// Prevent further retries
 		this._stopRetry();
 		this._cancelTimeouts();
+		this._removeListeners();
 
 		if (this.options) {
 			const {body} = this.options;


Test patch:
diff --git a/test/abort.ts b/test/abort.ts
index 92d81f4..525e4e8 100644
--- a/test/abort.ts
+++ b/test/abort.ts
@@ -5,6 +5,7 @@ import test from 'ava';
 import delay from 'delay';
 import {pEvent} from 'p-event';
 import type {Handler} from 'express';
+import {createSandbox} from 'sinon';
 import got from '../source/index.js';
 import slowDataStream from './helpers/slow-data-stream.js';
 import type {GlobalClock} from './helpers/types.js';
@@ -64,9 +65,25 @@ if (globalThis.AbortController !== undefined) {
 		);
 	};
 
+	const sandbox = createSandbox();
+
+	const createAbortController = (): {controller: AbortController; signalHandlersRemoved: () => boolean} => {
+		const controller = new AbortController();
+		sandbox.spy(controller.signal);
+		// @ts-expect-error AbortSignal type definition issue: https://github.com/DefinitelyTyped/DefinitelyTyped/discussions/57805
+		const signalHandlersRemoved = () => controller.signal.addEventListener.callCount === controller.signal.removeEventListener.callCount;
+		return {
+			controller, signalHandlersRemoved,
+		};
+	};
+
+	test.afterEach(() => {
+		sandbox.restore();
+	});
+
 	test.serial('does not retry after abort', withServerAndFakeTimers, async (t, server, got, clock) => {
 		const {emitter, promise} = prepareServer(server, clock);
-		const controller = new AbortController();
+		const {controller, signalHandlersRemoved} = createAbortController();
 
 		const gotPromise = got('redirect', {
 			signal: controller.signal,
@@ -88,12 +105,14 @@ if (globalThis.AbortController !== undefined) {
 		});
 
 		await t.notThrowsAsync(promise, 'Request finished instead of aborting.');
+
+		t.true(signalHandlersRemoved(), 'Abort signal event handlers not removed');
 	});
 
 	test.serial('abort request timeouts', withServer, async (t, server, got) => {
 		server.get('/', () => {});
 
-		const controller = new AbortController();
+		const {controller, signalHandlersRemoved} = createAbortController();
 
 		const gotPromise = got({
 			signal: controller.signal,
@@ -121,6 +140,8 @@ if (globalThis.AbortController !== undefined) {
 			message: 'This operation was aborted.',
 		});
 
+		t.true(signalHandlersRemoved(), 'Abort signal event handlers not removed');
+
 		// Wait for unhandled errors
 		await delay(40);
 	});
@@ -128,7 +149,7 @@ if (globalThis.AbortController !== undefined) {
 	test.serial('aborts in-progress request', withServerAndFakeTimers, async (t, server, got, clock) => {
 		const {emitter, promise} = prepareServer(server, clock);
 
-		const controller = new AbortController();
+		const {controller, signalHandlersRemoved} = createAbortController();
 
 		const body = new ReadableStream({
 			read() {},
@@ -148,12 +169,14 @@ if (globalThis.AbortController !== undefined) {
 			message: 'This operation was aborted.',
 		});
 		await t.notThrowsAsync(promise, 'Request finished instead of aborting.');
+
+		t.true(signalHandlersRemoved(), 'Abort signal event handlers not removed');
 	});
 
 	test.serial('aborts in-progress request with timeout', withServerAndFakeTimers, async (t, server, got, clock) => {
 		const {emitter, promise} = prepareServer(server, clock);
 
-		const controller = new AbortController();
+		const {controller, signalHandlersRemoved} = createAbortController();
 
 		const body = new ReadableStream({
 			read() {},
@@ -173,10 +196,12 @@ if (globalThis.AbortController !== undefined) {
 			message: 'This operation was aborted.',
 		});
 		await t.notThrowsAsync(promise, 'Request finished instead of aborting.');
+
+		t.true(signalHandlersRemoved(), 'Abort signal event handlers not removed');
 	});
 
 	test.serial('abort immediately', withServerAndFakeTimers, async (t, server, got, clock) => {
-		const controller = new AbortController();
+		const {controller, signalHandlersRemoved} = createAbortController();
 
 		const promise = new Promise<void>((resolve, reject) => {
 			// We won't get an abort or even a connection
@@ -198,11 +223,13 @@ if (globalThis.AbortController !== undefined) {
 			message: 'This operation was aborted.',
 		});
 		await t.notThrowsAsync(promise, 'Request finished instead of aborting.');
+
+		t.true(signalHandlersRemoved(), 'Abort signal event handlers not removed');
 	});
 
 	test('recover from abort using abortable promise attribute', async t => {
 		// Abort before connection started
-		const controller = new AbortController();
+		const {controller, signalHandlersRemoved} = createAbortController();
 
 		const p = got('http://example.com', {signal: controller.signal});
 		const recover = p.catch((error: Error) => {
@@ -216,10 +243,12 @@ if (globalThis.AbortController !== undefined) {
 		controller.abort();
 
 		await t.notThrowsAsync(recover);
+
+		t.true(signalHandlersRemoved(), 'Abort signal event handlers not removed');
 	});
 
 	test('recover from abort using error instance', async t => {
-		const controller = new AbortController();
+		const {controller, signalHandlersRemoved} = createAbortController();
 
 		const p = got('http://example.com', {signal: controller.signal});
 		const recover = p.catch((error: Error) => {
@@ -233,13 +262,15 @@ if (globalThis.AbortController !== undefined) {
 		controller.abort();
 
 		await t.notThrowsAsync(recover);
+
+		t.true(signalHandlersRemoved(), 'Abort signal event handlers not removed');
 	});
 
 	// TODO: Use `fakeTimers` here
 	test.serial('throws on incomplete (aborted) response', withServer, async (t, server, got) => {
 		server.get('/', downloadHandler());
 
-		const controller = new AbortController();
+		const {controller, signalHandlersRemoved} = createAbortController();
 
 		const promise = got('', {signal: controller.signal});
 
@@ -251,6 +282,8 @@ if (globalThis.AbortController !== undefined) {
 			code: 'ERR_ABORTED',
 			message: 'This operation was aborted.',
 		});
+
+		t.true(signalHandlersRemoved(), 'Abort signal event handlers not removed');
 	});
 
 	test('throws when aborting cached request', withServer, async (t, server, got) => {
@@ -263,7 +296,7 @@ if (globalThis.AbortController !== undefined) {
 
 		await got({cache});
 
-		const controller = new AbortController();
+		const {controller, signalHandlersRemoved} = createAbortController();
 		const promise = got({cache, signal: controller.signal});
 		controller.abort();
 
@@ -271,10 +304,12 @@ if (globalThis.AbortController !== undefined) {
 			code: 'ERR_ABORTED',
 			message: 'This operation was aborted.',
 		});
+
+		t.true(signalHandlersRemoved(), 'Abort signal event handlers not removed');
 	});
 
 	test('support setting the signal as a default option', async t => {
-		const controller = new AbortController();
+		const {controller, signalHandlersRemoved} = createAbortController();
 
 		const got2 = got.extend({signal: controller.signal});
 		const p = got2('http://example.com', {signal: controller.signal});
@@ -284,6 +319,8 @@ if (globalThis.AbortController !== undefined) {
 			code: 'ERR_ABORTED',
 			message: 'This operation was aborted.',
 		});
+
+		t.true(signalHandlersRemoved(), 'Abort signal event handlers not removed');
 	});
 } else {
 	test('x', t => {