fix: count for error response and network errors (nodejs#2966)

* fix: count for error response and network errors * test: add specific test for fix * fix: conflict * refactor: address feedback
mertcanaltin · Mar 26, 2024 · c61a57d · c61a57d
1 parent 07647db
commit c61a57d
Show file tree

Hide file tree

Showing 4 changed files with 120 additions and 10 deletions.
diff --git a/docs/docs/api/RetryHandler.md b/docs/docs/api/RetryHandler.md
@@ -35,6 +35,12 @@ Extends: [`Dispatch.DispatchOptions`](Dispatcher.md#parameter-dispatchoptions).
 - `state`: `RetryState` - Current retry state. It can be mutated.
 - `opts`: `Dispatch.DispatchOptions & RetryOptions` - Options passed to the retry handler.
 
+**`RetryState`**
+
+It represents the retry state for a given request.
+
+- `counter`: `number` - Current retry attempt.
+
 ### Parameter `RetryHandlers`
 
 - **dispatch** `(options: Dispatch.DispatchOptions, handlers: Dispatch.DispatchHandlers) => Promise<Dispatch.DispatchResponse>` (required) - Dispatch function to be called after every retry.

diff --git a/lib/handler/retry-handler.js b/lib/handler/retry-handler.js
@@ -60,6 +60,7 @@ class RetryHandler {
     }
 
     this.retryCount = 0
+    this.retryCountCheckpoint = 0
     this.start = 0
     this.end = null
     this.etag = null
@@ -112,10 +113,7 @@ class RetryHandler {
       errorCodes,
       methods
     } = retryOptions
-    let { counter, currentTimeout } = state
-
-    currentTimeout =
-      currentTimeout != null && currentTimeout > 0 ? currentTimeout : minTimeout
+    const { counter } = state
 
     // Any code that is not a Undici's originated and allowed to retry
     if (
@@ -160,9 +158,7 @@ class RetryHandler {
     const retryTimeout =
       retryAfterHeader > 0
         ? Math.min(retryAfterHeader, maxTimeout)
-        : Math.min(currentTimeout * timeoutFactor ** counter, maxTimeout)
-
-    state.currentTimeout = retryTimeout
+        : Math.min(minTimeout * timeoutFactor ** (counter - 1), maxTimeout)
 
     setTimeout(() => cb(null), retryTimeout)
   }
@@ -310,10 +306,19 @@ class RetryHandler {
       return this.handler.onError(err)
     }
 
+    // We reconcile in case of a mix between network errors
+    // and server error response
+    if (this.retryCount - this.retryCountCheckpoint > 0) {
+      // We count the difference between the last checkpoint and the current retry count
+      this.retryCount = this.retryCountCheckpoint + (this.retryCount - this.retryCountCheckpoint)
+    } else {
+      this.retryCount += 1
+    }
+
     this.retryOpts.retry(
       err,
       {
-        state: { counter: this.retryCount++, currentTimeout: this.retryAfter },
+        state: { counter: this.retryCount },
         opts: { retryOptions: this.retryOpts, ...this.opts }
       },
       onRetry.bind(this)
@@ -335,6 +340,7 @@ class RetryHandler {
       }
 
       try {
+        this.retryCountCheckpoint = this.retryCount
         this.dispatch(this.opts, this)
       } catch (err) {
         this.handler.onError(err)

diff --git a/test/retry-handler.js b/test/retry-handler.js
@@ -17,7 +17,105 @@ test('Should retry status code', async t => {
   const dispatchOptions = {
     retryOptions: {
       retry: (err, { state, opts }, done) => {
-        counter++
+        ++counter
+
+        if (
+          err.statusCode === 500 ||
+          err.message.includes('other side closed')
+        ) {
+          setTimeout(done, 500)
+          return
+        }
+
+        return done(err)
+      }
+    },
+    method: 'GET',
+    path: '/',
+    headers: {
+      'content-type': 'application/json'
+    }
+  }
+
+  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: {
+        onConnect () {
+          t.ok(true, 'pass')
+        },
+        onBodySent () {
+          t.ok(true, 'pass')
+        },
+        onHeaders (status, _rawHeaders, resume, _statusMessage) {
+          t.strictEqual(status, 200)
+          return true
+        },
+        onData (chunk) {
+          chunks.push(chunk)
+          return true
+        },
+        onComplete () {
+          t.strictEqual(Buffer.concat(chunks).toString('utf-8'), 'hello world!')
+          t.strictEqual(counter, 2)
+        },
+        onError () {
+          t.fail()
+        }
+      }
+    })
+
+    after(async () => {
+      await client.close()
+      server.close()
+
+      await once(server, 'close')
+    })
+
+    client.dispatch(
+      {
+        method: 'GET',
+        path: '/',
+        headers: {
+          'content-type': 'application/json'
+        }
+      },
+      handler
+    )
+  })
+
+  await t.completed
+})
+
+test('Should account for network and response errors', async t => {
+  t = tspl(t, { plan: 4 })
+
+  let counter = 0
+  const chunks = []
+  const server = createServer()
+  const dispatchOptions = {
+    retryOptions: {
+      retry: (err, { state, opts }, done) => {
+        counter = state.counter
 
         if (
           err.statusCode === 500 ||

diff --git a/types/retry-handler.d.ts b/types/retry-handler.d.ts
@@ -12,7 +12,7 @@ declare class RetryHandler implements Dispatcher.DispatchHandlers {
 }
 
 declare namespace RetryHandler {
-  export type RetryState = { counter: number; currentTimeout: number };
+  export type RetryState = { counter: number; };
 
   export type RetryContext = {
     state: RetryState;