Skip to content

add filter option to retry() and retryable(). PR for #1256. #1261

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

Merged
megawac merged 4 commits into from
Aug 8, 2016
Merged

add filter option to retry() and retryable(). PR for #1256. #1261

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cbc5b4f
add filter option to retry() and retryable() to allow for error filte…
bojand Aug 2, 2016
0337ee0
changed the error test function to continueOperation. improved commen…
bojand Aug 3, 2016
0af976c
rename the new retry option to errorFilter and consolidate retry atte…
bojand Aug 5, 2016
4367751
fix errorFilter comment to reflect that we only pass err argument. Re…
bojand Aug 8, 2016
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
Prev Previous commit
Next Next commit
changed the error test function to continueOperation. improved commen… 
…t documentation and fixed code based on PR feedback.
  • Loading branch information
@bojand
bojand committed Aug 3, 2016
commit 0337ee0b2e582917cc15e6efc186e3c89e0a2cdb
22 changes: 9 additions & 13 deletions lib/retry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ import constant from 'lodash/constant';
* * `interval` - The time to wait between retries, in milliseconds. The
* default is `0`. The interval may also be specified as a function of the
* retry count (see example).
* * `filter` - Synchronous function that is invoked on erroneous result with the
* the error. If it returns `true` the retry attempts will continue, if the
* function returns `false` the retry flow is aborting with the current
* attempt's error and result being returned to the final callback.
* * `continueOperation` - An optional synchronous function that is invoked on
* erroneous result with the the error. If it returns `true` the retry attempts
* will continue, if the function returns `false` the retry flow is aborted
* with the current attempt's error and result being returned to the final
* callback. Invoked with (err).

@megawac megawac Aug 4, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

isn't it invoked with (error, ...any other arguments)

* * If `opts` is a number, the number specifies the number of times to retry,
* with the default interval of `0`.
* @param {Function} task - A function which receives two arguments: (1) a
Expand Down Expand Up @@ -69,7 +70,7 @@ import constant from 'lodash/constant';
* // try calling apiMethod only when error condition satisfies, all other
* // errors will abort the retry control flow and return to final callback
* async.retry({
* filter: function(err) {
* continueOperation: function(err) {
* return err.message === 'Temporary error'; // only retry on a specific error
* }
* }, apiMethod, function(err, result) {
Expand Down Expand Up @@ -103,9 +104,7 @@ export default function retry(opts, task, callback) {
t.interval :
constant(+t.interval || DEFAULT_INTERVAL);

if(typeof t.filter === 'function') {
acc.filter = t.filter;
}
acc.continueOperation = t.continueOperation;
} else if (typeof t === 'number' || typeof t === 'string') {
acc.times = +t || DEFAULT_TIMES;
} else {
Expand All @@ -129,11 +128,8 @@ export default function retry(opts, task, callback) {
function retryAttempt() {
task(function(err) {
if (err && attempt++ < options.times) {
var proceed = true;
if(options.filter) {
proceed = options.filter(err);
}

var proceed = typeof options.continueOperation != 'function' ||
options.continueOperation(err);
if(proceed) {
setTimeout(retryAttempt, options.intervalFunc(attempt));
} else {
Expand Down
30 changes: 15 additions & 15 deletions mocha_test/retry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ describe("retry", function () {
});
});

it('retry when all attempts fail and error filter returns true',function(done) {
it('retry when all attempts fail and error continue test returns true',function(done) {
var times = 3;
var callCount = 0;
var error = 'ERROR';
Expand All @@ -168,12 +168,12 @@ describe("retry", function () {
callCount++;
callback(error + callCount, erroredResult + callCount);
}
function filter(err) {
function errorTest(err) {
return err && err !== special;
}
var options = {
times: times,
filter: filter
continueOperation: errorTest
};
async.retry(options, fn, function(err, result){
assert.equal(callCount, 3, "did not retry the correct number of times");
Expand All @@ -183,7 +183,7 @@ describe("retry", function () {
});
});

it('retry when some attempts fail and error filter returns false at some invokation',function(done) {
it('retry when some attempts fail and error test returns false at some invokation',function(done) {
var callCount = 0;
var error = 'ERROR';
var special = 'SPECIAL_ERROR';
Expand All @@ -193,11 +193,11 @@ describe("retry", function () {
var err = callCount === 2 ? special : error + callCount;
callback(err, erroredResult + callCount);
}
function filter(err) {
function errorTest(err) {
return err && err === error + callCount; // just a different pattern
}
var options = {
filter: filter
continueOperation: errorTest
};
async.retry(options, fn, function(err, result){
assert.equal(callCount, 2, "did not retry the correct number of times");
Expand All @@ -207,7 +207,7 @@ describe("retry", function () {
});
});

it('retry with interval when some attempts fail and error filter returns false at some invokation',function(done) {
it('retry with interval when some attempts fail and error test returns false at some invokation',function(done) {
var interval = 50;
var callCount = 0;
var error = 'ERROR';
Expand All @@ -219,11 +219,11 @@ describe("retry", function () {
var err = callCount === specialCount ? special : error + callCount;
callback(err, erroredResult + callCount);
}
function filter(err) {
function errorTest(err) {
return err && err !== special;
}
var start = new Date().getTime();
async.retry({ interval: interval, filter: filter }, fn, function(err, result){
async.retry({ interval: interval, continueOperation: errorTest }, fn, function(err, result){
var now = new Date().getTime();
var duration = now - start;
assert(duration >= (interval * (specialCount - 1)), 'did not include interval');

@megawac megawac Aug 7, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this is a bit fragile of a test

@bojand bojand Aug 8, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

hi, can you elaborate? Do you mean the specific assertion? I based it on existing test. Just checks that the appropriate time has passed?

@megawac megawac Aug 8, 2016

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

its probably fine, we have just seen issues lately with some tests erring out (especially in CI) due to slight timing issues

Expand All @@ -234,26 +234,26 @@ describe("retry", function () {
});
});

it('retry when first attempt succeeds and error filter should not be called',function(done) {
it('retry when first attempt succeeds and error test should not be called',function(done) {
var callCount = 0;
var error = 'ERROR';
var erroredResult = 'RESULT';
var filterCalled = false;
var continueTestCalled = false;
function fn(callback) {
callCount++;
callback(null, erroredResult + callCount);
}
function filter(err) {
filterCalled = true;
function errorTest(err) {
continueTestCalled = true;
return err && err === error;
}
var options = {
filter: filter
continueOperation: errorTest
};
async.retry(options, fn, _.rest(function(args) {
assert.equal(callCount, 1, "did not retry the correct number of times");
expect(args).to.be.eql([null, erroredResult + callCount]);
assert.equal(filterCalled, false, "filter function was called");
assert.equal(continueTestCalled, false, "error test function was called");
done();
}));
});
Expand Down
4 changes: 2 additions & 2 deletions mocha_test/retryable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ describe('retryable', function () {
}, 15);
});

it('basics with filter function', function (done) {
it('basics with error test function', function (done) {
var calls = 0;
var special = 'special';
var opts = {
filter: function(err) {
continueOperation: function(err) {
return err == special;
}
};
Expand Down