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

Author: ronag

.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' &&

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.

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`

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
   }
 

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)

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