fix: non-zero delay between first attempt and first retry for linear …

…and exp strategy (#163)

* tests: static backoff: check delay

See issue #122.

This is to confirm that the delay
between the first (actual) attempt
and the first retry is not zero,
but the configured delay.

tests: prettier format (noop)

tests: prettier format (noop)

* fix delay calc: non-zero between first two attempts

See issue #122.

* calc delay: fix currentRetryAttempt mutation

The mutation did not affect the code below.
This is for issue #122.

calc delay: prettier (format)

* tests: test delay for linear, exp strategies

* calc delay: mutate cfg on orig err obj

Found that state is not retained across
retries when mutating
`config.currentRetryAttempt` here.

* calc delay: use expressive variable name

* package.json: require nodejs 10.7.0 or newer

This is to address the `gts fix` error

    The 'process.hrtime.bigint' is not supported
    until Node.js 10.7.0. The configured version
    range is '>=10.0.0'

This function is only used in the test suite
so maybe it's not the best solution to change
the NodeJS version requirement for the
entire package. However, whoever runs this on
NodeJS older than 10.7.0 (released on
2018-07-18) might want to be encouraged to
use a more recent version.

package.json

@@ -8,7 +8,7 @@
   "unpkg": "dist/index.umd.js",
   "types": "dist/src/index.d.ts",
   "engines": {
-    "node": ">=10.0.0"
+    "node": ">=10.7.0"
   },
   "repository": {
     "type": "git",

src/index.ts

@@ -223,19 +223,42 @@ function onError(err: AxiosError) {
         return reject(err);
       }
     }
-    // Else calculate delay according to chosen strategy
+
+    // Now it's certain that a retry is supposed to happen. Incremenent the
+    // counter, critical for linear and exp backoff delay calc. Note that
+    // `config.currentRetryAttempt` is local to this function whereas
+    // `(err.config as RaxConfig).raxConfig` is state that is tranferred across
+    // retries. That is, we want to mutate `(err.config as
+    // RaxConfig).raxConfig`. Another important note is about the definition of
+    // `currentRetryAttempt`: When we are here becasue the first and actual
+    // HTTP request attempt failed then `currentRetryAttempt` is still zero. We
+    // have found that a retry is indeed required. Since that is (will be)
+    // indeed the first retry it makes sense to now increase
+    // `currentRetryAttempt` by 1. So that it is in fact 1 for the first retry
+    // (as opposed to 0 or 2); an intuitive convention to use for the math
+    // below.
+    (err.config as RaxConfig).raxConfig!.currentRetryAttempt! += 1;
+
+    // store with shorter and more expressive variable name.
+    const retrycount = (err.config as RaxConfig).raxConfig!
+      .currentRetryAttempt!;
+
+    // Calculate delay according to chosen strategy
     // Default to exponential backoff - formula: ((2^c - 1) / 2) * 1000
-    else {
+    if (delay === 0) {
+      // was not set by Retry-After logic
       if (config.backoffType === 'linear') {
-        delay = config.currentRetryAttempt! * 1000;
+        // The delay between the first (actual) attempt and the first retry
+        // should be non-zero. Rely on the convention that `retrycount` is
+        // equal to 1 for the first retry when we are in here (was once 0,
+        // which was a bug -- see #122).
+        delay = retrycount * 1000;
       } else if (config.backoffType === 'static') {
         delay = config.retryDelay!;
       } else {
-        delay = ((Math.pow(2, config.currentRetryAttempt!) - 1) / 2) * 1000;
+        delay = ((Math.pow(2, retrycount) - 1) / 2) * 1000;
       }
     }
-    // We're going to retry!  Incremenent the counter.
-    (err.config as RaxConfig).raxConfig!.currentRetryAttempt! += 1;
     setTimeout(resolve, delay);
   });
 

test/index.ts

@@ -109,6 +109,126 @@ describe('retry-axios', () => {
     assert.fail('Expected to throw');
   });
 
+  it('should have non-zero delay between first and second attempt, static backoff', async () => {
+    const requesttimes: bigint[] = [];
+    const scopes = [
+      nock(url)
+        .get('/')
+        .reply((_, __) => {
+          requesttimes.push(process.hrtime.bigint());
+          return [500, 'foo'];
+        }),
+      nock(url)
+        .get('/')
+        .reply((_, __) => {
+          requesttimes.push(process.hrtime.bigint());
+          return [200, 'bar'];
+        }),
+    ];
+
+    interceptorId = rax.attach();
+    const res = await axios({
+      url,
+      raxConfig: {
+        backoffType: 'static',
+      },
+    });
+
+    // Confirm that first retry did yield 200 OK with expected body
+    assert.strictEqual(res.data, 'bar');
+    scopes.forEach(s => s.done());
+
+    assert.strictEqual(requesttimes.length, 2);
+    const delayInSeconds = Number(requesttimes[1] - requesttimes[0]) / 10 ** 9;
+
+    // The default delay between attempts using the
+    // static backoff strategy is 100 ms. Test with tolerance.
+    assert.strict(
+      0.16 > delayInSeconds && delayInSeconds > 0.1,
+      `unexpected delay: ${delayInSeconds.toFixed(3)} s`
+    );
+  });
+
+  it('should have non-zero delay between first and second attempt, linear backoff', async () => {
+    const requesttimes: bigint[] = [];
+    const scopes = [
+      nock(url)
+        .get('/')
+        .reply((_, __) => {
+          requesttimes.push(process.hrtime.bigint());
+          return [500, 'foo'];
+        }),
+      nock(url)
+        .get('/')
+        .reply((_, __) => {
+          requesttimes.push(process.hrtime.bigint());
+          return [200, 'bar'];
+        }),
+    ];
+
+    interceptorId = rax.attach();
+    const res = await axios({
+      url,
+      raxConfig: {
+        backoffType: 'linear',
+      },
+    });
+
+    // Confirm that first retry did yield 200 OK with expected body
+    assert.strictEqual(res.data, 'bar');
+    scopes.forEach(s => s.done());
+
+    assert.strictEqual(requesttimes.length, 2);
+    const delayInSeconds = Number(requesttimes[1] - requesttimes[0]) / 10 ** 9;
+
+    // The default delay between the first two attempts using the
+    // linear backoff strategy is 1000 ms. Test with tolerance.
+    assert.strict(
+      1.1 > delayInSeconds && delayInSeconds > 1.0,
+      `unexpected delay: ${delayInSeconds.toFixed(3)} s`
+    );
+  });
+
+  it('should have non-zero delay between first and second attempt, exp backoff', async () => {
+    const requesttimes: bigint[] = [];
+    const scopes = [
+      nock(url)
+        .get('/')
+        .reply((_, __) => {
+          requesttimes.push(process.hrtime.bigint());
+          return [500, 'foo'];
+        }),
+      nock(url)
+        .get('/')
+        .reply((_, __) => {
+          requesttimes.push(process.hrtime.bigint());
+          return [200, 'bar'];
+        }),
+    ];
+
+    interceptorId = rax.attach();
+    const res = await axios({
+      url,
+      raxConfig: {
+        backoffType: 'exponential',
+      },
+    });
+
+    // Confirm that first retry did yield 200 OK with expected body
+    assert.strictEqual(res.data, 'bar');
+    scopes.forEach(s => s.done());
+
+    assert.strictEqual(requesttimes.length, 2);
+    const delayInSeconds = Number(requesttimes[1] - requesttimes[0]) / 10 ** 9;
+
+    // The default delay between attempts using the
+    // exp backoff strategy is 500 ms. Test with tolerance.
+    assert.strict(
+      0.55 > delayInSeconds && delayInSeconds > 0.5,
+      `unexpected delay: ${delayInSeconds.toFixed(3)} s`
+    );
+  });
+
   it('should accept a new axios instance', async () => {
     const scopes = [
       nock(url).get('/').times(2).reply(500),