Make call to onBodySent conditional in RetryHandler (#2478)

MzUgM · web-flow · commit ed15e984df16 · 2023-11-29T22:29:45.000-05:00
* failing test

RequestHandler does not have `onBodySent` but `RetryHandler` always
sends to `onBodySent` which causes a problem for users who use
`RetryHandler` via `request()`

* is this a fix?

* refactor: clean up test case
diff --git a/lib/api/api-request.js b/lib/api/api-request.js
@@ -177,3 +177,4 @@ function request (opts, callback) {
 }
 
 module.exports = request
+module.exports.RequestHandler = RequestHandler
diff --git a/lib/handler/RetryHandler.js b/lib/handler/RetryHandler.js
@@ -95,7 +95,7 @@ class RetryHandler {
   }
 
   onBodySent (chunk) {
-    return this.handler.onBodySent(chunk)
+    if (this.handler.onBodySent) return this.handler.onBodySent(chunk)
   }
 
   static [kRetryHandlerDefaultRetry] (err, { state, opts }, cb) {
diff --git a/test/retry-handler.js b/test/retry-handler.js
@@ -5,6 +5,7 @@ const { once } = require('node:events')
 const tap = require('tap')
 
 const { RetryHandler, Client } = require('..')
+const { RequestHandler } = require('../lib/api/api-request')
 
 tap.test('Should retry status code', t => {
   let counter = 0
@@ -542,3 +543,80 @@ tap.test('Should handle 206 partial content - bad-etag', t => {
     })
   })
 })
+
+tap.test('retrying a request with a body', t => {
+  let counter = 0
+  const server = createServer()
+  const dispatchOptions = {
+    retryOptions: {
+      retry: (err, { state, opts }, done) => {
+        counter++
+
+        if (
+          err.statusCode === 500 ||
+          err.message.includes('other side closed')
+        ) {
+          setTimeout(done, 500)
+          return
+        }
+
+        return done(err)
+      }
+    },
+    method: 'POST',
+    path: '/',
+    headers: {
+      'content-type': 'application/json'
+    },
+    body: JSON.stringify({ hello: 'world' })
+  }
+
+  t.plan(1)
+
+  server.on('request', (req, res) => {
+    switch (counter) {
+      case 0:
+        req.destroy()
+        return
+      case 1:
+        res.writeHead(500)
+        res.end('failed')
+        return
+      case 2:
+        res.writeHead(200)
+        res.end('hello world!')
+        return
+      default:
+        t.fail()
+    }
+  })
+
+  server.listen(0, () => {
+    const client = new Client(`http://localhost:${server.address().port}`)
+    const handler = new RetryHandler(dispatchOptions, {
+      dispatch: client.dispatch.bind(client),
+      handler: new RequestHandler(dispatchOptions, (err, data) => {
+        t.error(err)
+      })
+    })
+
+    t.teardown(async () => {
+      await client.close()
+      server.close()
+
+      await once(server, 'close')
+    })
+
+    client.dispatch(
+      {
+        method: 'POST',
+        path: '/',
+        headers: {
+          'content-type': 'application/json'
+        },
+        body: JSON.stringify({ hello: 'world' })
+      },
+      handler
+    )
+  })
+})

Original file line number	Diff line number	Diff line change
`@@ -177,3 +177,4 @@ function request (opts, callback) {`
`177`	`177`	`}`
`178`	`178`
`179`	`179`	`module.exports = request`
	`180`	`+module.exports.RequestHandler = RequestHandler`
Original file line number	Diff line number	Diff line change
`@@ -95,7 +95,7 @@ class RetryHandler {`
`95`	`95`	`}`
`96`	`96`
`97`	`97`	`onBodySent (chunk) {`
`98`		`- return this.handler.onBodySent(chunk)`
	`98`	`+ if (this.handler.onBodySent) return this.handler.onBodySent(chunk)`
`99`	`99`	`}`
`100`	`100`
`101`	`101`	`static [kRetryHandlerDefaultRetry] (err, { state, opts }, cb) {`