Skip to content

Commit d1d4ed2

Browse files
committed
Fix HTTP/2 memory leak from timeout listeners with connection reuse
Fixes #2351
1 parent 2d8182d commit d1d4ed2

2 files changed

Lines changed: 75 additions & 3 deletions

File tree

source/core/index.ts

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -933,13 +933,22 @@ export default class Request extends Duplex implements RequestEvents<Request> {
933933

934934
timer(request);
935935

936+
this._cancelTimeouts = timedOut(request, timeout, url as URL);
937+
936938
if (this.options.http2) {
937939
// Unset stream timeout, as the `timeout` option was used only for connection timeout.
938-
request.setTimeout(0);
940+
// We remove all 'timeout' listeners instead of calling setTimeout(0) because:
941+
// 1. setTimeout(0) causes a memory leak (see https://github.com/sindresorhus/got/issues/690)
942+
// 2. With HTTP/2 connection reuse, setTimeout(0) accumulates listeners on the socket
943+
// 3. removeAllListeners('timeout') properly cleans up without the memory leak
944+
request.removeAllListeners('timeout');
945+
946+
// For HTTP/2, wait for socket and remove timeout listeners from it
947+
request.once('socket', (socket: Socket) => {
948+
socket.removeAllListeners('timeout');
949+
});
939950
}
940951

941-
this._cancelTimeouts = timedOut(request, timeout, url as URL);
942-
943952
const responseEventName = options.cache ? 'cacheableResponse' : 'response';
944953

945954
request.once(responseEventName, (response: IncomingMessageWithTimings) => {

test/timeout.ts

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -750,3 +750,66 @@ test('http2 timeout', async t => {
750750

751751
t.true(error?.code === 'ETIMEDOUT' || error?.code === 'EUNSUPPORTED', error?.stack);
752752
});
753+
754+
// Reproduces the memory leak reported in https://github.com/sindresorhus/got/issues/2351
755+
test.serial('no memory leak when using http2 with socket timeout and connection reuse', withHttpsServer(), async (t, server, got) => {
756+
const {default: http2wrapper} = await import('http2-wrapper');
757+
const customAgent = new http2wrapper.Agent();
758+
759+
server.get('/:id', (_request, response) => {
760+
response.end('ok');
761+
});
762+
763+
const requestCount = 15; // Make concurrent requests to accumulate listeners
764+
let sharedSocket: any;
765+
let maxListenerCount = 0;
766+
767+
// Make requests that overlap in time to reproduce the listener accumulation
768+
server.get('/slow/:id', async (_request, response) => {
769+
// Slow response to keep requests active simultaneously
770+
await delay(200);
771+
response.end('ok');
772+
});
773+
774+
// Start many concurrent requests with the same agent to reuse the connection
775+
const promises = [];
776+
const handleRequest = (request: any) => {
777+
// Track socket and check listener count DURING requests
778+
request.once('socket', (socket: any) => {
779+
if (!sharedSocket) {
780+
sharedSocket = socket;
781+
}
782+
783+
// Check how many listeners are on the socket while requests are active
784+
const currentCount = socket.listenerCount('timeout');
785+
if (currentCount > maxListenerCount) {
786+
maxListenerCount = currentCount;
787+
}
788+
});
789+
};
790+
791+
for (let i = 0; i < requestCount; i++) {
792+
const promise = got(`slow/${i}`, {
793+
http2: true,
794+
agent: {http2: customAgent},
795+
timeout: {socket: 30_000},
796+
https: {rejectUnauthorized: false},
797+
}).on('request', handleRequest);
798+
promises.push(promise);
799+
}
800+
801+
// Wait for all concurrent requests to finish
802+
await Promise.all(promises);
803+
804+
// The bug: setTimeout(0) doesn't remove timeout listeners, so with HTTP/2
805+
// connection reuse and concurrent requests, listeners accumulate on the shared socket
806+
// The fix: removeAllListeners('timeout') properly cleans up
807+
808+
t.truthy(sharedSocket, 'Should have a socket');
809+
810+
// With the bug (setTimeout(0)), maxListenerCount would be >> 1 (grows with concurrent requests)
811+
// With the fix (removeAllListeners), maxListenerCount should be 0 or 1
812+
t.true(maxListenerCount <= 1, `Socket peaked at ${maxListenerCount} timeout listeners (expected ≤ 1)`);
813+
814+
customAgent.destroy();
815+
});

0 commit comments

Comments
 (0)