fix(h1): do not reuse idle sockets whose keep-alive timeout expired

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) {

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)
+})