refactor: apply reviews

docs/api/RetryHandler.md

@@ -8,7 +8,7 @@ A handler class that implements the retry logic for a request.
 
 Arguments:
 
-- **options** `Dispatch.DispatchOptions & RetryOptions` (required) - It is an intersection of `Dispatcher.DispatchOptions`  and `RetryOptions`.
+- **options** `Dispatch.DispatchOptions & RetryOptions` (required) - It is an intersection of `Dispatcher.DispatchOptions` and `RetryOptions`.
 - **retryHandlers** `RetryHandlers` (required) - Object containing the `dispatch` to be used on every retry, and `handler` for handling the `dispatch` lifecycle.
 
 Returns: `retryHandler`
@@ -19,7 +19,7 @@ Extends: [`Dispatch.DispatchOptions`](Dispatcher.md#parameter-dispatchoptions).
 
 #### `RetryOptions`
 
-- **retry** `(err: Error, context: RetryContext, done: (result?: boolean) => void) => void` (optional) - Function to be called after every retry. It should call the `done` parameter to try the request once more.
+- **retry** `(err: Error, context: RetryContext, callback: (err?: Error | null) => void) => void` (optional) - Function to be called after every retry. It should pass error if no more retries should be performed.
 - **maxRetries** `number` (optional) - Maximum number of retries. Default: `5`
 - **maxTimeout** `number` (optional) - Maximum number of milliseconds to wait before retrying. Default: `30000` (30 seconds)
 - **minTimeout** `number` (optional) - Minimum number of milliseconds to wait before retrying. Default: `500` (half a second)
@@ -28,8 +28,7 @@ Extends: [`Dispatch.DispatchOptions`](Dispatcher.md#parameter-dispatchoptions).
 -
 - **methods** `string[]` (optional) - Array of HTTP methods to retry. Default: `['GET', 'PUT', 'HEAD', 'OPTIONS', 'DELETE']`
 - **statusCodes** `number[]` (optional) - Array of HTTP status codes to retry. Default: `[429, 500, 502, 503, 504]`
-- **errorCodes** `string[]` (optional) - Array of Error codes to retry. Default: `['ECONNRESET', 'ECONNREFUSED', 'ENOTFOUND', 'ENETDOWN','ENETUNREACH', 'EHOSTDOWN', 
-
+- **errorCodes** `string[]` (optional) - Array of Error codes to retry. Default: `['ECONNRESET', 'ECONNREFUSED', 'ENOTFOUND', 'ENETDOWN','ENETUNREACH', 'EHOSTDOWN',
 
 **`RetryContext`**
 
@@ -51,18 +50,22 @@ const handler = new RetryHandler(
     ...dispatchOptions,
     retryOptions: {
       // custom retry function
-      retry: function (err, state, done) {
+      retry: function (err, state, callback) {
         counter++;
 
         if (err.code && err.code === "UND_ERR_DESTROYED") {
-          return null;
+          callback(err);
+          return;
+        }
+
+        if (err.statusCode === 206) {
+          callback(err);
+          return;
         }
 
-        if (err.statusCode === 206) return done(false);
-
-        setTimeout(done, 800);
+        setTimeout(() => callback(null), 1000);
       },
-    }
+    },
   },
   {
     dispatch: (...args) => {
@@ -83,11 +86,10 @@ const handler = new RetryHandler(
         // handle error properly
       },
     },
-  },
+  }
 );
 ```
 
-
 #### Example - Basic RetryHandler with defaults
 
 ```js

lib/handler/RetryHandler.js

@@ -98,7 +98,7 @@ class RetryHandler {
     return this.handler.onBodySent(chunk)
   }
 
-  static [kRetryHandlerDefaultRetry] (err, { state, opts }, done) {
+  static [kRetryHandlerDefaultRetry] (err, { state, opts }, cb) {
     const { statusCode, code, headers } = err
     const { method, retryOptions } = opts
     const {
@@ -122,23 +122,31 @@ class RetryHandler {
       code !== 'UND_ERR_SOCKET' &&
       !errorCodes.includes(code)
     ) {
-      return done(false)
+      cb(err)
+      return
     }
 
     // If a set of method are provided and the current method is not in the list
-    if (Array.isArray(methods) && !methods.includes(method)) return done(false)
+    if (Array.isArray(methods) && !methods.includes(method)) {
+      cb(err)
+      return
+    }
 
     // If a set of status code are provided and the current status code is not in the list
     if (
       statusCode != null &&
       Array.isArray(statusCodes) &&
       !statusCodes.includes(statusCode)
     ) {
-      return done(false)
+      cb(err)
+      return
     }
 
     // If we reached the max number of retries
-    if (counter > maxRetries) return done(false)
+    if (counter > maxRetries) {
+      cb(err)
+      return
+    }
 
     let retryAfterHeader = headers != null && headers['retry-after']
     if (retryAfterHeader) {
@@ -155,15 +163,15 @@ class RetryHandler {
 
     state.currentTimeout = retryTimeout
 
-    setTimeout(done, retryTimeout)
+    setTimeout(() => cb(null), retryTimeout)
   }
 
   onHeaders (statusCode, rawHeaders, resume, statusMessage) {
     const headers = parseHeaders(rawHeaders)
 
     this.retryCount += 1
 
-    if (statusCode > 299) {
+    if (statusCode >= 300) {
       this.abort(
         new RequestRetryError('Request failed', statusCode, {
           headers,
@@ -182,12 +190,8 @@ class RetryHandler {
       }
 
       const contentRange = parseRangeHeader(headers['content-range'])
-      if (
-        // Let's start with a weak etag check
-        (this.etag != null && this.etag !== headers.etag) ||
-        // If no content range
-        !contentRange
-      ) {
+      // If no content range
+      if (!contentRange) {
         this.abort(
           new RequestRetryError('Content-Range mismatch', statusCode, {
             headers,
@@ -197,6 +201,17 @@ class RetryHandler {
         return false
       }
 
+      // Let's start with a weak etag check
+      if (this.etag != null && this.etag !== headers.etag) {
+        this.abort(
+          new RequestRetryError('ETag mismatch', statusCode, {
+            headers,
+            count: this.retryCount
+          })
+        )
+        return false
+      }
+
       const { start, size, end = size } = contentRange
 
       assert(this.start === start, 'content-range mismatch')
@@ -317,8 +332,8 @@ class RetryHandler {
       onRetry.bind(this)
     )
 
-    function onRetry (next) {
-      if (next === false || this.aborted || isDisturbed(this.opts.body)) {
+    function onRetry (err) {
+      if (err != null || this.aborted || isDisturbed(this.opts.body)) {
         return this.handler.onError(err)
       }
 

test/retry-handler.js

@@ -23,7 +23,7 @@ tap.test('Should retry status code', t => {
           return
         }
 
-        return done(false)
+        return done(err)
       }
     },
     method: 'GET',
@@ -388,7 +388,7 @@ tap.test('Should handle 206 partial content', t => {
           return done(false)
         }
 
-        if (err.statusCode === 206) done(false)
+        if (err.statusCode === 206) return done(err)
 
         setTimeout(done, 800)
       }
@@ -520,15 +520,6 @@ tap.test('Should handle 206 partial content - bad-etag', t => {
             t.equal(err.code, 'UND_ERR_REQ_RETRY')
           }
         }
-      },
-      {
-        retry: function (err) {
-          if (err.code && err.code === 'UND_ERR_DESTROYED') {
-            return null
-          }
-
-          return err.statusCode === 206 ? null : 800
-        }
       }
     )
 

types/retry-handler.d.ts

@@ -14,6 +14,15 @@ declare class RetryHandler implements Dispatcher.DispatchHandlers {
 declare namespace RetryHandler {
   export type RetryState = { counter: number; currentTimeout: number };
 
+  export type RetryContext = {
+    state: RetryState;
+    opts: Dispatcher.DispatchOptions & {
+      retryOptions?: RetryHandler.RetryOptions;
+    };
+  }
+
+  export type OnRetryCallback = (result?: Error | null) => void;
+
   export type RetryCallback = (
     err: Error,
     context: {
@@ -22,7 +31,7 @@ declare namespace RetryHandler {
         retryOptions?: RetryHandler.RetryOptions;
       };
     },
-    done: (result?: boolean) => void
+    callback: OnRetryCallback
   ) => number | null;
 
   export interface RetryOptions {