Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add simplistic exponential backoff retry #6

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ var options = {
retry : 2, // will retry the call twice, in case of error.
verbose_logging : false, // will log errors only, if set to be true, will log all actions
accepted: [ 400, 404 ] // Accepted HTTP Status codes (will not retry if request response has any of these HTTP Status Code)
delay: 2000 // will delay retries by 2000 ms. The default is 100.
factor: 2 // will multiple the delay by the factor each time a retry is attempted.
};

rp(options)
Expand Down
22 changes: 19 additions & 3 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,18 @@ class rpRetry {
}
const tries = options.retry || 1;
delete options.retry;
const fetchDataWithRetry = tryCount => {

const delay = options.delay || 100; // default ms delay between retries
delete options.delay;

const factor = options.factor || 1; // If absent, delay will always be the same.
delete options.factor;

if (options.verbose_logging) {
logger.info(`calling ${options.uri} with retry ${tries}, initial delay=${delay}, factor=${factor}`);
}

const fetchDataWithRetry = (tryCount, delay) => {
return requestPromise(options)
.then(result => {
if (options.verbose_logging) {
Expand All @@ -28,12 +39,17 @@ class rpRetry {
logger.info(`Encountered error ${err.message} for ${options.method} request to ${options.uri}, retry count ${tryCount}`);
tryCount -= 1;
if (tryCount) {
return fetchDataWithRetry(tryCount);
return new Promise((resolve, reject) => {
setTimeout(() => {
logger.debug(`waiting for ${delay} ms before next retry for ${options.uri}. Next wait ${delay * factor}`);
resolve(fetchDataWithRetry(tryCount, delay * factor));
}, delay);
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider multiplying the delay by the factor after the fetch.

On might expect the second request to happen 100 ms after the first request, but currently it's 100 ms * factor instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markstos nope, this is in the catch clause. So as I understand it, this is already on the first failure. I did have the same thought as you are, and agree waiting on the first download would be wasteful :-).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I didn't read enough code context. Well done.

}
return Promise.reject(err);
});
};
return fetchDataWithRetry(tries);
return fetchDataWithRetry(tries, delay);
}

static _rp(options) {
Expand Down
5 changes: 4 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
"main": "index.js",
"scripts": {
"test": "node_modules/gulp/bin/gulp.js test",
"coveralls": "cat ./coverage/lcov.info | ./node_modules/.bin/coveralls"
"coveralls": "cat ./coverage/lcov.info | ./node_modules/.bin/coveralls",
"eslint": "eslint test/. index.js",
"eslint-fix": "eslint test/. index.js --fix"
},
"repository": {
"type": "git",
Expand Down Expand Up @@ -45,6 +47,7 @@
"chai": "^4.1.2",
"chai-subset": "^1.6.0",
"coveralls": "^3.0.0",
"eslint": "^5.5.0",
"gulp": "^3.9.1",
"gulp-eslint": "^4.0.0",
"gulp-if": "^2.0.2",
Expand Down
13 changes: 13 additions & 0 deletions test/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,19 @@ describe('request-promise-retry', function () {
expect(data.error).equal(undefined);
});
});
it('should pass, retry with exponential backoff', () => {
// failure should take a bit of time to happen
const startTime = new Date();
return rp({
uri: 'http://adadadadad.com/',
method: 'GET',
retry: 4,
delay: 30,
factor: 10 // 0 + 30 + 300 + 3000
})
.catch(error => {
expect(new Date() - startTime).to.be.above(0 + 30 + 300 + 3000);
});
it('should not retry, accepted options enabled', () => {
return rp(optionsDontRetryAcceptedOptions)
.catch(data => {
Expand Down