[SDK-1975] Use exponential backoff rather than rate limit headers (#538)

* Use exponential backoff rather than rate limit headers * Let some debugging stuff in there * Allow setting node-retry options * 10 retries was thought to be too many for a default Co-authored-by: David Patrick <[email protected]>
auth0 · Oct 14, 2020 · cf2a1e5 · cf2a1e5
1 parent e35908b
commit cf2a1e5
Show file tree

Hide file tree

Showing 4 changed files with 119 additions and 207 deletions.
diff --git a/package.json b/package.json
@@ -57,7 +57,7 @@
     "prettier": "^1.18.2",
     "pretty-quick": "^1.11.1",
     "proxyquire": "^2.1.1",
-    "sinon": "^7.3.2",
+    "sinon": "^9.0.3",
     "string-replace-webpack-plugin": "0.1.3",
     "webpack": "^4.36.1"
   }

diff --git a/src/RetryRestClient.js b/src/RetryRestClient.js
@@ -1,6 +1,11 @@
 var retry = require('retry');
 var ArgumentError = require('rest-facade').ArgumentError;
-var DEFAULT_OPTIONS = { maxRetries: 10, enabled: true };
+
+var DEFAULT_OPTIONS = {
+  maxRetries: 3,
+  enabled: true,
+  randomize: true
+};
 
 /**
  * @class RetryRestClient
@@ -11,6 +16,7 @@ var DEFAULT_OPTIONS = { maxRetries: 10, enabled: true };
  * @param {Object}  [options]                    Options for the RetryRestClient.
  * @param {Object}  [options.enabled:true]       Enabled or Disable Retry Policy functionality.
  * @param {Number}  [options.maxRetries=10]      The maximum amount of times to retry the operation. Default is 10.
+ * @param {*}       [options.*]                  Any options that are available in https://github.com/tim-kos/node-retry#retryoperationoptions
  */
 var RetryRestClient = function(restClient, options) {
   if (restClient === null || typeof restClient !== 'object') {
@@ -28,8 +34,8 @@ var RetryRestClient = function(restClient, options) {
   }
 
   this.restClient = restClient;
-  this.maxRetries = params.maxRetries;
   this.enabled = params.enabled;
+  this.retryOptions = Object.assign({ retries: params.maxRetries }, params);
 };
 
 RetryRestClient.prototype.getAll = function(/* [params], [callback] */) {
@@ -79,16 +85,9 @@ RetryRestClient.prototype.handleRetry = function(method, args) {
     return this.restClient[method].apply(this.restClient, args);
   }
 
-  var retryOptions = {
-    retries: this.maxRetries,
-    factor: 1,
-    minTimeout: 1, // retry immediate, use custom logic to control this.
-    randomize: false
-  };
-
   var self = this;
-  var promise = new Promise(function(resolve, reject) {
-    var operation = retry.operation(retryOptions);
+  return new Promise(function(resolve, reject) {
+    var operation = retry.operation(self.retryOptions);
 
     operation.attempt(function() {
       self.restClient[method]
@@ -97,57 +96,13 @@ RetryRestClient.prototype.handleRetry = function(method, args) {
           resolve(body);
         })
         .catch(function(err) {
-          self.invokeRetry(err, operation, reject);
+          if (err && err.statusCode === 429 && operation.retry(err)) {
+            return;
+          }
+          reject(err);
         });
     });
   });
-
-  return promise;
-};
-
-RetryRestClient.prototype.invokeRetry = function(err, operation, reject) {
-  var ratelimits = this.extractRatelimits(err);
-  if (ratelimits) {
-    var delay = ratelimits.reset * 1000 - new Date().getTime();
-    if (delay > 0) {
-      this.retryWithDelay(delay, operation, err, reject);
-    } else {
-      this.retryWithImmediate(operation, err, reject);
-    }
-  } else {
-    reject(err);
-  }
-};
-
-RetryRestClient.prototype.extractRatelimits = function(err) {
-  if (err && err.statusCode === 429 && err.originalError && err.originalError.response) {
-    var headers = err.originalError.response.header;
-    if (headers && headers['x-ratelimit-limit']) {
-      return {
-        limit: headers['x-ratelimit-limit'],
-        remaining: headers['x-ratelimit-remaining'],
-        reset: headers['x-ratelimit-reset']
-      };
-    }
-  }
-
-  return;
-};
-
-RetryRestClient.prototype.retryWithImmediate = function(operation, err, reject) {
-  if (operation.retry(err)) {
-    return;
-  }
-  reject(err);
-};
-
-RetryRestClient.prototype.retryWithDelay = function(delay, operation, err, reject) {
-  setTimeout(() => {
-    if (operation.retry(err)) {
-      return;
-    }
-    reject(err);
-  }, delay);
 };
 
 module.exports = RetryRestClient;
diff --git a/test/retry-rest-client.tests.js b/test/retry-rest-client.tests.js
@@ -1,4 +1,5 @@
 var expect = require('chai').expect;
+var sinon = require('sinon');
 var nock = require('nock');
 
 var ArgumentError = require('rest-facade').ArgumentError;
@@ -102,15 +103,7 @@ describe('RetryRestClient', function() {
 
     nock(API_URL)
       .get('/')
-      .reply(
-        429,
-        { success: false },
-        {
-          'x-ratelimit-limit': '10',
-          'x-ratelimit-remaining': '0',
-          'x-ratelimit-reset': '1508253300'
-        }
-      )
+      .reply(429, { success: false })
       .get('/')
       .reply(200, { success: true });
 
@@ -124,108 +117,60 @@ describe('RetryRestClient', function() {
 
   it('should try 4 times when request fails 3 times', function(done) {
     var self = this;
+    var clock = sinon.useFakeTimers();
     var timesCalled = 0;
     var restClientSpy = {
       getAll: function() {
         timesCalled += 1;
-        return self.restClient.getAll(arguments);
+        return self.restClient.getAll(arguments).finally(() => {
+          clock.runAllAsync();
+        });
       }
     };
 
     nock(API_URL)
       .get('/')
       .times(3)
-      .reply(
-        429,
-        { success: false },
-        {
-          'x-ratelimit-limit': '10',
-          'x-ratelimit-remaining': '0',
-          'x-ratelimit-reset': '1508253300'
-        }
-      )
+      .reply(429, { success: false })
       .get('/')
       .reply(200, { success: true });
 
     var client = new RetryRestClient(restClientSpy);
     client.getAll().then(function(data) {
+      clock.restore();
       expect(data.success).to.be.true;
       expect(timesCalled).to.be.equal(4);
       done();
     });
   });
 
-  it('should retry 2 times and fail when maxRetries is exceeded with no delay time', function(done) {
+  it('should retry 2 times and fail when maxRetries is exceeded', function(done) {
     var self = this;
+    var clock = sinon.useFakeTimers();
     var timesCalled = 0;
     var restClientSpy = {
       getAll: function() {
         timesCalled += 1;
-        return self.restClient.getAll(arguments);
+        return self.restClient.getAll(arguments).finally(() => {
+          clock.runAllAsync();
+        });
       }
     };
 
     nock(API_URL)
       .get('/')
       .times(4)
-      .reply(
-        429,
-        { success: false },
-        {
-          'x-ratelimit-limit': '10',
-          'x-ratelimit-remaining': '0',
-          'x-ratelimit-reset': (new Date().getTime() - 10) / 1000 // past.
-        }
-      );
+      .reply(429, { success: false });
 
     var client = new RetryRestClient(restClientSpy, { maxRetries: 3 });
     client.getAll().catch(function(err) {
+      clock.restore();
       expect(err).to.not.null;
       expect(timesCalled).to.be.equal(4); // Initial call + 3 retires.
       done();
     });
   });
 
-  it('should retry 2 times and fail when maxRetries is exceeded with delay time', function(done) {
-    var self = this;
-    var timesCalled = 0;
-    var restClientSpy = {
-      getAll: function() {
-        timesCalled += 1;
-        return self.restClient.getAll(arguments);
-      }
-    };
-
-    nock(API_URL)
-      .get('/')
-      .reply(
-        429,
-        { success: false },
-        {
-          'x-ratelimit-limit': '10',
-          'x-ratelimit-remaining': '0',
-          'x-ratelimit-reset': (new Date().getTime() + 50) / 1000 // epoch seconds + 50ms
-        }
-      )
-      .get('/')
-      .reply(
-        429,
-        { success: false },
-        {
-          'x-ratelimit-limit': '10',
-          'x-ratelimit-remaining': '0',
-          'x-ratelimit-reset': (new Date().getTime() + 100) / 1000 // epoch seconds + 100ms
-        }
-      );
-
-    var client = new RetryRestClient(restClientSpy, { maxRetries: 1 });
-    client.getAll().catch(function(err) {
-      expect(err).to.not.null;
-      expect(timesCalled).to.be.equal(2); // Initial call + 3 retires.
-      done();
-    });
-  });
-
   it('should not retry when status code is not 429', function(done) {
     nock(API_URL)
       .get('/')
@@ -239,38 +184,44 @@ describe('RetryRestClient', function() {
     });
   });
 
-  it('should delay the retry using x-ratelimit-reset header value and succeed after retry', function(done) {
+  it('should delay the retry using exponential backoff and succeed after retry', function(done) {
     var self = this;
-    var calledAt = [];
+    var clock = sinon.useFakeTimers();
+    var backoffs = [];
+    var prev = 0;
     var restClientSpy = {
       getAll: function() {
-        calledAt.push(new Date().getTime());
-        return self.restClient.getAll(arguments);
+        var now = new Date().getTime();
+        backoffs.push(now - prev);
+        prev = now;
+        return self.restClient.getAll(arguments).finally(() => {
+          clock.runAllAsync();
+        });
       }
     };
 
     nock(API_URL)
       .get('/')
-      .reply(
-        429,
-        { success: false },
-        {
-          'x-ratelimit-limit': '10',
-          'x-ratelimit-remaining': '0',
-          'x-ratelimit-reset': (new Date().getTime() + 50) / 1000 // epoch seconds + 50ms
-        }
-      )
+      .times(9)
+      .reply(429, { success: false })
       .get('/')
       .reply(200, { success: true });
 
-    var client = new RetryRestClient(restClientSpy);
+    var client = new RetryRestClient(restClientSpy, { maxRetries: 10 });
 
     client.getAll().then(function(data) {
+      clock.restore();
       expect(data.success).to.be.true;
-      expect(calledAt.length).to.be.equal(2);
-
-      var elapsedTime = calledAt[1] - calledAt[0];
-      expect(elapsedTime).to.be.above(49); // Time between the requests should at least be more than 49ms
+      expect(backoffs.length).to.be.equal(10);
+
+      expect(backoffs.shift()).to.be.equal(0, 'first request should happen immediately');
+      for (var i = 0; i < backoffs.length; i++) {
+        expect(backoffs[i] / 1000).to.be.within(
+          Math.pow(2, i),
+          2 * Math.pow(2, i),
+          'attempt ' + (i + 1) + ' in secs'
+        );
+      }
       done();
     });
   });
@@ -287,15 +238,7 @@ describe('RetryRestClient', function() {
 
     nock(API_URL)
       .get('/')
-      .reply(
-        429,
-        { success: false },
-        {
-          'x-ratelimit-limit': '10',
-          'x-ratelimit-remaining': '0',
-          'x-ratelimit-reset': '1508253300'
-        }
-      );
+      .reply(429, { success: false });
 
     var client = new RetryRestClient(restClientSpy, { enabled: false });
     client.getAll().catch(function(err) {