From 39e53d1174a081fc13e996ca4740fc09051a283e Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Wed, 17 Aug 2022 16:43:08 +0200 Subject: [PATCH] fix: don't timeout while waiting for client to send request (#1604) * fix: don't timeout while waiting for client to send request When e.g. uploading a large file from the client one would always get a headers timeout even though everything was working and the file was being in progress of sending. * fixuP --- .github/workflows/nodejs.yml | 1 + docs/api/Client.md | 2 +- docs/api/Dispatcher.md | 2 +- lib/client.js | 17 +++++++++++-- test/request-timeout.js | 16 +++--------- test/request-timeout2.js | 48 ++++++++++++++++++++++++++++++++++++ 6 files changed, 69 insertions(+), 17 deletions(-) create mode 100644 test/request-timeout2.js diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml index c246e0bdd08..531b6d1b00a 100644 --- a/.github/workflows/nodejs.yml +++ b/.github/workflows/nodejs.yml @@ -23,6 +23,7 @@ jobs: exclude: | - runs-on: windows-latest node-version: 16 + - node-version: 12 automerge: if: > github.event_name == 'pull_request' && diff --git a/docs/api/Client.md b/docs/api/Client.md index cfc4c393e0f..76a22253ffc 100644 --- a/docs/api/Client.md +++ b/docs/api/Client.md @@ -18,7 +18,7 @@ Returns: `Client` ### Parameter: `ClientOptions` * **bodyTimeout** `number | null` (optional) - Default: `30e3` - The timeout after which a request will time out, in milliseconds. Monitors time between receiving body data. Use `0` to disable it entirely. Defaults to 30 seconds. -* **headersTimeout** `number | null` (optional) - Default: `30e3` - The amount of time the parser will wait to receive the complete HTTP headers. Defaults to 30 seconds. +* **headersTimeout** `number | null` (optional) - Default: `30e3` - The amount of time the parser will wait to receive the complete HTTP headers while not sending the request. Defaults to 30 seconds. * **keepAliveMaxTimeout** `number | null` (optional) - Default: `600e3` - The maximum allowed `keepAliveTimeout` when overridden by *keep-alive* hints from the server. Defaults to 10 minutes. * **keepAliveTimeout** `number | null` (optional) - Default: `4e3` - The timeout after which a socket without active requests will time out. Monitors time between activity on a connected socket. This value may be overridden by *keep-alive* hints from the server. See [MDN: HTTP - Headers - Keep-Alive directives](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Keep-Alive#directives) for more details. Defaults to 4 seconds. * **keepAliveTimeoutThreshold** `number | null` (optional) - Default: `1e3` - A number subtracted from server *keep-alive* hints when overriding `keepAliveTimeout` to account for timing inaccuracies caused by e.g. transport latency. Defaults to 1 second. diff --git a/docs/api/Dispatcher.md b/docs/api/Dispatcher.md index 32ccb57993f..25565152e50 100644 --- a/docs/api/Dispatcher.md +++ b/docs/api/Dispatcher.md @@ -199,7 +199,7 @@ Returns: `Boolean` - `false` if dispatcher is busy and further dispatch calls wo * **blocking** `boolean` (optional) - Default: `false` - Whether the response is expected to take a long time and would end up blocking the pipeline. When this is set to `true` further pipelining will be avoided on the same connection until headers have been received. * **upgrade** `string | null` (optional) - Default: `null` - Upgrade the request. Should be used to specify the kind of upgrade i.e. `'Websocket'`. * **bodyTimeout** `number | null` (optional) - The timeout after which a request will time out, in milliseconds. Monitors time between receiving body data. Use `0` to disable it entirely. Defaults to 30 seconds. -* **headersTimeout** `number | null` (optional) - The amount of time the parser will wait to receive the complete HTTP headers. Defaults to 30 seconds. +* **headersTimeout** `number | null` (optional) - The amount of time the parser will wait to receive the complete HTTP headers while not sending the request. Defaults to 30 seconds. * **throwOnError** `boolean` (optional) - Default: `false` - Whether Undici should throw an error upon receiving a 4xx or 5xx response from the server. #### Parameter: `DispatchHandler` diff --git a/lib/client.js b/lib/client.js index fb0b985faab..14fcaee2e3c 100644 --- a/lib/client.js +++ b/lib/client.js @@ -889,8 +889,10 @@ function onParserTimeout (parser) { /* istanbul ignore else */ if (timeoutType === TIMEOUT_HEADERS) { - assert(!parser.paused, 'cannot be paused while waiting for headers') - util.destroy(socket, new HeadersTimeoutError()) + if (!socket[kWriting] || socket.writableNeedDrain || client[kRunning] > 1) { + assert(!parser.paused, 'cannot be paused while waiting for headers') + util.destroy(socket, new HeadersTimeoutError()) + } } else if (timeoutType === TIMEOUT_BODY) { if (!parser.paused) { util.destroy(socket, new BodyTimeoutError()) @@ -1641,7 +1643,18 @@ class AsyncWriter { this.bytesWritten += len const ret = socket.write(chunk) + request.onBodySent(chunk) + + if (!ret) { + if (socket[kParser].timeout && socket[kParser].timeoutType === TIMEOUT_HEADERS) { + // istanbul ignore else: only for jest + if (socket[kParser].timeout.refresh) { + socket[kParser].timeout.refresh() + } + } + } + return ret } diff --git a/test/request-timeout.js b/test/request-timeout.js index 16547d7d117..ea60c2f0b11 100644 --- a/test/request-timeout.js +++ b/test/request-timeout.js @@ -18,37 +18,27 @@ const { test('request timeout', (t) => { t.plan(1) - const clock = FakeTimers.install() - t.teardown(clock.uninstall.bind(clock)) - const server = createServer((req, res) => { setTimeout(() => { res.end('hello') - }, 100) - clock.tick(100) + }, 1000) }) t.teardown(server.close.bind(server)) server.listen(0, () => { - const client = new Client(`http://localhost:${server.address().port}`, { headersTimeout: 50 }) + const client = new Client(`http://localhost:${server.address().port}`, { headersTimeout: 500 }) t.teardown(client.destroy.bind(client)) client.request({ path: '/', method: 'GET' }, (err, response) => { t.type(err, errors.HeadersTimeoutError) }) - - clock.tick(50) }) }) test('request timeout with readable body', (t) => { t.plan(1) - const clock = FakeTimers.install() - t.teardown(clock.uninstall.bind(clock)) - const server = createServer((req, res) => { - clock.tick(100) }) t.teardown(server.close.bind(server)) @@ -57,7 +47,7 @@ test('request timeout with readable body', (t) => { t.teardown(() => unlinkSync(tempfile)) server.listen(0, () => { - const client = new Client(`http://localhost:${server.address().port}`, { headersTimeout: 50 }) + const client = new Client(`http://localhost:${server.address().port}`, { headersTimeout: 1e3 }) t.teardown(client.destroy.bind(client)) const body = createReadStream(tempfile) diff --git a/test/request-timeout2.js b/test/request-timeout2.js new file mode 100644 index 00000000000..53943fbf5d3 --- /dev/null +++ b/test/request-timeout2.js @@ -0,0 +1,48 @@ +'use strict' + +const { test } = require('tap') +const { Client } = require('..') +const { createServer } = require('http') +const { Readable } = require('stream') + +test('request timeout with slow readable body', (t) => { + t.plan(1) + + const server = createServer(async (req, res) => { + let str = '' + for await (const x of req) { + str += x + } + res.end(str) + }) + t.teardown(server.close.bind(server)) + + server.listen(0, () => { + const client = new Client(`http://localhost:${server.address().port}`, { headersTimeout: 50 }) + t.teardown(client.close.bind(client)) + + const body = new Readable({ + read () { + if (this._reading) { + return + } + this._reading = true + + this.push('asd') + setTimeout(() => { + this.push('asd') + this.push(null) + }, 2e3) + } + }) + client.request({ + path: '/', + method: 'POST', + headersTimeout: 1e3, + body + }, async (err, response) => { + t.error(err) + await response.body.dump() + }) + }) +})