FEATURE-RELEASE: Support for async retryOnHttpResponse and retryOnHtt…

…pError (#74)

Co-authored-by: Jamie Delbick <[email protected]>

README.md

@@ -58,8 +58,8 @@ All the retry options are configurable and can be set in `retryOptions` in the `
 | `retryMaxDuration` | Number | time in milliseconds to retry until throwing an error | `NODE_FETCH_RETRY_MAX_RETRY` | 60000 ms |
 | `retryInitialDelay` | Number | time in milliseconds to wait between retries |`NODE_FETCH_RETRY_INITIAL_WAIT` | 100 ms |
 | `retryBackoff` | Number | backoff factor for wait time between retries | `NODE_FETCH_RETRY_BACKOFF` | 2.0 |
-| `retryOnHttpResponse` | Function | a *function* determining whether to retry given the HTTP response | none | retry on all 5xx errors|
-| `retryOnHttpError` | Function | a *function* determining whether to retry given the HTTP error exception thrown | none | retry on all `FetchError`'s of type `system`|
+| `retryOnHttpResponse` | Function | a *function* determining whether to retry given the HTTP response. Can be asynchronous | none | retry on all 5xx errors|
+| `retryOnHttpError` | Function | a *function* determining whether to retry given the HTTP error exception thrown. Can be asynchronous | none | retry on all `FetchError`'s of type `system`|
 | `socketTimeout` | Number | time until socket timeout in milliseconds. _Note: if `socketTimeout` is >= `retryMaxDuration`, it will automatically adjust the socket timeout to be exactly half of the `retryMaxDuration`. To disable this feature, see `forceSocketTimeout` below_ | `NODE_FETCH_RETRY_SOCKET_TIMEOUT` | 30000 ms |
 | `forceSocketTimeout` | Boolean | If true, socket timeout will be forced to use `socketTimeout` property declared regardless of the `retryMaxDuration`. _Note: this feature was designed to help with unit testing and is not intended to be used in practice_ | `NODE_FETCH_RETRY_FORCE_TIMEOUT` | false |
 
@@ -166,7 +166,7 @@ Disabling retry behavior will not prevent the usage of other options set on the
 
 ### Additional notes on retry duration
 
-If the fetch is unsucessful, the retry logic determines how long it will wait before the next attempt.  If the time remaining will exceed the total time allowed by retryMaxDuration then another attempt will not be made.  There are examples of how this works in the testing code.
+If the fetch is unsuccessful, the retry logic determines how long it will wait before the next attempt.  If the time remaining will exceed the total time allowed by retryMaxDuration then another attempt will not be made.  There are examples of how this works in the testing code.
 
 ### Apache OpenWhisk Action Support
 

index.js

@@ -40,14 +40,18 @@ function isResponseTimedOut(retryOptions) {
  * @param {Object} error error object if the fetch request returned an error
  * @param {Object} response fetch call response
  * @param {Number} wait Amount of time we will wait before retrying next
- * @returns {Boolean} whether or not to retry the request
+ * @returns {Promise<Boolean>} whether or not to retry the request
  */
-function shouldRetry(retryOptions, error, response, waitTime) {
+async function shouldRetry(retryOptions, error, response, waitTime) {
     if (getTimeRemaining(retryOptions) < waitTime) {
         return false;
     } else if (retryOptions && retryOptions.retryOnHttpError && error != null) {
+        // retryOnHttpError can be sync or async because either the promise or result will be
+        // bubbled up to what shouldRetry returns
         return retryOptions.retryOnHttpError(error);
     } else if (retryOptions && retryOptions.retryOnHttpResponse) {
+        // retryOnHttpResponse can be sync or async because either the promise or result will be
+        // bubbled up to what shouldRetry returns
         return retryOptions.retryOnHttpResponse(response);
     } else {
         return false;
@@ -213,15 +217,15 @@ module.exports = async function (url, options) {
                 try {
                     const response = await fetch(url, options);
 
-                    if (shouldRetry(retryOptions, null, response, waitTime)) {
+                    if (await shouldRetry(retryOptions, null, response, waitTime)) {
                         console.error(`Retrying in ${waitTime} milliseconds, attempt ${attempt} failed (status ${response.status}): ${response.statusText}`);
                     } else {
                         // response.timeout should reflect the actual timeout
                         response.timeout = retryOptions.socketTimeout;
                         return resolve(response);
                     }
                 } catch (error) {
-                    if (!shouldRetry(retryOptions, error, null, waitTime)) {
+                    if (!(await shouldRetry(retryOptions, error, null, waitTime))) {
                         if (error.name === 'AbortError') {
                             return reject(new FetchError(`network timeout at ${url}`, 'request-timeout'));
                         } else {

test/index.test.js

@@ -126,6 +126,37 @@ describe('test `retryInit` function', () => {
         assert.strictEqual(retryOptions.socketTimeout, 2000);
     });
 
+    it('pass in custom parameters with async functions', async () => {
+        const rewiredFetchRetry = rewire('../index');
+        const retryInit = rewiredFetchRetry.__get__('retryInit');
+        const retryOptions = retryInit({
+            retryOptions: {
+                retryMaxDuration: 3000,
+                retryInitialDelay: 200,
+                retryBackoff: 3.0,
+                retryOnHttpResponse: async () => {
+                    return Promise.resolve(false);
+                },
+                retryOnHttpError: async () => {
+                    return Promise.resolve(false);
+                },
+                socketTimeout: 2000
+            }
+        });
+        assert.strictEqual(typeof retryOptions.startTime, 'number');
+        assert.strictEqual(retryOptions.retryMaxDuration, 3000);
+        assert.strictEqual(retryOptions.retryInitialDelay, 200);
+        assert.strictEqual(retryOptions.retryBackoff, 3);
+        assert.strictEqual(typeof retryOptions.retryOnHttpResponse, 'function');
+        assert.strictEqual(await retryOptions.retryOnHttpResponse({ status: 500 }), false);
+        assert.strictEqual(await retryOptions.retryOnHttpResponse({ status: 400 }), false);
+        assert.strictEqual(await retryOptions.retryOnHttpError(new FetchError('hello!', 'non-system')), false);
+        assert.strictEqual(await retryOptions.retryOnHttpError(new FetchError('hello!', 'system')), false);
+        assert.strictEqual(await retryOptions.retryOnHttpError(new FetchError('ECONNRESET', 'system')), false);
+        assert.strictEqual(await retryOptions.retryOnHttpError(new Error('ECONNRESET', 'system')), false);
+        assert.strictEqual(retryOptions.socketTimeout, 2000);
+    });
+
     it('pass in custom parameters and set enviroment variables, passed parameters take priority', () => {
         const rewiredFetchRetry = rewire('../index');
         const retryInit = rewiredFetchRetry.__get__('retryInit');
@@ -242,6 +273,29 @@ describe('test `retryInit` function', () => {
         assert.strictEqual(retryOptions.retryOnHttpError(new Error('ECONNRESET', 'system')), false);
         assert.strictEqual(retryOptions.retryOnHttpError(new SpecialError('error!')), true);
     });
+
+    it('custom async retry on http response', async () => {
+        const rewiredFetchRetry = rewire('../index');
+        const retryInit = rewiredFetchRetry.__get__('retryInit');
+        const retryOptions = retryInit({
+            retryOptions: {
+                retryOnHttpError: async (error) => {
+                    return Promise.resolve(error.name === 'SpecialError' || false);
+                }
+            }
+        });
+        class SpecialError extends Error {
+            constructor(message) {
+                super(message);
+                this.name = 'SpecialError';
+            }
+        }
+        assert.strictEqual(await retryOptions.retryOnHttpError(new FetchError('hello!', 'non-system')), false);
+        assert.strictEqual(await retryOptions.retryOnHttpError(new FetchError('hello!', 'system')), false);
+        assert.strictEqual(await retryOptions.retryOnHttpError(new FetchError('ECONNRESET', 'system')), false);
+        assert.strictEqual(await retryOptions.retryOnHttpError(new Error('ECONNRESET', 'system')), false);
+        assert.strictEqual(await retryOptions.retryOnHttpError(new SpecialError('error!')), true);
+    });
 });
 
 describe('test fetch retry', () => {
@@ -471,6 +525,29 @@ describe('test fetch retry', () => {
         assert.strictEqual(response.status, 200);
     });
 
+    it('test retry with async retryOnHttpResponse', async () => {
+        nock(FAKE_BASE_URL)
+            .get(FAKE_PATH)
+            .twice()
+            .reply(403);
+        nock(FAKE_BASE_URL)
+            .get(FAKE_PATH)
+            .reply(200);
+        const response = await fetch(`${FAKE_BASE_URL}${FAKE_PATH}`,
+            {
+                method: 'GET', retryOptions: {
+                    retryInitialDelay: 200,
+                    retryMaxDuration: 1000,
+                    retryOnHttpResponse: async (response) => {
+                        return Promise.resolve(!response.ok);
+                    }
+                }
+            });
+        assert(nock.isDone());
+        assert.strictEqual(response.statusText, 'OK');
+        assert.strictEqual(response.status, 200);
+    });
+
     it('do not retry on some HTTP codes', async () => {
         nock(FAKE_BASE_URL)
             .get(FAKE_PATH)