fix(h2): honour headersTimeout (#5604)

Author: mcollina

lib/dispatcher/client-h2.js

@@ -8,7 +8,9 @@ const {
   RequestAbortedError,
   SocketError,
   InformationalError,
-  InvalidArgumentError
+  InvalidArgumentError,
+  HeadersTimeoutError,
+  BodyTimeoutError
 } = require('../core/errors.js')
 const {
   kUrl,
@@ -33,6 +35,7 @@ const {
   kHTTPContext,
   kClosed,
   kBodyTimeout,
+  kHeadersTimeout,
   kEnableConnectProtocol,
   kRemoteSettings,
   kHTTP2Stream,
@@ -416,7 +419,10 @@ function shouldSendContentLength (method) {
 }
 
 function writeH2 (client, request) {
-  const requestTimeout = request.bodyTimeout ?? client[kBodyTimeout]
+  // Time to the response headers, then time between body chunks. Using
+  // bodyTimeout for both made headersTimeout a no-op over HTTP/2.
+  const headersTimeout = request.headersTimeout ?? client[kHeadersTimeout]
+  const bodyTimeout = request.bodyTimeout ?? client[kBodyTimeout]
   const session = client[kHTTP2Session]
   const { method, path, host, upgrade, expectContinue, signal, protocol, headers: reqHeaders } = request
   let { body } = request
@@ -554,7 +560,7 @@ function writeH2 (client, request) {
         if (session[kOpenStreams] === 0) session.unref()
       })
 
-      stream.setTimeout(requestTimeout)
+      stream.setTimeout(headersTimeout)
       return true
     }
 
@@ -576,7 +582,7 @@ function writeH2 (client, request) {
       session[kOpenStreams] -= 1
       if (session[kOpenStreams] === 0) session.unref()
     })
-    stream.setTimeout(requestTimeout)
+    stream.setTimeout(headersTimeout)
 
     return true
   }
@@ -677,7 +683,7 @@ function writeH2 (client, request) {
 
   // Increment counter as we have new streams open
   ++session[kOpenStreams]
-  stream.setTimeout(requestTimeout)
+  stream.setTimeout(headersTimeout)
 
   // Track whether we received a response (headers)
   let responseReceived = false
@@ -686,6 +692,7 @@ function writeH2 (client, request) {
     const { [HTTP2_HEADER_STATUS]: statusCode, ...realHeaders } = headers
     request.onResponseStarted()
     responseReceived = true
+    stream.setTimeout(bodyTimeout)
 
     // Due to the stream nature, it is possible we face a race condition
     // where the stream has been assigned, but the request has been aborted
@@ -755,7 +762,9 @@ function writeH2 (client, request) {
   })
 
   stream.on('timeout', () => {
-    const err = new InformationalError(`HTTP/2: "stream timeout after ${requestTimeout}"`)
+    const err = responseReceived
+      ? new BodyTimeoutError(`HTTP/2: "body timeout after ${bodyTimeout}"`)
+      : new HeadersTimeoutError(`HTTP/2: "headers timeout after ${headersTimeout}"`)
     stream.removeAllListeners('data')
     session[kOpenStreams] -= 1
 

test/http2-headers-timeout.js

@@ -0,0 +1,83 @@
+'use strict'
+
+const { tspl } = require('@matteo.collina/tspl')
+const { test, after } = require('node:test')
+const { createSecureServer } = require('node:http2')
+const { once } = require('node:events')
+
+const pem = require('@metcoder95/https-pem')
+
+const { Client } = require('..')
+
+// writeH2() armed the stream timer from bodyTimeout for the whole request, so
+// headersTimeout had no effect at all over HTTP/2: a server that accepted the
+// stream and never sent headers was only cut off after bodyTimeout.
+
+test('headersTimeout is honoured over HTTP/2', async t => {
+  t = tspl(t, { plan: 2 })
+
+  const server = createSecureServer(await pem.generate({ opts: { keySize: 2048 } }))
+
+  // Accept the stream and never respond.
+  server.on('stream', (stream) => {
+    stream.on('error', () => {})
+  })
+
+  after(() => server.close())
+  await once(server.listen(0), 'listening')
+
+  const client = new Client(`https://localhost:${server.address().port}`, {
+    connect: { rejectUnauthorized: false },
+    allowH2: true,
+    headersTimeout: 200,
+    // Much larger, so a failure here means bodyTimeout was used instead.
+    bodyTimeout: 5000
+  })
+  after(() => client.close())
+
+  const start = Date.now()
+
+  await t.rejects(client.request({ path: '/', method: 'GET' }), {
+    message: 'HTTP/2: "headers timeout after 200"',
+    code: 'UND_ERR_HEADERS_TIMEOUT'
+  })
+
+  t.ok(Date.now() - start < 2000, 'must not wait for bodyTimeout')
+
+  await t.completed
+})
+
+test('bodyTimeout applies once the response headers arrive', async t => {
+  t = tspl(t, { plan: 2 })
+
+  const server = createSecureServer(await pem.generate({ opts: { keySize: 2048 } }))
+
+  // Answer immediately, then stall the body.
+  server.on('stream', (stream) => {
+    stream.on('error', () => {})
+    stream.respond({ ':status': 200 })
+  })
+
+  after(() => server.close())
+  await once(server.listen(0), 'listening')
+
+  const client = new Client(`https://localhost:${server.address().port}`, {
+    connect: { rejectUnauthorized: false },
+    allowH2: true,
+    // Small enough that a request still waiting on headersTimeout would fail
+    // the wrong way.
+    headersTimeout: 5000,
+    bodyTimeout: 200
+  })
+  after(() => client.close())
+
+  const res = await client.request({ path: '/', method: 'GET' })
+  t.strictEqual(res.statusCode, 200)
+
+  await t.rejects(res.body.text(), {
+    message: 'HTTP/2: "body timeout after 200"',
+    code: 'UND_ERR_BODY_TIMEOUT'
+  })
+
+  await t.completed
+})

test/http2-timeout.js

@@ -50,7 +50,8 @@ test('Should handle http2 stream timeout', async t => {
   })
 
   await t.rejects(res.body.text(), {
-    message: 'HTTP/2: "stream timeout after 50"'
+    message: 'HTTP/2: "body timeout after 50"',
+    code: 'UND_ERR_BODY_TIMEOUT'
   })
 
   await t.completed