Skip to content

Commit

Permalink
fix: don't timeout while waiting for client to send request (#1604)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
ronag committed Aug 17, 2022
1 parent fa9fd90 commit ecae314
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 17 deletions.
1 change: 1 addition & 0 deletions .github/workflows/nodejs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ jobs:
exclude: |
- runs-on: windows-latest
node-version: 16
- node-version: 12
automerge:
if: >
github.event_name == 'pull_request' &&
Expand Down
2 changes: 1 addition & 1 deletion docs/api/Client.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion docs/api/Dispatcher.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
17 changes: 15 additions & 2 deletions lib/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down Expand Up @@ -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
}

Expand Down
16 changes: 3 additions & 13 deletions test/request-timeout.js
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Expand All @@ -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)
Expand Down
48 changes: 48 additions & 0 deletions test/request-timeout2.js
Original file line number Diff line number Diff line change
@@ -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()
})
})
})

0 comments on commit ecae314

Please sign in to comment.