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

Successful error filter shouldn't invoke the fallback function #556

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
13 changes: 8 additions & 5 deletions lib/circuit.js
Expand Up @@ -655,15 +655,18 @@ class CircuitBreaker extends EventEmitter {
function handleError (error, circuit, timeout, args, latency, resolve, reject) {
clearTimeout(timeout);

if (circuit.options.errorFilter(error, ...args)) {
let errorFiltered;
if ((errorFiltered = circuit.options.errorFilter(error, ...args))) {
circuit.emit('success', error, latency);
resolve(errorFiltered);
} else {
fail(circuit, error, args, latency);
}

const fb = fallback(circuit, error, args);
if (fb) resolve(fb);
else reject(error);
// Only call the fallback function if there errorFilter doesn't succeed
const fb = fallback(circuit, error, args);
if (fb) resolve(fb);
else reject(error);
}
}

function fallback (circuit, err, args) {
Expand Down
10 changes: 6 additions & 4 deletions test/error-filter-test.js
Expand Up @@ -12,18 +12,20 @@ const options = {
errorFilter: err => err.statusCode < 500
};

test('Bypasses failure stats if errorFilter returns true', t => {
test('Bypasses failure stats if errorFilter returns true - Should return a success', t => {
t.plan(4);

const breaker = new CircuitBreaker(failWithCode, options);
breaker.fire(400)
.then(t.fail)
.catch(err => {
t.equal(err.statusCode, 400);
.then(returnValue => {
t.equal(returnValue, true, 'return value is the value of the errorFilter function');
t.equal(breaker.stats.failures, 0);
t.equal(breaker.stats.successes, 1);
t.ok(breaker.closed);
t.end();
})
.catch(_ => {
t.fail();
});
});

Expand Down
9 changes: 3 additions & 6 deletions test/half-open-test.js
Expand Up @@ -48,7 +48,7 @@ test('When half-open, the circuit only allows one request through', t => {
});

test('When half-open, a filtered error should close the circuit', t => {
t.plan(7);
t.plan(6);
const options = {
errorThresholdPercentage: 1,
resetTimeout: 100,
Expand All @@ -69,13 +69,10 @@ test('When half-open, a filtered error should close the circuit', t => {
t.ok(breaker.halfOpen, 'should be halfOpen after timeout');
t.ok(breaker.pendingClose, 'should be pending close after timeout');
breaker
.fire(400) // fail with a filtered error
.catch(e =>
t.equals(e.message, 'Failed with 400 status code',
'should fail again'))
.fire(400) // pass with a filtered error
.then(() => {
t.ok(breaker.closed,
'should be closed after failing with filtered error');
'should be closed after passing with filtered error');
})
.then(_ => breaker.shutdown())
.then(t.end);
Expand Down