perf: avoid intermediate promise on BodyReadable.dump (#4459)

* perf: avoid intermediate promise on `BodyReadable.dump`

* also test stacktraces

Author: Uzlopak

lib/api/readable.js

@@ -262,24 +262,26 @@ class BodyReadable extends Readable {
    * @param {AbortSignal} [opts.signal] An AbortSignal to cancel the dump.
    * @returns {Promise<null>}
    */
-  async dump (opts) {
+  dump (opts) {
     const signal = opts?.signal
 
     if (signal != null && (typeof signal !== 'object' || !('aborted' in signal))) {
-      throw new InvalidArgumentError('signal must be an AbortSignal')
+      return Promise.reject(new InvalidArgumentError('signal must be an AbortSignal'))
     }
 
     const limit = opts?.limit && Number.isFinite(opts.limit)
       ? opts.limit
       : 128 * 1024
 
-    signal?.throwIfAborted()
+    if (signal?.aborted) {
+      return Promise.reject(signal.reason ?? new AbortError())
+    }
 
     if (this._readableState.closeEmitted) {
-      return null
+      return Promise.resolve(null)
     }
 
-    return await new Promise((resolve, reject) => {
+    return new Promise((resolve, reject) => {
       if (
         (this[kContentLength] && (this[kContentLength] > limit)) ||
         this[kBytesRead] > limit

test/client-request.js

@@ -11,7 +11,7 @@ const { kConnect } = require('../lib/core/symbols')
 const { Readable } = require('node:stream')
 const net = require('node:net')
 const { promisify } = require('node:util')
-const { NotSupportedError, InvalidArgumentError } = require('../lib/core/errors')
+const { NotSupportedError, InvalidArgumentError, AbortError } = require('../lib/core/errors')
 const { parseFormDataString } = require('./utils/formdata')
 
 test('request dump head', async (t) => {
@@ -116,7 +116,80 @@ test('request dump', async (t) => {
 })
 
 test('request dump with abort signal', async (t) => {
-  t = tspl(t, { plan: 2 })
+  t = tspl(t, { plan: 10 })
+  const server = createServer({ joinDuplicateHeaders: true }, (req, res) => {
+    res.write('hello')
+  })
+  after(() => server.close())
+
+  server.listen(0, () => {
+    const client = new Client(`http://localhost:${server.address().port}`)
+    after(() => client.destroy())
+
+    client.request({
+      path: '/',
+      method: 'GET'
+    }, (err, { body }) => {
+      t.ifError(err)
+      const ac = new AbortController()
+      body.dump({ signal: ac.signal }).catch((err) => {
+        t.strictEqual(err.name, 'AbortError')
+        t.strictEqual(err.message, 'This operation was aborted')
+        const stackLines = err.stack.split('\n').map((l) => l.trim())
+
+        t.ok(stackLines[0].startsWith('AbortError: This operation was aborted'))
+        t.ok(stackLines[1].startsWith('at new DOMException'))
+        t.ok(stackLines[2].startsWith('at AbortController.abort'))
+        t.ok(/client-request.js/.test(stackLines[3]))
+        t.ok(stackLines[4].startsWith('at RequestHandler.runInAsyncScope'))
+        t.ok(stackLines[5].startsWith('at RequestHandler.onHeaders'))
+        t.ok(stackLines[6].startsWith('at Request.onHeaders'))
+        server.close()
+      })
+      ac.abort()
+    })
+  })
+
+  await t.completed
+})
+
+test('request dump with POJO as invalid signal', async (t) => {
+  t = tspl(t, { plan: 9 })
+  const server = createServer({ joinDuplicateHeaders: true }, (req, res) => {
+    res.write('hello')
+  })
+  after(() => server.close())
+
+  server.listen(0, () => {
+    const client = new Client(`http://localhost:${server.address().port}`)
+    after(() => client.destroy())
+
+    client.request({
+      path: '/',
+      method: 'GET'
+    }, (err, { body }) => {
+      t.ifError(err)
+      body.dump({ signal: {} }).catch((err) => {
+        t.strictEqual(err.name, 'InvalidArgumentError')
+        t.strictEqual(err.message, 'signal must be an AbortSignal')
+        const stackLines = err.stack.split('\n').map((l) => l.trim())
+
+        t.ok(stackLines[0].startsWith('InvalidArgumentError: signal must be an AbortSignal'))
+        t.ok(stackLines[1].startsWith('at BodyReadable.dump'))
+        t.ok(/client-request.js/.test(stackLines[2]))
+        t.ok(stackLines[3].startsWith('at RequestHandler.runInAsyncScope'))
+        t.ok(stackLines[4].startsWith('at RequestHandler.onHeaders'))
+        t.ok(stackLines[5].startsWith('at Request.onHeaders'))
+        server.close()
+      })
+    })
+  })
+
+  await t.completed
+})
+
+test('request dump with aborted signal', async (t) => {
+  t = tspl(t, { plan: 8 })
   const server = createServer({ joinDuplicateHeaders: true }, (req, res) => {
     res.write('hello')
   })
@@ -132,8 +205,18 @@ test('request dump with abort signal', async (t) => {
     }, (err, { body }) => {
       t.ifError(err)
       const ac = new AbortController()
+      ac.abort(new AbortError('This operation was with purpose aborted'))
+
       body.dump({ signal: ac.signal }).catch((err) => {
         t.strictEqual(err.name, 'AbortError')
+        t.strictEqual(err.message, 'This operation was with purpose aborted')
+        const stackLines = err.stack.split('\n').map((l) => l.trim())
+
+        t.ok(stackLines[0].startsWith('AbortError: This operation was with purpose aborted'))
+        t.ok(/client-request.js/.test(stackLines[1]))
+        t.ok(stackLines[2].startsWith('at RequestHandler.runInAsyncScope'))
+        t.ok(stackLines[3].startsWith('at RequestHandler.onHeaders'))
+        t.ok(stackLines[4].startsWith('at Request.onHeaders'))
         server.close()
       })
       ac.abort()