Skip to content

Commit

Permalink
feat: add options.volumeThreshold
Browse files Browse the repository at this point in the history
This option prevents the circuit from opening unless the
number of requests during the statistical window exceeds this
threshold.

Fixes: #232
  • Loading branch information
lance committed Oct 21, 2018
1 parent 64952fd commit f9a720e
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 0 deletions.
5 changes: 5 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ const defaults = {
* allow before enabling the circuit. This can help in situations where no matter
* what your `errorThresholdPercentage` is, if the first execution times out or
* fails, the circuit immediately opens. Default: 0
* @param options.volumeThreshold {Number} the minimum number of requests within
* the rolling statistical window that must exist before the circuit breaker
* can open. This is similar to `options.allowWarmUp` in that no matter how many
* failures there are, if the number of requests within the statistical window
* does not exceed this threshold, the circuit will remain closed. Default: 0
* @return a {@link CircuitBreaker} instance
*/
function circuitBreaker (action, options) {
Expand Down
12 changes: 12 additions & 0 deletions lib/circuit.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const HYSTRIX_STATS = Symbol('hystrix-stats');
const CACHE = new WeakMap();
const ENABLED = Symbol('Enabled');
const WARMING_UP = Symbol('warming-up');
const VOLUME_THRESHOLD = Symbol('volume-threshold');
const deprecation = `options.maxFailures is deprecated. \
Please use options.errorThresholdPercentage`;

Expand Down Expand Up @@ -65,6 +66,11 @@ Please use options.errorThresholdPercentage`;
* allow before enabling the circuit. This can help in situations where no matter
* what your `errorThresholdPercentage` is, if the first execution times out or
* fails, the circuit immediately opens. Default: 0
* @param options.volumeThreshold {Number} the minimum number of requests within
* the rolling statistical window that must exist before the circuit breaker
* can open. This is similar to `options.allowWarmUp` in that no matter how many
* failures there are, if the number of requests within the statistical window
* does not exceed this threshold, the circuit will remain closed. Default: 0
*/
class CircuitBreaker extends EventEmitter {
constructor (action, options) {
Expand All @@ -78,6 +84,7 @@ class CircuitBreaker extends EventEmitter {

this.semaphore = new Semaphore(this.options.capacity);

this[VOLUME_THRESHOLD] = Number.isInteger(options.volumeThreshold) ? options.volumeThreshold : 0;
this[WARMING_UP] = options.allowWarmUp === true;
this[STATUS] = new Status(this.options);
this[STATE] = CLOSED;
Expand Down Expand Up @@ -246,6 +253,10 @@ class CircuitBreaker extends EventEmitter {
return this[WARMING_UP];
}

get volumeThreshold () {
return this[VOLUME_THRESHOLD];
}

/**
* Provide a fallback function for this {@link CircuitBreaker}. This
* function will be executed when the circuit is `fire`d and fails.
Expand Down Expand Up @@ -511,6 +522,7 @@ function fail (circuit, err, args, latency) {

// check stats to see if the circuit should be opened
const stats = circuit.stats;
if (stats.fires < circuit.volumeThreshold) return;

This comment has been minimized.

Copy link
@brbradford

brbradford Feb 20, 2019

I believe this line change has introduced a bug.
Steps to duplicate...

  1. Set volumeThreshold to something.
  2. Cause the circuit to open because of failures.
  3. After the circuit goes to halfOpen, and after time has passed so that stats.fires is back below volumeThreshold...
  4. Fire again with a call that will fail.
  5. pendingClose gets set to false, this line causes an early return and keeps the circuit in halfOpen.
  6. The circuit is now stuck in halfOpen forever and all calls get blocked.

It seems like you shouldn't return early here if the circuit is halfOpen.

This comment has been minimized.

Copy link
@lance

lance Feb 20, 2019

Author Member

Interesting - thanks for pointing this out, and I will take a look. Feel free to submit a pull request with a test reproducing this if you have the bandwidth. :)

const errorRate = stats.failures / stats.fires * 100;
if (errorRate > circuit.options.errorThresholdPercentage ||
circuit.options.maxFailures >= stats.failures ||
Expand Down
60 changes: 60 additions & 0 deletions test/volume-threshold-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
'use strict';

const test = require('tape');
const opossum = require('../');
const { passFail } = require('./common');

test('By default does not have a volume threshold', t => {
t.plan(3);
const options = {
errorThresholdPercentage: 1,
resetTimeout: 100
};

const breaker = opossum(passFail, options);
breaker.fire(-1)
.catch(e => t.equals(e, 'Error: -1 is < 0'))
.then(() => {
t.ok(breaker.opened, 'should be open after initial fire');
t.notOk(breaker.pendingClose,
'should not be pending close after initial fire');
});
});

test('Has a volume threshold before tripping when option is provided', t => {
t.plan(6);
const options = {
errorThresholdPercentage: 1,
resetTimeout: 100,
volumeThreshold: 3
};

const breaker = opossum(passFail, options);
breaker.fire(-1)
.then(t.fail)
.catch(e => {
t.notOk(breaker.opened,
'should not be open before volume threshold has been reached');
t.notOk(breaker.pendingClose,
'should not be pending close before volume threshold has been reached');
})
.then(_ => {
breaker.fire(-1)
.then(t.fail)
.catch(e => {
t.notOk(breaker.opened,
'should not be open before volume threshold has been reached');
t.notOk(breaker.pendingClose,
'should not be pending close before volume threshold has been reached');
})
.then(_ => {
breaker.fire(-1)
.catch(e => {
t.equals(e, 'Error: -1 is < 0');
t.ok(breaker.opened,
'should be open after volume threshold has been reached');
})
.then(t.end);
});
});
});

0 comments on commit f9a720e

Please sign in to comment.