From 2c5b4a228828cec88fcc9e062fc1abab7d2e85d8 Mon Sep 17 00:00:00 2001
From: Tom Jenkinson <tjenkinson@users.noreply.github.com>
Date: Fri, 8 Nov 2019 15:29:29 +0000
Subject: [PATCH] fix: Fix stuck open (#386)

fixes #385 by changing the behaviour of open() back to being a noop if the state is already open
---
 lib/circuit.js      | 12 +++++++-----
 test/closed-test.js |  2 +-
 test/open-test.js   | 31 -------------------------------
 test/test.js        | 24 ++++++++++++++++++++++++
 4 files changed, 32 insertions(+), 37 deletions(-)
 delete mode 100644 test/open-test.js

diff --git a/lib/circuit.js b/lib/circuit.js
index d8011eda..8ef1e5ec 100644
--- 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
index 80379b37..ff77f4ad 100644
--- 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
deleted file mode 100644
index 5ac233a7..00000000
--- a/test/open-test.js
+++ /dev/null
@@ -1,31 +0,0 @@
-'use strict';
-
-const test = require('tape');
-const CircuitBreaker = require('../');
-const { timedFailingFunction } = require('./common');
-
-test('when open() is called it cancels the resetTimeout', t => {
-  t.plan(5);
-  const options = {
-    errorThresholdPercentage: 1,
-    resetTimeout: 100
-  };
-
-  const breaker = new CircuitBreaker(timedFailingFunction, options);
-  breaker.fire(0)
-    .then(t.fail)
-    .catch(e => t.equals(e, 'Failed after 0'))
-    .then(() => {
-      t.ok(breaker.opened, 'should be open after initial fire');
-      t.notOk(breaker.pendingClose,
-        'should not be pending close after initial fire');
-      breaker.open();
-    });
-
-  setTimeout(() => {
-    t.ok(breaker.opened, 'should not have been opened by the resetTimeout');
-    t.notOk(breaker.pendingClose,
-      'should still not be pending close');
-    t.end();
-  }, options.resetTimeout * 1.5);
-});
diff --git a/test/test.js b/test/test.js
index e250d3e9..15f6bf8b 100644
--- 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,