From 8f1768a4d2fe2b49f4c3639d15ac28c9fc303bca Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 6 Nov 2026 10:15:02 +0100 Subject: [PATCH] fix(h1): do not reuse idle sockets whose keep-alive timeout expired The keep-alive timer of an idle socket cannot fire while the event loop is blocked. When the next request is dispatched from the poll phase (an I/O callback, a server request handler, or a continuation after awaiting I/O), the idle socket validation added for GHSA-35p6-xmwp-9g52 completes in the check phase of the same loop iteration, before that timer fires and before a FIN received during the block is read. The request is then written to a socket that the server may already have closed, and fails with ECONNRESET. Record when the keep-alive timer is armed, and destroy the socket with an InformationalError instead of reusing it once its keep-alive timeout has passed. Nothing is running on the socket, so the pending requests are sent on a new connection. The check does not depend on kSocketUsed, so it also covers the experimental Milo parser, which never sets that symbol. Refs: https://github.com/nodejs/node/pull/67292 Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Sfj4nEWQ4ovycNDjbvXcRs Signed-off-by: Matteo Collina --- lib/dispatcher/client-h1.js | 24 +++++++ test/client-keep-alive-blocked-loop.js | 90 ++++++++++++++++++++++++++ 2 files changed, 114 insertions(+) create mode 100644 test/client-keep-alive-blocked-loop.js diff --git a/lib/dispatcher/client-h1.js b/lib/dispatcher/client-h1.js index a26b9c014e8..13bc66f9372 100644 --- a/lib/dispatcher/client-h1.js +++ b/lib/dispatcher/client-h1.js @@ -67,6 +67,7 @@ const removeAllListeners = util.removeAllListeners const kIdleSocketValidation = Symbol('kIdleSocketValidation') const kIdleSocketValidationTimeout = Symbol('kIdleSocketValidationTimeout') const kSocketUsed = Symbol('kSocketUsed') +const kIdleSince = Symbol('kIdleSince') const kTypeOfService = Symbol('kTypeOfService') let extractBody @@ -1107,6 +1108,19 @@ function scheduleIdleSocketValidation (client, socket) { }) } +/** + * kIdleSince is recorded when the keep-alive timer is armed, so this never + * expires a socket before that timer is due. + * @param {import('net').Socket} socket + * @returns {boolean} + */ +function isIdleSocketExpired (socket) { + const parser = socket[kParser] + return parser.timeoutType === TIMEOUT_KEEP_ALIVE && + parser.timeoutValue > 0 && + performance.now() - socket[kIdleSince] >= parser.timeoutValue +} + /** * @param {import('./client.js')} client */ @@ -1124,6 +1138,15 @@ function resumeH1 (client) { socket[kNoRef] = false } + // The keep-alive timer cannot fire while the event loop is blocked, and a + // request dispatched from the poll phase passes the idle socket validation + // below before that timer or a FIN received during the block is processed. + // Check the clock so that an expired socket is not reused. + if (client[kRunning] === 0 && client[kPending] > 0 && isIdleSocketExpired(socket)) { + util.destroy(socket, new InformationalError('socket idle timeout')) + return + } + if (client[kRunning] === 0 && client[kPending] > 0 && socket[kSocketUsed]) { if (socket[kIdleSocketValidation] === 0) { scheduleIdleSocketValidation(client, socket) @@ -1152,6 +1175,7 @@ function resumeH1 (client) { if (client[kSize] === 0) { if (socket[kParser].timeoutType !== TIMEOUT_KEEP_ALIVE) { + socket[kIdleSince] = performance.now() socket[kParser].setTimeout(client[kKeepAliveTimeoutValue], TIMEOUT_KEEP_ALIVE) } } else if (client[kRunning] > 0 && socket[kParser].statusCode < 200) { diff --git a/test/client-keep-alive-blocked-loop.js b/test/client-keep-alive-blocked-loop.js new file mode 100644 index 00000000000..67887f571af --- /dev/null +++ b/test/client-keep-alive-blocked-loop.js @@ -0,0 +1,90 @@ +'use strict' + +const { test } = require('node:test') +const assert = require('node:assert') +const { once } = require('node:events') +const { stat } = require('node:fs') +const { createServer } = require('node:http') +const { setImmediate: waitImmediate } = require('node:timers/promises') +const { Client } = require('..') + +// The keep-alive timer cannot fire while the event loop is blocked. When the +// next request is dispatched from the poll phase, the idle socket validation +// completes in the check phase of the same iteration, before that timer or a +// FIN that arrived during the block is processed. The client must check the +// clock instead, or it reuses a socket that the server may already have +// closed (ECONNRESET). + +function sleepSync (ms) { + Atomics.wait(new Int32Array(new SharedArrayBuffer(4)), 0, 0, ms) +} + +async function setup (t, keepAliveTimeout) { + const state = { connections: 0, disconnects: [] } + + const server = createServer((req, res) => { + res.end('ok') + }) + // No Keep-Alive hint from the server, so the client's keepAliveTimeout + // alone sets the idle timeout. The server never closes the connection + // itself, so the test only passes if the client opens a new one. + server.keepAliveTimeout = 0 + server.on('connection', () => { + state.connections++ + }) + server.listen(0) + await once(server, 'listening') + + const client = new Client(`http://localhost:${server.address().port}`, { + keepAliveTimeout + }) + client.on('disconnect', (origin, targets, err) => { + state.disconnects.push(err) + }) + + t.after(async () => { + await client.close() + server.close() + }) + + const res = await client.request({ path: '/', method: 'GET' }) + assert.strictEqual(await res.body.text(), 'ok') + // Let the socket settle into the idle state. + await waitImmediate() + assert.strictEqual(state.connections, 1) + + return { client, state } +} + +// Blocks for `blockMs`, then dispatches a request, from an fs callback, i.e. +// from the poll phase. +function requestAfterBlockInPollPhase (client, blockMs) { + return new Promise((resolve, reject) => { + stat(__filename, () => { + sleepSync(blockMs) + client.request({ path: '/', method: 'GET' }).then(resolve, reject) + }) + }) +} + +test('does not reuse an idle socket whose keep-alive timeout passed while the event loop was blocked', async (t) => { + const { client, state } = await setup(t, 200) + + const res = await requestAfterBlockInPollPhase(client, 600) + assert.strictEqual(await res.body.text(), 'ok') + + assert.strictEqual(state.connections, 2) + assert.strictEqual(state.disconnects.length, 1) + assert.strictEqual(state.disconnects[0].code, 'UND_ERR_INFO') + assert.strictEqual(state.disconnects[0].message, 'socket idle timeout') +}) + +test('reuses an idle socket whose keep-alive timeout did not pass while the event loop was blocked', async (t) => { + const { client, state } = await setup(t, 60e3) + + const res = await requestAfterBlockInPollPhase(client, 600) + assert.strictEqual(await res.body.text(), 'ok') + + assert.strictEqual(state.connections, 1) + assert.strictEqual(state.disconnects.length, 0) +})