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.

Sign up for GitHub

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

Jump to bottom

fix: correctly calculate retry attempt #33

Merged
merged 3 commits into from
Jul 31, 2020
Merged

fix: correctly calculate retry attempt #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8ed1715
fix: correctly calculate retry attempt
stephenplusplus Jul 31, 2020
31e7980
update circle config
stephenplusplus Jul 31, 2020
3bb560f
update circle config
stephenplusplus Jul 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 5 additions & 11 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
version: 3
version: 2.1

install_and_test: &install_and_test
steps:
Expand All @@ -11,23 +11,17 @@ install_and_test: &install_and_test
command: npm test

jobs:
test_node8:
docker:
- image: node:8
<<: *install_and_test
test_node10:
docker:
- image: node:10
- image: circleci/node:10
<<: *install_and_test
test_node11:
test_node12:
docker:
- image: node:11
- image: circleci/node:12
<<: *install_and_test

workflows:
version: 3
test:
jobs:
- test_node8
- test_node10
- test_node11
- test_node12
12 changes: 10 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,12 @@ function retryRequest(requestOpts, opts, callback) {
if (typeof opts.retries !== 'number') {
opts.retries = DEFAULTS.retries;
}
if (typeof opts.currentRetryAttempt !== 'number') {

var manualCurrentRetryAttemptWasSet = typeof opts.currentRetryAttempt === 'number';
if (!manualCurrentRetryAttemptWasSet) {
opts.currentRetryAttempt = DEFAULTS.currentRetryAttempt;
}

if (typeof opts.noResponseRetries !== 'number') {
opts.noResponseRetries = DEFAULTS.noResponseRetries;
}
Expand Down Expand Up @@ -190,7 +193,12 @@ function retryRequest(requestOpts, opts, callback) {
}

// Send the response to see if we should try again.
if (currentRetryAttempt <= opts.retries && opts.shouldRetryFn(response)) {
// NOTE: "currentRetryAttempt" isn't accurate by default, as it counts
// the very first request sent as the first "retry". It is only accurate
// when a user provides their own "currentRetryAttempt" option at
// instantiation.
var adjustedCurrentRetryAttempt = manualCurrentRetryAttemptWasSet ? currentRetryAttempt : currentRetryAttempt - 1;
if (adjustedCurrentRetryAttempt < opts.retries && opts.shouldRetryFn(response)) {
retryAfterDelay(currentRetryAttempt);
return;
}
Expand Down
2 changes: 1 addition & 1 deletion test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ describe('retry-request', function () {
};

retryRequest(URI_404, opts, function (err) {
assert.strictEqual(numAttempts, 2);
assert.strictEqual(numAttempts, 1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default number of retries is 2, which means we should make a total of 3 requests for the user who doesn't have any custom settings. In this case, the test says "we've already made 1 retry" by setting "currentRetryAttempt" to 1. That means there's one more retry to go until we reach 2 retries. So, total number of requests = 1.

'currentRetryAttempt` was introduced exclusively for Bigtable: googleapis/nodejs-bigtable#27. I wasn't the author of that code, but I ran this change against the system tests, and they still pass.

done();
});
});
Expand Down