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 filter option to retry() and retryable(). PR for #1256. #1261

Merged
merged 4 commits into from Aug 8, 2016
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
27 changes: 25 additions & 2 deletions lib/retry.js
Expand Up @@ -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).
* * `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).
Copy link
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 @@ -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({
* continueOperation: 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({
Expand All @@ -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;
Expand All @@ -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.continueOperation = t.continueOperation;
} 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;
Expand All @@ -111,7 +128,13 @@ export default function retry(opts, task, callback) {
function retryAttempt() {
task(function(err) {
if (err && attempt++ < options.times) {
setTimeout(retryAttempt, options.intervalFunc(attempt));
var proceed = typeof options.continueOperation != 'function' ||
options.continueOperation(err);
if(proceed) {
setTimeout(retryAttempt, options.intervalFunc(attempt));
} else {
callback.apply(null, arguments);
Copy link
Collaborator

Choose a reason for hiding this comment

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

should it not be called with err?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

callback is applied with null for context / this, and arguments for the actual arguments, so the error should be passed through. The new tests cover this. This is consistent with the scenario where we've exhausted all attempts and still had an error possibly.

}
} else {
callback.apply(null, arguments);
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you try to merge these two else conditions

}
Expand Down
102 changes: 101 additions & 1 deletion mocha_test/retry.js
Expand Up @@ -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,
continueOperation: 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 = {
continueOperation: 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, continueOperation: errorTest }, fn, function(err, result){
var now = new Date().getTime();
var duration = now - start;
assert(duration >= (interval * (specialCount - 1)), 'did not include interval');
Copy link
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

Copy link
Contributor Author

@bojand bojand Aug 8, 2016

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?

Copy link
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

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 = {
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(continueTestCalled, false, "error test function was called");
done();
}));
});
});
24 changes: 24 additions & 0 deletions mocha_test/retryable.js
Expand Up @@ -21,6 +21,30 @@ describe('retryable', function () {
}, 15);
});

it('basics with error test function', function (done) {
var calls = 0;
var special = 'special';
var opts = {
continueOperation: 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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

qu'est-ce que?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What is this timeout for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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;
Expand Down