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 filter option to retry() and retryable(). PR for #1256. #1261
Changes from 1 commit
cbc5b4f
0337ee0
0af976c
4367751
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,10 @@ 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. | ||
* * 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 | ||
|
@@ -62,6 +66,16 @@ import constant from 'lodash/constant'; | |
* // do something with the result | ||
* }); | ||
* | ||
* // 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) { | ||
* return err.message === 'Temporary error'; // only retry on a specific error | ||
* } | ||
* }, apiMethod, function(err, result) { | ||
* // do something with the result | ||
* }); | ||
* | ||
* // It can also be embedded within other control flow functions to retry | ||
* // individual methods that are not as reliable, like this: | ||
* async.auto({ | ||
|
@@ -70,6 +84,7 @@ import constant from 'lodash/constant'; | |
* }, function(err, results) { | ||
* // do something with the results | ||
* }); | ||
* | ||
*/ | ||
export default function retry(opts, task, callback) { | ||
var DEFAULT_TIMES = 5; | ||
|
@@ -87,14 +102,17 @@ export default function retry(opts, task, callback) { | |
acc.intervalFunc = typeof t.interval === 'function' ? | ||
t.interval : | ||
constant(+t.interval || DEFAULT_INTERVAL); | ||
|
||
if(typeof t.filter === 'function') { | ||
acc.filter = t.filter; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need for the conditional just set it here and check if it is a function when you execute |
||
} else if (typeof t === 'number' || typeof t === 'string') { | ||
acc.times = +t || DEFAULT_TIMES; | ||
} else { | ||
throw new Error("Invalid arguments for async.retry"); | ||
} | ||
} | ||
|
||
|
||
if (arguments.length < 3 && typeof opts === 'function') { | ||
callback = task || noop; | ||
task = opts; | ||
|
@@ -111,7 +129,16 @@ export default function retry(opts, task, callback) { | |
function retryAttempt() { | ||
task(function(err) { | ||
if (err && attempt++ < options.times) { | ||
setTimeout(retryAttempt, options.intervalFunc(attempt)); | ||
var proceed = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
if(options.filter) { | ||
proceed = options.filter(err); | ||
} | ||
|
||
if(proceed) { | ||
setTimeout(retryAttempt, options.intervalFunc(attempt)); | ||
} else { | ||
callback.apply(null, arguments); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should it not be called with err? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
} | ||
} else { | ||
callback.apply(null, arguments); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you try to merge these two else conditions |
||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -156,5 +156,105 @@ describe("retry", function () { | |
async.retry(5, fn, function(err, result) { | ||
expect(result).to.be.eql({a: 1}); | ||
}); | ||
}) | ||
}); | ||
|
||
it('retry when all attempts fail and error filter returns true',function(done) { | ||
var times = 3; | ||
var callCount = 0; | ||
var error = 'ERROR'; | ||
var special = 'SPECIAL_ERROR'; | ||
var erroredResult = 'RESULT'; | ||
function fn(callback) { | ||
callCount++; | ||
callback(error + callCount, erroredResult + callCount); | ||
} | ||
function filter(err) { | ||
return err && err !== special; | ||
} | ||
var options = { | ||
times: times, | ||
filter: filter | ||
}; | ||
async.retry(options, fn, function(err, result){ | ||
assert.equal(callCount, 3, "did not retry the correct number of times"); | ||
assert.equal(err, error + times, "Incorrect error was returned"); | ||
assert.equal(result, erroredResult + times, "Incorrect result was returned"); | ||
done(); | ||
}); | ||
}); | ||
|
||
it('retry when some attempts fail and error filter returns false at some invokation',function(done) { | ||
var callCount = 0; | ||
var error = 'ERROR'; | ||
var special = 'SPECIAL_ERROR'; | ||
var erroredResult = 'RESULT'; | ||
function fn(callback) { | ||
callCount++; | ||
var err = callCount === 2 ? special : error + callCount; | ||
callback(err, erroredResult + callCount); | ||
} | ||
function filter(err) { | ||
return err && err === error + callCount; // just a different pattern | ||
} | ||
var options = { | ||
filter: filter | ||
}; | ||
async.retry(options, fn, function(err, result){ | ||
assert.equal(callCount, 2, "did not retry the correct number of times"); | ||
assert.equal(err, special, "Incorrect error was returned"); | ||
assert.equal(result, erroredResult + 2, "Incorrect result was returned"); | ||
done(); | ||
}); | ||
}); | ||
|
||
it('retry with interval when some attempts fail and error filter returns false at some invokation',function(done) { | ||
var interval = 50; | ||
var callCount = 0; | ||
var error = 'ERROR'; | ||
var erroredResult = 'RESULT'; | ||
var special = 'SPECIAL_ERROR'; | ||
var specialCount = 3; | ||
function fn(callback) { | ||
callCount++; | ||
var err = callCount === specialCount ? special : error + callCount; | ||
callback(err, erroredResult + callCount); | ||
} | ||
function filter(err) { | ||
return err && err !== special; | ||
} | ||
var start = new Date().getTime(); | ||
async.retry({ interval: interval, filter: filter }, fn, function(err, result){ | ||
var now = new Date().getTime(); | ||
var duration = now - start; | ||
assert(duration >= (interval * (specialCount - 1)), 'did not include interval'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a bit fragile of a test There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
assert.equal(callCount, specialCount, "did not retry the correct number of times"); | ||
assert.equal(err, special, "Incorrect error was returned"); | ||
assert.equal(result, erroredResult + specialCount, "Incorrect result was returned"); | ||
done(); | ||
}); | ||
}); | ||
|
||
it('retry when first attempt succeeds and error filter should not be called',function(done) { | ||
var callCount = 0; | ||
var error = 'ERROR'; | ||
var erroredResult = 'RESULT'; | ||
var filterCalled = false; | ||
function fn(callback) { | ||
callCount++; | ||
callback(null, erroredResult + callCount); | ||
} | ||
function filter(err) { | ||
filterCalled = true; | ||
return err && err === error; | ||
} | ||
var options = { | ||
filter: filter | ||
}; | ||
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"); | ||
done(); | ||
})); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,30 @@ describe('retryable', function () { | |
}, 15); | ||
}); | ||
|
||
it('basics with filter function', function (done) { | ||
var calls = 0; | ||
var special = 'special'; | ||
var opts = { | ||
filter: function(err) { | ||
return err == special; | ||
} | ||
}; | ||
var retryableTask = async.retryable(opts, function (arg, cb) { | ||
calls++; | ||
expect(arg).to.equal(42); | ||
cb(calls === 3 ? 'fail' : special); | ||
}); | ||
|
||
retryableTask(42, function (err) { | ||
expect(err).to.equal('fail'); | ||
expect(calls).to.equal(3); | ||
done(); | ||
}); | ||
|
||
setTimeout(function () { | ||
}, 15); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. qu'est-ce que? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this timeout for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had used an existing test which had it; and I didn't notice it. Not sure why it's there to be honest. In the latest commit I removed it from the existing test and the new one. Tests pass locally for me, and in CI it seems. |
||
}); | ||
|
||
it('should work as an embedded task', function(done) { | ||
var retryResult = 'RETRY'; | ||
var fooResults; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think filter is the right term for this. Perhaps
continueOperation
or something? /cc @hargasinski @aearlyAlso this should be documented as optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 for
continueOperation
. I'd also be open toerrorFilter
from the issue,Also, you don't need to run the script to generate the documentation. Just make sure the tests pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't fan of
filter
either. Most of the do / while type of functions within async usetest
... maybe something similar to that?continueTest
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed the comment somehow. Thanks for the feedback. I will change it to
continueOperation
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errorFilter
/errorTest
/shouldError
/onError
/shouldContinueOnError
I think
continueOperation
is a bit misleading, because it's not something you do when you continue, it's a test/callback to see if you should continue or not.Cache invalidation and naming things....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like
errorFilter
orerrorTest
. Another option I suppose isretryTest
?