Wait handling cleanup to improve readability

Author: Brendan Robert

index.js

@@ -39,10 +39,11 @@ function isResponseTimedOut(retryOptions) {
  * @param {RetryOptions} retryOptions whether or not to retry on all http error codes or just >500
  * @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
  */
-function shouldRetry(retryOptions, error, response) {
-    if (isResponseTimedOut(retryOptions)) {
+function shouldRetry(retryOptions, error, response, waitTime) {
+    if (getTimeRemaining(retryOptions) < waitTime) {
         return false;
     } else if (retryOptions && retryOptions.retryOnHttpResponse) {
         return retryOptions.retryOnHttpResponse(response);
@@ -166,9 +167,9 @@ module.exports = async function (url, options) {
 
     return new Promise(function (resolve, reject) {
         const wrappedFetch = async () => {
-            let processLastResult = ()=>{};
             while (!isResponseTimedOut(retryOptions)) {
                 ++attempt;
+                const waitTime = getRetryDelay(retryOptions);
 
                 let timeoutHandler;
                 if (retryOptions.socketTimeout) {
@@ -178,39 +179,30 @@ module.exports = async function (url, options) {
                 }                
 
                 try {
-                    // console.log(`Fetching ${url} attempt ${attempt}`);
                     const response = await fetch(url, options);
-                    processLastResult = ()=>resolve(response);
 
-                    if (shouldRetry(retryOptions, null, response)) {
-                        console.error(`Retrying in ${retryOptions.retryInitialDelay} milliseconds, attempt ${attempt} failed (status ${response.status}): ${response.statusText}`);
+                    if (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) {
-                    processLastResult = ()=>reject(error);
                     const response = {status: error.code || 500, statusMessage : error.message || "Unknown server error"};
-                    if (!shouldRetry(retryOptions, error, response)) {
+                    if (!shouldRetry(retryOptions, error, response, waitTime)) {
                         if (error.name === 'AbortError') {
                             return reject(new FetchError(`network timeout at ${url}`, 'request-timeout'));
                         } else {
                             return reject(error);
                         }
                     }
-                    console.error(`Retrying in ${retryOptions.retryInitialDelay} milliseconds, attempt ${attempt} error: ${error.message}`);
+                    console.error(`Retrying in ${waitTime} milliseconds, attempt ${attempt} error: ${error.message}`);
                 } finally {
                     clearTimeout(timeoutHandler);
                 }
-
                 // Fetch loop is about to repeat, delay as needed first.
-                const waitTime = getRetryDelay(retryOptions);
-                if (getTimeRemaining(retryOptions) < waitTime) {
-                    console.error(`Timeout during retry delay, returning last received response`);
-                    processLastResult();
-                    break;
-                } else if (waitTime > 0) {
+                if (waitTime > 0) {
                     await new Promise(resolve => setTimeout(resolve, waitTime));
                 }
                 retryOptions.retryInitialDelay *= retryOptions.retryBackoff; // update retry interval

test/index.test.js

@@ -27,6 +27,20 @@ const getPort = require('get-port');
 const FAKE_BASE_URL = 'https://fakeurl.com';
 const FAKE_PATH = '/image/test.png';
 
+class Timer {
+    constructor() {
+        this.start = Date.now();
+    }
+
+    get ellapsed() {
+        return Date.now() - this.start;
+    }
+
+    isBetween(v1, v2) {
+        const answer = (this.ellapsed >= v1 && this.ellapsed <= v2);
+        return answer;
+    }
+}
 
 describe('test `retryInit` function', () => {
     afterEach(() => {
@@ -365,13 +379,17 @@ describe('test fetch retry', () => {
                 message: 'something awful happened',
                 code: '503',
             });
+        const timer = new Timer();
         try {
-            await fetch(`${FAKE_BASE_URL}${FAKE_PATH}`, { method: 'GET', retryOptions: { retryMaxDuration: 800 } });
+            await fetch(`${FAKE_BASE_URL}${FAKE_PATH}`, { method: 'GET', retryOptions: { retryMaxDuration: 700 } });
+            assert.fail("Should have thrown an error!");
         } catch (e) {
             assert(nock.isDone());
             assert.strictEqual(e.message, 'request to https://fakeurl.com/image/test.png failed, reason: something awful happened');
             assert.strictEqual(e.code, '503');
         }
+        console.log(`ellapsed: ${timer.ellapsed}`);
+        assert.ok(timer.isBetween(300, 500), "Should have taken approximately 400ms");
     });
 
     it('test retry timeout on 404 response', async () => {
@@ -397,13 +415,15 @@ describe('test fetch retry', () => {
             .get(FAKE_PATH)
             .twice()
             .reply(505);
+        const timer = new Timer();
         const response = await fetch(`${FAKE_BASE_URL}${FAKE_PATH}`,
             {
                 method: 'GET', retryOptions: {
                     retryInitialDelay: 5000,
                     retryMaxDuration: 1000
                 }
             });
+        assert.ok(timer.isBetween(0, 100), "Should have taken < 100ms");
         assert(!nock.isDone()); // should fail on first fetch call
         nock.cleanAll();
         assert.strictEqual(response.statusText, 'HTTP Version Not Supported');