chore: dump on abort by default

nodejs · May 12, 2024 · 0d67095 · 0d67095
1 parent fb4ccd8
commit 0d67095
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 147 deletions.
diff --git a/docs/docs/api/Dispatcher.md b/docs/docs/api/Dispatcher.md
@@ -958,7 +958,6 @@ The `dump` interceptor enables you to dump the response body from a request upon
 
 **Options**
 - `maxSize` - The maximum size (in bytes) of the response body to dump. If the size of the request's body exceeds this value then the connection will be closed. Default: `1048576`.
-- `abortOnDumped` - States whether or not abort the request after the response's body being dumped. Default: `true`.
 
 > The `Dispatcher#options` also gets extended with the options `dumpMaxSize`, `abortOnDumped`, and `waitForTrailers` which can be used to configure the interceptor at a request-per-request basis.
 

diff --git a/lib/interceptor/dump.js b/lib/interceptor/dump.js
@@ -6,41 +6,34 @@ const DecoratorHandler = require('../handler/decorator-handler')
 
 class DumpHandler extends DecoratorHandler {
   #maxSize = 1024 * 1024
+  #timeout = 30e3 // 30s
   #abort = null
-  #dumpOnAbort = true
   #dumped = false
   #aborted = false
   #size = 0
   #reason = null
   #handler = null
 
-  constructor ({ maxSize, dumpOnAbort }, handler) {
+  constructor ({ maxSize, timeout }, handler) {
     super(handler)
 
     if (maxSize != null && (!Number.isFinite(maxSize) || maxSize < 1)) {
       throw new InvalidArgumentError('maxSize must be a number greater than 0')
     }
 
-    if (dumpOnAbort != null && typeof dumpOnAbort !== 'boolean') {
-      throw new InvalidArgumentError('dumpOnAbort must be a boolean')
+    if (timeout != null && (!Number.isFinite(timeout) || timeout < 1)) {
+      throw new InvalidArgumentError('timeout must be a number greater than 0')
     }
 
     this.#maxSize = maxSize ?? this.#maxSize
-    this.#dumpOnAbort = dumpOnAbort ?? this.#dumpOnAbort
+    this.#timeout = timeout ?? this.#timeout
     this.#handler = handler
   }
 
   onConnect (abort) {
-    if (this.#aborted) {
-      abort(this.#reason)
-      return
-    }
-
     this.#abort = abort
 
-    this.#handler.onConnect(
-      this.#dumpOnAbort === true ? this.#customAbort.bind(this) : this.#abort
-    )
+    this.#handler.onConnect(this.#customAbort.bind(this))
   }
 
   #customAbort (reason) {
@@ -61,6 +54,10 @@ class DumpHandler extends DecoratorHandler {
       )
     }
 
+    if (this.#aborted) {
+      return true
+    }
+
     return this.#handler.onHeaders(
       statusCode,
       rawHeaders,
@@ -70,14 +67,13 @@ class DumpHandler extends DecoratorHandler {
   }
 
   onError (err) {
-    if (!this.#dumped) {
-      if (this.#aborted) {
-        this.#handler.onError(this.reason)
-        return
-      }
-
-      this.#handler.onError(err)
+    if (this.#dumped) {
+      return
     }
+
+    err = this.#reason ?? err
+
+    this.#handler.onError(err)
   }
 
   onData (chunk) {
@@ -97,21 +93,23 @@ class DumpHandler extends DecoratorHandler {
   }
 
   onComplete (trailers) {
-    if (!this.#dumped) {
-      if (this.#aborted) {
-        this.#handler.onError(this.reason)
-        return
-      }
+    if (this.#dumped) {
+      return
+    }
 
-      this.#handler.onComplete(trailers)
+    if (this.#aborted) {
+      this.#handler.onError(this.reason)
+      return
     }
+
+    this.#handler.onComplete(trailers)
   }
 }
 
 function createDumpInterceptor (
   { maxSize: defaultMaxSize, dumpOnAbort: defaultDumpOnAbort } = {
     maxSize: 1024 * 1024,
-    dumpOnAbort: true
+    timeout: 30e3 // 30s
   }
 ) {
   return dispatch => {

diff --git a/test/interceptors/dump-interceptor.js b/test/interceptors/dump-interceptor.js
@@ -14,18 +14,29 @@ if (platform() === 'win32') {
   process.exit(0)
 }
 
-test('Should not dump on abort', async t => {
-  t = tspl(t, { plan: 4 })
+test('Should dump on abort', async t => {
+  t = tspl(t, { plan: 2 })
+  let offset = 0
   const server = createServer((req, res) => {
     const max = 1024 * 1024
     const buffer = Buffer.alloc(max)
 
     res.writeHead(200, {
-      'Content-Length': buffer.length,
       'Content-Type': 'application/octet-stream'
     })
 
-    res.write(buffer)
+    const interval = setInterval(() => {
+      offset += 256
+      const chunk = buffer.subarray(offset - 256, offset)
+
+      if (offset === max) {
+        clearInterval(interval)
+        res.end(chunk)
+        return
+      }
+
+      res.write(chunk)
+    }, 0)
   })
 
   const abc = new AbortController()
@@ -42,7 +53,7 @@ test('Should not dump on abort', async t => {
 
   const client = new Client(
     `http://localhost:${server.address().port}`
-  ).compose(dump({ dumpOnAbort: false }))
+  ).compose(dump({ maxSize: 512 }))
 
   after(async () => {
     await client.close()
@@ -52,32 +63,34 @@ test('Should not dump on abort', async t => {
   })
 
   const response = await client.request(requestOptions)
-  t.equal(response.statusCode, 200)
-  t.equal(response.headers['content-length'], `${1024 * 1024}`)
+
   abc.abort()
 
   try {
     await response.body.text()
-    t.fail('Should not reach this point')
   } catch (error) {
+    t.equal(response.statusCode, 200)
     t.equal(error.name, 'AbortError')
-    t.equal(error.message, 'This operation was aborted')
   }
 
   await t.completed
 })
 
-test('Should dump on abort', async t => {
+test('Should dump on already aborted request', async t => {
   t = tspl(t, { plan: 2 })
   let offset = 0
   const server = createServer((req, res) => {
-    const max = 1024 * 1024
+    const max = 1024
     const buffer = Buffer.alloc(max)
 
     res.writeHead(200, {
       'Content-Type': 'application/octet-stream'
     })
 
+    res.once('close', () => {
+      t.equal(offset, 1024)
+    })
+
     const interval = setInterval(() => {
       offset += 256
       const chunk = buffer.subarray(offset - 256, offset)
@@ -115,16 +128,9 @@ test('Should dump on abort', async t => {
     await once(server, 'close')
   })
 
-  const response = await client.request(requestOptions)
-
   abc.abort()
-
-  try {
-    await response.body.text()
-  } catch (error) {
-    t.equal(response.statusCode, 200)
-    t.equal(error.name, 'AbortError')
-  }
+  const response = await client.request(requestOptions)
+  t.equal(response.statusCode, undefined)
 
   await t.completed
 })
@@ -251,105 +257,8 @@ test('Should forward common error', async t => {
 })
 
 test('Should throw on bad opts', async t => {
-  t = tspl(t, { plan: 12 })
+  t = tspl(t, { plan: 6 })
 
-  t.throws(
-    () => {
-      new Client('http://localhost')
-        .compose(dump({ dumpOnAbort: {} }))
-        .dispatch(
-          {
-            method: 'GET',
-            path: '/'
-          },
-          {}
-        )
-    },
-    {
-      name: 'InvalidArgumentError',
-      message: 'dumpOnAbort must be a boolean'
-    }
-  )
-  t.throws(
-    () => {
-      new Client('http://localhost')
-        .compose(dump({ dumpOnAbort: 'true' }))
-        .dispatch(
-          {
-            method: 'GET',
-            path: '/'
-          },
-          {}
-        )
-    },
-    {
-      name: 'InvalidArgumentError',
-      message: 'dumpOnAbort must be a boolean'
-    }
-  )
-  t.throws(
-    () => {
-      new Client('http://localhost').compose(dump({ dumpOnAbort: 1 })).dispatch(
-        {
-          method: 'GET',
-          path: '/'
-        },
-        {}
-      )
-    },
-    {
-      name: 'InvalidArgumentError',
-      message: 'dumpOnAbort must be a boolean'
-    }
-  )
-  t.throws(
-    () => {
-      new Client('http://localhost').compose(dump()).dispatch(
-        {
-          method: 'GET',
-          path: '/',
-          dumpOnAbort: {}
-        },
-        {}
-      )
-    },
-    {
-      name: 'InvalidArgumentError',
-      message: 'dumpOnAbort must be a boolean'
-    }
-  )
-  t.throws(
-    () => {
-      new Client('http://localhost').compose(dump()).dispatch(
-        {
-          method: 'GET',
-          path: '/',
-          dumpOnAbort: 'false'
-        },
-        {}
-      )
-    },
-    {
-      name: 'InvalidArgumentError',
-      message: 'dumpOnAbort must be a boolean'
-    }
-  )
-  t.throws(
-    () => {
-      new Client('http://localhost').compose(dump()).dispatch(
-        {
-          method: 'GET',
-          path: '/',
-          dumpOnAbort: '0'
-        },
-        {}
-      )
-    },
-    {
-      name: 'InvalidArgumentError',
-      message: 'dumpOnAbort must be a boolean'
-    }
-  )
   t.throws(
     () => {
       new Client('http://localhost').compose(dump({ maxSize: {} })).dispatch(

diff --git a/types/interceptors.d.ts b/types/interceptors.d.ts
@@ -1,7 +1,7 @@
 import Dispatcher from "./dispatcher";
 import RetryHandler from "./retry-handler";
 
-export type DumpInterceptorOpts = { maxSize?: number, abortOnDumped?: boolean }
+export type DumpInterceptorOpts = { maxSize?: number }
 export type RetryInterceptorOpts = RetryHandler.RetryOptions
 export type RedirectInterceptorOpts = { maxRedirections?: number }