fix: replace socketTimeout with bodyTimeout

Replaces socketTimeout with a bodyTimeout parameter. Removes socketTimeout.

Removes unecessary retry logic.

Fixes: #447

README.md

@@ -46,13 +46,13 @@ It should only include the protocol, hostname, and the port.
 
 Options:
 
-- `socketTimeout: Number`, the timeout after which a socket with active requests
-  will time out. Monitors time between activity on a connected socket.
-  Use `0` to disable it entirely. Default: `30e3` milliseconds (30s).
-
 - `socketPath: String|Null`, an IPC endpoint, either Unix domain socket or Windows named pipe.
   Default: `null`.
 
+- `bodyTimeout: Number`, the timeout after which a socket with active requests
+  will time out. Monitors time between activity on an active socket.
+  Use `0` to disable it entirely. Default: `30e3` milliseconds (30s).
+
 - `idleTimeout: Number`, the timeout after which a socket without active requests
   will time out. Monitors time between activity on a connected socket.
   This value may be overriden by *keep-alive* hints from the server.
@@ -606,7 +606,6 @@ const { errors } = require('undici')
 | -----------------------------|-----------------------------------|------------------------------------------------|
 | `InvalidArgumentError`       |  `UND_ERR_INVALID_ARG`            | passed an invalid argument.                    |
 | `InvalidReturnValueError`    |  `UND_ERR_INVALID_RETURN_VALUE`   | returned an invalid value.                     |
-| `SocketTimeoutError`         |  `UND_ERR_SOCKET_TIMEOUT`         | a socket exceeds the `socketTimeout` option.   |
 | `RequestTimeoutError`        |  `UND_ERR_REQUEST_TIMEOUT`        | a request exceeds the `requestTimeout` option. |
 | `RequestAbortedError`        |  `UND_ERR_ABORTED`                | the request has been aborted by the user       |
 | `ClientDestroyedError`       |  `UND_ERR_DESTROYED`              | trying to use a destroyed client.              |

lib/core/client.js

@@ -10,10 +10,10 @@ const util = require('./util')
 const Request = require('./request')
 const {
   ContentLengthMismatchError,
-  SocketTimeoutError,
   TrailerMismatchError,
   InvalidArgumentError,
   RequestAbortedError,
+  BodyTimeoutError,
   ClientDestroyedError,
   ClientClosedError,
   HeadersTimeoutError,
@@ -35,7 +35,8 @@ const {
   kNeedDrain,
   kTLSServerName,
   kIdleTimeout,
-  kSocketTimeout,
+  kHostHeader,
+  kBodyTimeout,
   kTLSOpts,
   kClosed,
   kDestroyed,
@@ -44,8 +45,6 @@ const {
   kError,
   kOnDestroyed,
   kPipelining,
-  kRetryDelay,
-  kRetryTimeout,
   kSocket,
   kSocketPath,
   kKeepAliveTimeout,
@@ -57,8 +56,6 @@ const {
   kTLSSession
 } = require('./symbols')
 
-const kHostHeader = Symbol('host header')
-
 const nodeVersions = process.version.split('.')
 const nodeMajorVersion = parseInt(nodeVersions[0].slice(1))
 const nodeMinorVersion = parseInt(nodeVersions[1])
@@ -68,7 +65,7 @@ class Client extends EventEmitter {
   constructor (url, {
     maxHeaderSize,
     headersTimeout,
-    socketTimeout,
+    bodyTimeout,
     idleTimeout,
     maxKeepAliveTimeout,
     keepAlive,
@@ -112,8 +109,8 @@ class Client extends EventEmitter {
       throw new InvalidArgumentError('invalid maxHeaderSize')
     }
 
-    if (socketTimeout != null && !Number.isFinite(socketTimeout)) {
-      throw new InvalidArgumentError('invalid socketTimeout')
+    if (bodyTimeout != null && !Number.isFinite(bodyTimeout)) {
+      throw new InvalidArgumentError('invalid bodyTimeout')
     }
 
     if (idleTimeout != null && (!Number.isFinite(idleTimeout) || idleTimeout <= 0)) {
@@ -141,9 +138,9 @@ class Client extends EventEmitter {
     this[kPipelining] = pipelining || 1
     this[kMaxHeadersSize] = maxHeaderSize || 16384
     this[kHeadersTimeout] = headersTimeout == null ? 30e3 : headersTimeout
+    this[kBodyTimeout] = bodyTimeout == null ? 30e3 : bodyTimeout
     this[kUrl] = url
     this[kSocketPath] = socketPath
-    this[kSocketTimeout] = socketTimeout == null ? 30e3 : socketTimeout
     this[kIdleTimeout] = idleTimeout == null ? 4e3 : idleTimeout
     this[kKeepAliveMaxTimeout] = keepAliveMaxTimeout == null ? 600e3 : keepAliveMaxTimeout
     this[kKeepAliveTimeoutThreshold] = keepAliveTimeoutThreshold == null ? 1e3 : keepAliveTimeoutThreshold
@@ -154,8 +151,6 @@ class Client extends EventEmitter {
     this[kTLSServerName] = (tls && tls.servername) || null
     this[kHost] = null
     this[kTLSOpts] = tls
-    this[kRetryDelay] = 0
-    this[kRetryTimeout] = null
     this[kOnDestroyed] = []
     this[kWriting] = false
     this[kResuming] = 0 // 0, idle, 1, scheduled, 2 resuming
@@ -321,8 +316,6 @@ class Client extends EventEmitter {
       request.onError(err)
     }
 
-    clearTimeout(this[kRetryTimeout])
-    this[kRetryTimeout] = null
     this[kClosed] = true
     this[kDestroyed] = true
     this[kOnDestroyed].push(callback)
@@ -385,6 +378,13 @@ class Parser extends HTTPParser {
     this.shouldKeepAlive = false
     this.read = 0
     this.request = null
+    this.bodyTimeout = client[kBodyTimeout]
+      ? setTimeout((socket) => {
+        if (socket._handle.reading) {
+          util.destroy(socket, new BodyTimeoutError())
+        }
+      }, client[kBodyTimeout], socket)
+      : null
   }
 
   [HTTPParser.kOnTimeout] () {
@@ -572,6 +572,10 @@ class Parser extends HTTPParser {
 
     assert(statusCode >= 200)
 
+    if (this.bodyTimeout && this.bodyTimeout.refresh) {
+      this.bodyTimeout.refresh()
+    }
+
     try {
       if (request.onBody(chunk, offset, length) === false) {
         socket[kPause]()
@@ -651,23 +655,27 @@ class Parser extends HTTPParser {
       resume(client)
     }
   }
+
+  destroy () {
+    clearTimeout(this.bodyTimeout)
+    this.unconsume()
+    setImmediate((self) => self.close(), this)
+  }
 }
 
 function onSocketConnect () {
   const { [kClient]: client } = this
 
   assert(!this.destroyed)
   assert(!client[kWriting])
-  assert(!client[kRetryTimeout])
 
   client[kReset] = false
-  client[kRetryDelay] = 0
   client.emit('connect')
   resume(client)
 }
 
 function onSocketTimeout () {
-  util.destroy(this, new SocketTimeoutError())
+  util.destroy(this, new InformationalError('socket idle timeout'))
 }
 
 function onSocketError (err) {
@@ -705,8 +713,7 @@ function onSocketClose () {
 
   client[kSocket] = null
 
-  parser.unconsume()
-  setImmediate(() => parser.close())
+  parser.destroy()
 
   if (err.code !== 'UND_ERR_INFO') {
     // Evict session on errors.
@@ -747,6 +754,7 @@ function detachSocket (socket) {
   socket[kPause] = null
   socket[kResume] = null
   socket[kClient] = null
+  socket[kParser].destroy()
   socket[kParser] = null
   socket[kError] = null
   socket
@@ -759,7 +767,6 @@ function detachSocket (socket) {
 
 function connect (client) {
   assert(!client[kSocket])
-  assert(!client[kRetryTimeout])
 
   const { protocol, port, hostname } = client[kUrl]
 
@@ -867,9 +874,7 @@ function _resume (client, sync) {
     }
 
     if (client[kSocket]) {
-      const timeout = client.running
-        ? client[kSocketTimeout]
-        : client[kKeepAliveTimeout]
+      const timeout = client.running ? 0 : client[kKeepAliveTimeout]
 
       if (client[kSocket].timeout !== timeout) {
         client[kSocket].setTimeout(timeout)
@@ -929,17 +934,8 @@ function _resume (client, sync) {
       }
     }
 
-    if (!client[kSocket] && !client[kRetryTimeout]) {
-      if (client[kRetryDelay]) {
-        client[kRetryTimeout] = setTimeout(() => {
-          client[kRetryTimeout] = null
-          connect(client)
-        }, client[kRetryDelay])
-        client[kRetryDelay] = Math.min(client[kRetryDelay] * 2, client[kSocketTimeout])
-      } else {
-        connect(client)
-        client[kRetryDelay] = 1e3
-      }
+    if (!client[kSocket]) {
+      connect(client)
       return
     }
 

lib/core/errors.js

@@ -18,13 +18,13 @@ class HeadersTimeoutError extends UndiciError {
   }
 }
 
-class SocketTimeoutError extends UndiciError {
+class BodyTimeoutError extends UndiciError {
   constructor (message) {
     super(message)
-    Error.captureStackTrace(this, SocketTimeoutError)
-    this.name = 'SocketTimeoutError'
-    this.message = message || 'Socket Timeout Error'
-    this.code = 'UND_ERR_SOCKET_TIMEOUT'
+    Error.captureStackTrace(this, BodyTimeoutError)
+    this.name = 'BodyTimeoutError'
+    this.message = message || 'Body Timeout Error'
+    this.code = 'UND_ERR_BODY_TIMEOUT'
   }
 }
 
@@ -140,8 +140,8 @@ class NotSupportedError extends UndiciError {
 
 module.exports = {
   UndiciError,
-  SocketTimeoutError,
   HeadersTimeoutError,
+  BodyTimeoutError,
   RequestTimeoutError,
   ContentLengthMismatchError,
   TrailerMismatchError,

lib/core/symbols.js

@@ -6,7 +6,6 @@ module.exports = {
   kConnect: Symbol('connect'),
   kResume: Symbol('resume'),
   kPause: Symbol('pause'),
-  kSocketTimeout: Symbol('socket timeout'),
   kIdleTimeout: Symbol('idle timeout'),
   kKeepAliveMaxTimeout: Symbol('max keep alive timeout'),
   kKeepAliveTimeoutThreshold: Symbol('keep alive timeout threshold'),
@@ -28,9 +27,9 @@ module.exports = {
   kParser: Symbol('parser'),
   kOnDestroyed: Symbol('destroy callbacks'),
   kPipelining: Symbol('pipelinig'),
-  kRetryDelay: Symbol('retry delay'),
   kSocketPath: Symbol('socket path'),
   kSocket: Symbol('socket'),
-  kRetryTimeout: Symbol('retry timeout'),
-  kTLSSession: Symbol('tls session cache')
+  kTLSSession: Symbol('tls session cache'),
+  kHostHeader: Symbol('host header'),
+  kBodyTimeout: Symbol('body timeout')
 }

test/client-errors.js

@@ -324,11 +324,11 @@ test('invalid options throws', (t) => {
 
   try {
     new Client(new URL('http://localhost:200'), { // eslint-disable-line
-      socketTimeout: 'asd'
+      bodyTimeout: 'asd'
     })
   } catch (err) {
     t.ok(err instanceof errors.InvalidArgumentError)
-    t.strictEqual(err.message, 'invalid socketTimeout')
+    t.strictEqual(err.message, 'invalid bodyTimeout')
   }
 
   try {

test/request-timeout.js

@@ -622,7 +622,7 @@ test('client.close should not deadlock', (t) => {
 
   server.listen(0, () => {
     const client = new Client(`http://localhost:${server.address().port}`, {
-      socketTimeout: 200
+      bodyTimeout: 200
     })
     t.teardown(client.destroy.bind(client))
 

test/socket-timeout.js

@@ -25,12 +25,12 @@ test('timeout with pipelining 1', (t) => {
   server.listen(0, () => {
     const client = new Client(`http://localhost:${server.address().port}`, {
       pipelining: 1,
-      socketTimeout: 500
+      bodyTimeout: 500
     })
     t.tearDown(client.close.bind(client))
 
     client.request({ path: '/', method: 'GET', opaque: 'asd' }, (err, data) => {
-      t.ok(err instanceof errors.SocketTimeoutError) // we are expecting an error
+      t.ok(err instanceof errors.BodyTimeoutError) // we are expecting an error
       t.strictEqual(data.opaque, 'asd')
     })
 
@@ -66,7 +66,7 @@ test('Disable socket timeout', (t) => {
 
   server.listen(0, () => {
     const client = new Client(`http://localhost:${server.address().port}`, {
-      socketTimeout: 0
+      bodyTimeout: 0
     })
     t.tearDown(client.close.bind(client))