From d9448d3b7ecd808a14f00ae3ed20c139faa4ccbc Mon Sep 17 00:00:00 2001 From: cpv123 Date: Thu, 6 Dec 2018 21:24:25 +0000 Subject: [PATCH 1/3] Allow varying delays between consecutive retries The delay between each consecutive retry can be specified, using an array of delays, which allows a back-off retry pattern to be implemented by increasing each consecutive delay. If the number of delays specified is fewer than the number of retries, then the last specified delay will be used for the remaining retries. The existing syntax of delay being a single number still works. --- src/index.js | 35 ++++++++++++++++++++++++++++++----- 1 file changed, 30 insertions(+), 5 deletions(-) diff --git a/src/index.js b/src/index.js index 9fc9953..197b2b7 100644 --- a/src/index.js +++ b/src/index.js @@ -69,10 +69,17 @@ function shouldRetry (err, res, allowedStatuses) { */ function callback (err, res) { if (this._maxRetries && this._retries++ < this._maxRetries && shouldRetry(err, res, this._allowedStatuses)) { + var delay + if (!this._retries) { + delay = 0 + } else { + delay = this._retryDelays[this._retries - 1] + } + var req = this return setTimeout(function () { return req._retry() - }, this._retryDelay) + }, delay) } var fn = this._callback @@ -87,16 +94,16 @@ function callback (err, res) { } /** - * Override Request retry to also set a delay. + * Override Request retry to also set delays between requests. * * In miliseconds. * * @param {Number} retries - * @param {Number} delay + * @param {Number[] || Number} delays * @param {Number[]} allowedStatuses * @return {retry} */ -function retry (retries, delay, allowedStatuses) { +function retry (retries, delays, allowedStatuses) { if (arguments.length === 0 || retries === true) { retries = 1 } @@ -105,9 +112,27 @@ function retry (retries, delay, allowedStatuses) { retries = 0 } + if (typeof delays === 'number') { + delays = [delays] + } + + var numberOfDelays = delays.length + var diff = retries - numberOfDelays + if (diff !== 0) { + if (diff < 0) { + throw new Error('Cannot have more delays than retries') + } else { + // Extrapolate delays list until there is a delay for each retry + var finalDelay = delays[numberOfDelays - 1] + for (var i = 0; i < (diff + 1); i++) { + delays.push(finalDelay) + } + } + } + this._maxRetries = retries this._retries = 0 - this._retryDelay = delay || 0 + this._retryDelays = delays || [0] this._allowedStatuses = allowedStatuses || [] return this From edf03abdea046bce72303a479d55a8228df940a6 Mon Sep 17 00:00:00 2001 From: cpv123 Date: Thu, 6 Dec 2018 21:58:53 +0000 Subject: [PATCH 2/3] Update tests to include new syntax and add varying delay tests Each existing test has a corresponding test using the new syntax for regression purposes. Additional tests for varying the delays, where the actual delays have been measured and compared (within reason) to the expected delays. --- tests/test.js | 224 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 224 insertions(+) diff --git a/tests/test.js b/tests/test.js index 1ecb1cd..b308b22 100644 --- a/tests/test.js +++ b/tests/test.js @@ -26,6 +26,10 @@ describe('superagent-retry-delay', function () { server = app.listen(port, done) }) + afterEach(function () { + requests = 0 + }) + it('should not retry on success', function (done) { agent .get('http://localhost:' + port) @@ -33,7 +37,17 @@ describe('superagent-retry-delay', function () { .end(function (err, res) { res.text.should.eql('hello!') requests.should.eql(1) + done(err) + }) + }) + it('should not retry on success - multiple delays format', function (done) { + agent + .get('http://localhost:' + port) + .retry(5, [17, 17, 17, 17, 17]) + .end(function (err, res) { + res.text.should.eql('hello!') + requests.should.eql(1) done(err) }) }) @@ -62,6 +76,10 @@ describe('superagent-retry-delay', function () { server = app.listen(port, done) }) + afterEach(function () { + requests = 0 + }) + it('should not retry on handled errors', function (done) { agent .get('http://localhost:' + port) @@ -73,6 +91,17 @@ describe('superagent-retry-delay', function () { }) }) + it('should not retry on handled errors - multiple delays format', function (done) { + agent + .get('http://localhost:' + port) + .retry(5, [13, 13, 13, 13, 13], [404]) + .end(function (err, res) { + res.status.should.eql(404) + requests.should.eql(3) + done(err) + }) + }) + after(function (done) { server.close(done) }) }) @@ -95,6 +124,10 @@ describe('superagent-retry-delay', function () { server = app.listen(port, done) }) + afterEach(function () { + requests = 0 + }) + it('should retry on errors', function (done) { agent .get('http://localhost:' + port) @@ -114,6 +147,25 @@ describe('superagent-retry-delay', function () { }) }) + it('should retry on errors - multiple delays format', function (done) { + agent + .get('http://localhost:' + port) + .end(function (err, res) { + res.status.should.eql(503) + + // appease eslint, do nothing with error to allow it to bubble up + if (err) { } + }) + + agent + .get('http://localhost:' + port) + .retry(5, [17, 17, 17, 17, 17]) + .end(function (err, res) { + res.text.should.eql('hello!') + done(err) + }) + }) + after(function (done) { server.close(done) }) }) @@ -136,6 +188,10 @@ describe('superagent-retry-delay', function () { server = app.listen(port, done) }) + afterEach(function () { + requests = 0 + }) + it('should retry on errors', function (done) { agent .get('http://localhost:' + port) @@ -156,6 +212,26 @@ describe('superagent-retry-delay', function () { }) }) + it('should retry on errors - multiple delays format', function (done) { + agent + .get('http://localhost:' + port) + .end(function (err, res) { + res.status.should.eql(500) + + // appease eslint, do nothing with error to allow it to bubble up + if (err) { } + }) + + agent + .get('http://localhost:' + port) + .retry(5, [13, 13, 13, 13, 13]) + .end(function (err, res) { + res.text.should.eql('hello!') + requests.should.eql(5) + done(err) + }) + }) + after(function (done) { server.close(done) }) }) @@ -178,6 +254,10 @@ describe('superagent-retry-delay', function () { server = app.listen(port, done) }) + afterEach(function () { + requests = 0 + }) + it('should retry on errors', function (done) { agent .get('http://localhost:' + port) @@ -198,6 +278,26 @@ describe('superagent-retry-delay', function () { }) }) + it('should retry on errors - multiple delays format', function (done) { + agent + .get('http://localhost:' + port) + .end(function (err, res) { + res.status.should.eql(404) + + // appease eslint, do nothing with error to allow it to bubble up + if (err) { } + }) + + agent + .get('http://localhost:' + port) + .retry(5, [13, 13, 13, 13, 13]) + .end(function (err, res) { + res.text.should.eql('hello!') + requests.should.eql(5) + done(err) + }) + }) + after(function (done) { server.close(done) }) }) @@ -220,6 +320,10 @@ describe('superagent-retry-delay', function () { server = app.listen(port, done) }) + afterEach(function () { + requests = 0 + }) + it('should retry on errors', function (done) { agent .get('http://localhost:' + port) @@ -240,6 +344,126 @@ describe('superagent-retry-delay', function () { }) }) + it('should retry on errors - multiple delays format', function (done) { + agent + .get('http://localhost:' + port) + .end(function (err, res) { + res.status.should.eql(401) + + // appease eslint, do nothing with error to allow it to bubble up + if (err) { } + }) + + agent + .get('http://localhost:' + port) + .retry(5, [13, 13, 13, 13, 13]) + .end(function (err, res) { + res.text.should.eql('hello!') + requests.should.eql(5) + done(err) + }) + }) + + after(function (done) { server.close(done) }) + }) + + describe('specifying different delays between retries', function () { + let requests = 0 + let delays + let delaysMeasured = [] + let delaysAdjusted = [] + let start + let now + const port = 10410 + const app = express() + let server + + before(function (done) { + app.get('/', function (req, res, next) { + requests++ + if (requests === 1) { + start = new Date().valueOf() + } else { + now = new Date().valueOf() + delaysMeasured.push(now - start) + } + if (requests > 5) { + res.send('hello!') + } else { + res.sendStatus(401) + } + }) + + server = app.listen(port, done) + }) + + afterEach(function () { + requests = 0 + delaysMeasured = [] + delaysAdjusted = [] + }) + + it('retries using the specified delays', function (done) { + delays = [100, 200, 300, 400, 500] + + agent + .get('http://localhost:' + port) + .retry(5, delays) + .end(function (err, res) { + res.text.should.eql('hello!') + requests.should.eql(6) + delaysMeasured.length.should.equal(5) + + // Create a list of the actual delays measured between consecutive retries + for (let i = 0; i < delaysMeasured.length; i++) { + if (i === 0) { + delaysAdjusted[i] = delaysMeasured[i] + } else { + delaysAdjusted[i] = delaysMeasured[i] - delaysMeasured[i - 1] + } + } + + // Assert that each delay measured is close to the specified delay + for (let i = 0; i < delaysAdjusted.length; i++) { + let delayDiff = delaysAdjusted[i] - delays[i] + delayDiff.should.be.within(0, 20) + } + + done(err) + }) + }) + + it('extrapolates the list of delays', function (done) { + delays = [100, 200, 300] + const expectedDelays = [100, 200, 300, 300, 300] + + agent + .get('http://localhost:' + port) + .retry(5, delays) + .end(function (err, res) { + res.text.should.eql('hello!') + requests.should.eql(6) + delaysMeasured.length.should.equal(5) + + // Create a list of the actual delays measured between consecutive retries + for (let i = 0; i < delaysMeasured.length; i++) { + if (i === 0) { + delaysAdjusted[i] = delaysMeasured[i] + } else { + delaysAdjusted[i] = delaysMeasured[i] - delaysMeasured[i - 1] + } + } + + // Assert that each delay measured is close to the specified delay + for (let i = 0; i < delaysAdjusted.length; i++) { + let delayDiff = delaysAdjusted[i] - expectedDelays[i] + delayDiff.should.be.within(0, 20) + } + + done(err) + }) + }) + after(function (done) { server.close(done) }) }) }) From a067ca59bd4659155fa3afa3ab273b0f7b723f8c Mon Sep 17 00:00:00 2001 From: cpv123 Date: Thu, 6 Dec 2018 22:27:59 +0000 Subject: [PATCH 3/3] Update README with new usage examples --- README.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/README.md b/README.md index 691486e..1feafca 100644 --- a/README.md +++ b/README.md @@ -20,6 +20,16 @@ superagent .retry(2, 5000, [401, 404]) // retry twice before responding, wait 5 seconds between failures, do not retry when response is success, or 401 or 404 .end(onresponse); +superagent + .get('https://segment.io') + .retry(3, [1000, 3000, 10000], [401, 404]) // retry three times before responding, first wait 1 second, then 3 seconds, and finally 10 seconds between failures, do not retry when response is success, or 401 or 404 + .end(onresponse); + +superagent + .get('https://segment.io') + .retry(5, [1000, 3000], [401, 404]) // retry five times before responding, first wait 1 second, and then wait 3 seconds between all other failures, do not retry when response is success, or 401 or 404 + .end(onresponse); + function onresponse (err, res) { console.log(res.status, res.headers); console.log(res.body);