refactor: simplify busy implementation

README.md

@@ -23,11 +23,11 @@ Machine: 2.8GHz AMD EPYC 7402P<br/>
 Configuration: Node v14.4, HTTP/1.1 without TLS, 100 connections, Linux 5.4.12-1-lts
 
 ```
-http - keepalive x 5,634 ops/sec ±2.53% (274 runs sampled)
-undici - pipeline x 8,642 ops/sec ±3.08% (276 runs sampled)
-undici - request x 12,681 ops/sec ±0.51% (279 runs sampled)
-undici - stream x 14,006 ops/sec ±0.53% (280 runs sampled)
-undici - dispatch x 15,002 ops/sec ±0.39% (278 runs sampled)
+http - keepalive x 5,882 ops/sec ±1.87% (274 runs sampled)
+undici - pipeline x 9,189 ops/sec ±2.02% (272 runs sampled)
+undici - request x 12,623 ops/sec ±0.89% (277 runs sampled)
+undici - stream x 14,136 ops/sec ±0.61% (280 runs sampled)
+undici - dispatch x 14,883 ops/sec ±0.44% (281 runs sampled)
 ```
 
 The benchmark is a simple `hello world` [example](benchmarks/index.js).

lib/core/client.js

@@ -213,35 +213,7 @@ class Client extends EventEmitter {
   }
 
   get busy () {
-    // TODO: ignore aborted requests.
-
-    if (this.size >= this[kPipelining]) {
-      return true
-    }
-
-    if (this.size && !this.connected) {
-      return true
-    }
-
-    if (this[kReset] || this[kWriting]) {
-      return true
-    }
-
-    if (this[kResuming]) {
-      for (let n = this[kPendingIdx]; n < this[kQueue].length; n++) {
-        const { idempotent, body, reset } = this[kQueue][n]
-        if (!idempotent || reset) {
-          return true
-        }
-        if (util.isStream(body) && util.bodyLength(body) !== 0) {
-          return true
-        }
-      }
-    } else if (this.pending > 0) {
-      return true
-    }
-
-    return false
+    return this[kReset] || this[kWriting] || this.pending > 0
   }
 
   get destroyed () {
@@ -938,7 +910,22 @@ function _resume (client) {
 }
 
 function write (client, request) {
-  const { body, header } = request
+  const { body, header, method, upgrade } = request
+
+  // https://tools.ietf.org/html/rfc7231#section-4.3.1
+  // https://tools.ietf.org/html/rfc7231#section-4.3.2
+  // https://tools.ietf.org/html/rfc7231#section-4.3.5
+
+  // Sending a payload body on a request that does not
+  // expect it can cause undefined behavior on some
+  // servers and corrupt connection state. Do not
+  // re-use the connection for further requests.
+
+  const expectsPayload = (
+    method === 'PUT' ||
+    method === 'POST' ||
+    method === 'PATCH'
+  )
 
   if (body && typeof body.read === 'function') {
     // Try to read EOF in order to get length.
@@ -951,7 +938,7 @@ function write (client, request) {
     contentLength = request.contentLength
   }
 
-  if (contentLength === 0 && !request.expectsPayload) {
+  if (contentLength === 0 && !expectsPayload) {
     // https://tools.ietf.org/html/rfc7230#section-3.3.2
     // A user agent SHOULD NOT send a Content-Length header field when
     // the request message does not contain a payload body and the method
@@ -976,7 +963,19 @@ function write (client, request) {
     return false
   }
 
-  if (request.reset) {
+  if (method === 'HEAD') {
+    // https://github.com/mcollina/undici/issues/258
+
+    // Close after a HEAD request to interop with misbehaving servers
+    // that may send a body in the response.
+
+    client[kReset] = true
+  }
+
+  if (method === 'CONNECT' || upgrade) {
+    // On CONNECT or upgrade, block pipeline from dispatching further
+    // requests on this connection.
+
     client[kReset] = true
   }
 
@@ -1004,6 +1003,10 @@ function write (client, request) {
     socket.write('\r\n', 'ascii')
     socket.uncork()
 
+    if (!expectsPayload) {
+      client[kReset] = true
+    }
+
     request.body = null
   } else {
     client[kWriting] = true
@@ -1030,6 +1033,10 @@ function write (client, request) {
       }
 
       if (bytesWritten === 0) {
+        if (!expectsPayload) {
+          client[kReset] = true
+        }
+
         if (contentLength === null) {
           socket.write(`${header}transfer-encoding: chunked\r\n`, 'ascii')
         } else {
@@ -1094,7 +1101,7 @@ function write (client, request) {
       }
 
       if (bytesWritten === 0) {
-        if (request.expectsPayload) {
+        if (expectsPayload) {
           // https://tools.ietf.org/html/rfc7230#section-3.3.2
           // A user agent SHOULD send a Content-Length in a request message when
           // no Transfer-Encoding is sent and the request method defines a meaning

lib/core/request.js

@@ -191,48 +191,6 @@ class Request {
     this[kResume] = resume
   }
 
-  get expectsPayload () {
-    const { method } = this
-    return (
-      method === 'PUT' ||
-      method === 'POST' ||
-      method === 'PATCH'
-    )
-  }
-
-  get reset () {
-    const { method, upgrade, body } = this
-
-    if (method === 'HEAD') {
-      // https://github.com/mcollina/undici/issues/258
-
-      // Close after a HEAD request to interop with misbehaving servers
-      // that may send a body in the response.
-
-      return true
-    }
-
-    if (method === 'CONNECT' || upgrade) {
-      // On CONNECT or upgrade, block pipeline from dispatching further
-      // requests on this connection.
-      return true
-    }
-
-    if (body && !this.expectsPayload && util.bodyLength(body) !== 0) {
-      // https://tools.ietf.org/html/rfc7231#section-4.3.1
-      // https://tools.ietf.org/html/rfc7231#section-4.3.2
-      // https://tools.ietf.org/html/rfc7231#section-4.3.5
-
-      // Sending a payload body on a request that does not
-      // expect it can cause undefined behavior on some
-      // servers and corrupt connection state. Do not
-      // re-use the connection for further requests.
-      return true
-    }
-
-    return false
-  }
-
   onConnect () {
     assert(!this.aborted)
 

test/client-pipelining.js

@@ -423,7 +423,7 @@ test('pipelining HEAD busy', (t) => {
             })
         })
         body.push(null)
-        t.strictEqual(client.busy, false)
+        t.strictEqual(client.busy, true)
       }
 
       {
@@ -568,7 +568,7 @@ test('pipelining idempotent busy', (t) => {
             })
         })
         body.push(null)
-        t.strictEqual(client.busy, false)
+        t.strictEqual(client.busy, true)
       }
 
       {

test/pipeline-pipelining.js

@@ -30,7 +30,7 @@ test('pipeline pipelining', (t) => {
         method: 'GET',
         path: '/'
       }, ({ body }) => body).end().resume()
-      t.strictEqual(client.busy, false)
+      t.strictEqual(client.busy, true)
       t.strictDeepEqual(client.running, 0)
       t.strictDeepEqual(client.pending, 1)
 
@@ -82,15 +82,15 @@ test('pipeline pipelining retry', (t) => {
         .on('error', (err) => {
           t.ok(err)
         })
-      t.strictEqual(client.busy, false)
+      t.strictEqual(client.busy, true)
       t.strictDeepEqual(client.running, 0)
       t.strictDeepEqual(client.pending, 1)
 
       client.pipeline({
         method: 'GET',
         path: '/'
       }, ({ body }) => body).end().resume()
-      t.strictEqual(client.busy, false)
+      t.strictEqual(client.busy, true)
       t.strictDeepEqual(client.running, 0)
       t.strictDeepEqual(client.pending, 2)