Fix timeout error firing even though it's cancelled (#2399)

Author: paulrutter

source/core/timed-out.ts

@@ -51,13 +51,15 @@ export default function timedOut(request: ClientRequest, delays: Delays, options
 	request[reentry] = true;
 	const cancelers: Array<typeof noop> = [];
 	const {once, unhandleAll} = unhandler();
+	const handled: Map<string, boolean> = new Map<string, boolean>();
 
 	const addTimeout = (delay: number, callback: (delay: number, event: string) => void, event: string): (typeof noop) => {
 		const timeout = setTimeout(callback, delay, delay, event) as unknown as NodeJS.Timeout;
 
 		timeout.unref?.();
 
 		const cancel = (): void => {
+			handled.set(event, true);
 			clearTimeout(timeout);
 		};
 
@@ -69,7 +71,13 @@ export default function timedOut(request: ClientRequest, delays: Delays, options
 	const {host, hostname} = options;
 
 	const timeoutHandler = (delay: number, event: string): void => {
-		request.destroy(new TimeoutError(delay, event));
+		// Use setTimeout to allow for any cancelled events to be handled first,
+		// to prevent firing any TimeoutError unneeded when the event loop is busy or blocked
+		setTimeout(() => {
+			if (!handled.has(event)) {
+				request.destroy(new TimeoutError(delay, event));
+			}
+		}, 0);
 	};
 
 	const cancelTimeouts = (): void => {

test/retry.ts

@@ -1,8 +1,8 @@
-import process from 'node:process';
 import {EventEmitter} from 'node:events';
 import {PassThrough as PassThroughStream} from 'node:stream';
 import type {Socket} from 'node:net';
 import http from 'node:http';
+import https from 'node:https';
 import test from 'ava';
 import is from '@sindresorhus/is';
 import type {Handler} from 'express';
@@ -22,18 +22,16 @@ const handler413: Handler = (_request, response) => {
 	response.end();
 };
 
-const createSocketTimeoutStream = (): http.ClientRequest => {
-	const stream = new PassThroughStream();
-	// @ts-expect-error Mocking the behaviour of a ClientRequest
-	stream.setTimeout = (ms, callback) => {
-		process.nextTick(callback);
-	};
-
-	// @ts-expect-error Mocking the behaviour of a ClientRequest
-	stream.abort = () => {};
-	stream.resume();
+const createSocketTimeoutStream = (url: string): http.ClientRequest => {
+	if (url.includes('https:')) {
+		return https.request(url, {
+			timeout: 1,
+		});
+	}
 
-	return stream as unknown as http.ClientRequest;
+	return http.request(url, {
+		timeout: socketTimeout,
+	});
 };
 
 test('works on timeout', withServer, async (t, server, got) => {
@@ -57,7 +55,7 @@ test('works on timeout', withServer, async (t, server, got) => {
 			}
 
 			knocks++;
-			return createSocketTimeoutStream();
+			return createSocketTimeoutStream(server.url);
 		},
 	})).body, 'who`s there?');
 });
@@ -93,7 +91,7 @@ test('setting to `0` disables retrying', async t => {
 				return 0;
 			},
 		},
-		request: () => createSocketTimeoutStream(),
+		request: () => createSocketTimeoutStream('https://example.com'),
 	}), {
 		instanceOf: TimeoutError,
 		message: `Timeout awaiting 'socket' for ${socketTimeout}ms`,

test/timeout.ts

@@ -1,8 +1,9 @@
 import process from 'node:process';
 import {EventEmitter} from 'node:events';
-import stream, {PassThrough as PassThroughStream} from 'node:stream';
+import stream from 'node:stream';
 import {pipeline as streamPipeline} from 'node:stream/promises';
 import http from 'node:http';
+import https from 'node:https';
 import net from 'node:net';
 import getStream from 'get-stream';
 import test from 'ava';
@@ -90,17 +91,9 @@ test.serial('socket timeout', async t => {
 				limit: 0,
 			},
 			request() {
-				const stream = new PassThroughStream();
-				// @ts-expect-error Mocking the behaviour of a ClientRequest
-				stream.setTimeout = (ms, callback) => {
-					process.nextTick(callback);
-				};
-
-				// @ts-expect-error Mocking the behaviour of a ClientRequest
-				stream.abort = () => {};
-				stream.resume();
-
-				return stream as unknown as http.ClientRequest;
+				return https.request('https://example.com', {
+					timeout: 0,
+				});
 			},
 		}),
 		{