fix: Fix stuck open (#386)

fixes #385 by changing the behaviour of open() back to being a noop if the state is already open
nodeshift · Nov 8, 2019 · 2c5b4a2 · 2c5b4a2
1 parent a7d00e5
commit 2c5b4a2
Show file tree

Hide file tree

Showing 4 changed files with 32 additions and 37 deletions.
diff --git a/lib/circuit.js b/lib/circuit.js
@@ -207,9 +207,12 @@ class CircuitBreaker extends EventEmitter {
    * @returns {void}
    */
   close () {
-    this[PENDING_CLOSE] = false;
     if (this[STATE] !== CLOSED) {
+      if (this[RESET_TIMEOUT]) {
+        clearTimeout(this[RESET_TIMEOUT]);
+      }
       this[STATE] = CLOSED;
+      this[PENDING_CLOSE] = false;
       /**
        * Emitted when the breaker is reset allowing the action to execute again
        * @event CircuitBreaker#close
@@ -222,16 +225,15 @@ class CircuitBreaker extends EventEmitter {
    * Opens the breaker. Each time the breaker is fired while the circuit is
    * opened, a failed Promise is returned, or if any fallback function
    * has been provided, it is invoked.
+   *
+   * If the breaker is already open this call does nothing.
    * @fires CircuitBreaker#open
    * @returns {void}
    */
   open () {
-    if (this[RESET_TIMEOUT]) {
-      clearTimeout(this[RESET_TIMEOUT]);
-    }
-    this[PENDING_CLOSE] = false;
     if (this[STATE] !== OPEN) {
       this[STATE] = OPEN;
+      this[PENDING_CLOSE] = false;
       /**
        * Emitted when the breaker opens because the action has
        * failed more than `options.maxFailures` number of times.

diff --git a/test/closed-test.js b/test/closed-test.js
@@ -4,7 +4,7 @@ const test = require('tape');
 const CircuitBreaker = require('../');
 
 test(
-  'When closed, an existing request resolving does not reopen circuit',
+  'When open, an existing request resolving does not close circuit',
   t => {
     t.plan(6);
     const options = {

diff --git a/test/open-test.js b/test/open-test.js
diff --git a/test/test.js b/test/test.js
@@ -241,6 +241,30 @@ test('Breaker resets after a configurable amount of time', t => {
     });
 });
 
+test(
+  'Breaker resets after a configurable amount of time when multiple jobs fail',
+  t => {
+    t.plan(1);
+    const fails = -1;
+    const resetTimeout = 100;
+    const breaker = new CircuitBreaker(passFail,
+      { errorThresholdPercentage: 1, resetTimeout });
+
+    breaker.fire(fails);
+    breaker.fire(fails)
+      .catch(() => {
+        // Now the breaker should be open. Wait for reset and
+        // fire again.
+        setTimeout(() => {
+          breaker.fire(100)
+            .then(arg => t.equals(arg, 100, 'breaker has reset'))
+            .then(_ => breaker.shutdown())
+            .then(t.end);
+        }, resetTimeout * 1.25);
+      });
+  }
+);
+
 test('Breaker status reflects open state', t => {
   t.plan(1);
   const breaker = new CircuitBreaker(passFail,