Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Feb 10, 2025
10 changes: 9 additions & 1 deletion source/core/timed-out.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};

Expand All @@ -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 => {
Expand Down
26 changes: 12 additions & 14 deletions test/retry.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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) => {
Expand All @@ -57,7 +55,7 @@ test('works on timeout', withServer, async (t, server, got) => {
}

knocks++;
return createSocketTimeoutStream();
return createSocketTimeoutStream(server.url);
},
})).body, 'who`s there?');
});
Expand Down Expand Up @@ -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`,
Expand Down
17 changes: 5 additions & 12 deletions test/timeout.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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,
});
},
}),
{
Expand Down