Skip to content

Commit

Permalink
bug: fix stuck halfOpen when using errorFilter (#389)
Browse files Browse the repository at this point in the history
* Fix stuck halfOpen when using errorFilter

When an error is filtered by an errorFilter the request is
considered neither success nor failure, leaving the circuit
in an inconsistent state. This fixes that issue by emitting
success event when the error is filtered.

* Call fallback when an error is filtered

When an error is filtered, it is considered a success, however we need a
result to resolve with, this way, we should call fallback to get a valid
result. If there is no fallback, we reject to give the application a
chance to handle the error.
  • Loading branch information
jairelton authored and lance committed Dec 17, 2019
1 parent 7a96e79 commit a721f3f
Showing 4 changed files with 56 additions and 15 deletions.
8 changes: 5 additions & 3 deletions lib/circuit.js
Original file line number Diff line number Diff line change
@@ -640,7 +640,11 @@ class CircuitBreaker extends EventEmitter {

function handleError (error, circuit, timeout, args, latency, resolve, reject) {
clearTimeout(timeout);
fail(circuit, error, args, latency);

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

const fb = fallback(circuit, error, args);
if (fb) resolve(fb);
else reject(error);
@@ -663,8 +667,6 @@ function fallback (circuit, err, args) {
}

function fail (circuit, err, args, latency) {
if (circuit.options.errorFilter(err)) return;

/**
* Emitted when the circuit breaker action fails
* @event CircuitBreaker#failure
9 changes: 8 additions & 1 deletion test/common.js
Original file line number Diff line number Diff line change
@@ -58,6 +58,12 @@ function failedCallbackFunction () {
Array.prototype.slice.call(arguments).pop()('Whoops!');
}

function failWithCode (errorCode) {
const err = new Error(`Failed with ${errorCode} status code`);
err.statusCode = errorCode;
return Promise.reject(err);
}

function identity (_) { return _; }

module.exports = exports = {
@@ -68,5 +74,6 @@ module.exports = exports = {
callbackFunction,
failedCallbackFunction,
nonPromise,
identity
identity,
failWithCode
};
16 changes: 6 additions & 10 deletions test/error-filter-test.js
Original file line number Diff line number Diff line change
@@ -2,12 +2,7 @@

const test = require('tape');
const CircuitBreaker = require('../lib/circuit');

function mightFail (errorCode) {
const err = new Error('Something went wrong');
err.statusCode = errorCode;
return Promise.reject(err);
}
const { failWithCode } = require('./common');

const options = {
errorThresholdPercentage: 1,
@@ -18,14 +13,15 @@ const options = {
};

test('Bypasses failure stats if errorFilter returns true', t => {
t.plan(3);
t.plan(4);

const breaker = new CircuitBreaker(mightFail, options);
const breaker = new CircuitBreaker(failWithCode, options);
breaker.fire(400)
.then(t.fail)
.catch(err => {
t.equal(err.statusCode, 400);
t.equal(breaker.stats.failures, 0);
t.equal(breaker.stats.successes, 1);
t.ok(breaker.closed);
t.end();
});
@@ -34,7 +30,7 @@ test('Bypasses failure stats if errorFilter returns true', t => {
test('Increments failure stats if errorFilter returns false', t => {
t.plan(3);

const breaker = new CircuitBreaker(mightFail, options);
const breaker = new CircuitBreaker(failWithCode, options);
breaker.fire(500)
.then(t.fail)
.catch(err => {
@@ -47,7 +43,7 @@ test('Increments failure stats if errorFilter returns false', t => {

test('Increments failure stats if no filter is provided', t => {
t.plan(3);
const breaker = new CircuitBreaker(mightFail,
const breaker = new CircuitBreaker(failWithCode,
{ errorThresholdPercentage: 1 });
breaker.fire(500)
.then(t.fail)
38 changes: 37 additions & 1 deletion test/half-open-test.js
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@

const test = require('tape');
const CircuitBreaker = require('../lib/circuit');
const { timedFailingFunction } = require('./common');
const { timedFailingFunction, failWithCode } = require('./common');

test('When half-open, the circuit only allows one request through', t => {
t.plan(10);
@@ -46,3 +46,39 @@ test('When half-open, the circuit only allows one request through', t => {
.then(t.end);
}, options.resetTimeout * 1.5);
});

test('When half-open, a filtered error should close the circuit', t => {
t.plan(7);
const options = {
errorThresholdPercentage: 1,
resetTimeout: 100,
errorFilter: err => err.statusCode < 500
};

const breaker = new CircuitBreaker(failWithCode, options);
breaker.fire(500)
.then(t.fail)
.catch(e => t.equals(e.message, 'Failed with 500 status code'))
.then(() => {
t.ok(breaker.opened, 'should be open after initial fire');
t.notOk(breaker.pendingClose,
'should not be pending close after initial fire');

// Fire again after reset timeout. should be half open
setTimeout(() => {
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'))
.then(() => {
t.ok(breaker.closed,
'should be closed after failing with filtered error');
})
.then(_ => breaker.shutdown())
.then(t.end);
}, options.resetTimeout * 1.5);
});
});

0 comments on commit a721f3f

Please sign in to comment.