diff --git a/lib/dispatcher/client-h2.js b/lib/dispatcher/client-h2.js index 0585e7cd925..f0a1264cb20 100644 --- a/lib/dispatcher/client-h2.js +++ b/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 diff --git a/test/http2-headers-timeout.js b/test/http2-headers-timeout.js new file mode 100644 index 00000000000..c3e2df69c49 --- /dev/null +++ b/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 +}) diff --git a/test/http2-timeout.js b/test/http2-timeout.js index cb99c4d5374..c65c9c0ef0d 100644 --- a/test/http2-timeout.js +++ b/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