-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 3 commits
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,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). | ||
* * `errorFilter` - An optional synchronous function that is invoked on | ||
* erroneous result. 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, result). | ||
* * 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 +67,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({ | ||
* errorFilter: 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 +85,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 +103,15 @@ export default function retry(opts, task, callback) { | |
acc.intervalFunc = typeof t.interval === 'function' ? | ||
t.interval : | ||
constant(+t.interval || DEFAULT_INTERVAL); | ||
|
||
acc.errorFilter = t.errorFilter; | ||
} 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; | ||
|
@@ -110,7 +127,9 @@ export default function retry(opts, task, callback) { | |
var attempt = 1; | ||
function retryAttempt() { | ||
task(function(err) { | ||
if (err && attempt++ < options.times) { | ||
if (err && attempt++ < options.times && | ||
(typeof options.errorFilter != 'function' || | ||
options.errorFilter(err))) { | ||
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. 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 continue test 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 errorTest(err) { | ||
return err && err !== special; | ||
} | ||
var options = { | ||
times: times, | ||
errorFilter: errorTest | ||
}; | ||
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 test 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 errorTest(err) { | ||
return err && err === error + callCount; // just a different pattern | ||
} | ||
var options = { | ||
errorFilter: errorTest | ||
}; | ||
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 test 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 errorTest(err) { | ||
return err && err !== special; | ||
} | ||
var start = new Date().getTime(); | ||
async.retry({ interval: interval, errorFilter: errorTest }, 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 test should not be called',function(done) { | ||
var callCount = 0; | ||
var error = 'ERROR'; | ||
var erroredResult = 'RESULT'; | ||
var continueTestCalled = false; | ||
function fn(callback) { | ||
callCount++; | ||
callback(null, erroredResult + callCount); | ||
} | ||
function errorTest(err) { | ||
continueTestCalled = true; | ||
return err && err === error; | ||
} | ||
var options = { | ||
errorFilter: 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(continueTestCalled, false, "error test 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 error test function', function (done) { | ||
var calls = 0; | ||
var special = 'special'; | ||
var opts = { | ||
errorFilter: 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.
It's currently invoked with just err