From a721f3f345f67cdf0917d75d637c4e8bfbce0e8e Mon Sep 17 00:00:00 2001
From: Jair Elton Batista de Souza <jaireltonb@gmail.com>
Date: Tue, 17 Dec 2019 10:34:42 -0300
Subject: [PATCH] bug: fix stuck halfOpen when using errorFilter (#389)

* 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.
---
 lib/circuit.js            |  8 +++++---
 test/common.js            |  9 ++++++++-
 test/error-filter-test.js | 16 ++++++----------
 test/half-open-test.js    | 38 +++++++++++++++++++++++++++++++++++++-
 4 files changed, 56 insertions(+), 15 deletions(-)

diff --git a/lib/circuit.js b/lib/circuit.js
index 8ef1e5ec..f9f2368a 100644
--- a/lib/circuit.js
+++ b/lib/circuit.js
@@ -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
diff --git a/test/common.js b/test/common.js
index a9310487..0bba12f2 100644
--- a/test/common.js
+++ b/test/common.js
@@ -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
 };
diff --git a/test/error-filter-test.js b/test/error-filter-test.js
index 214957de..4df5d289 100644
--- a/test/error-filter-test.js
+++ b/test/error-filter-test.js
@@ -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)
diff --git a/test/half-open-test.js b/test/half-open-test.js
index 42edb2e2..dc7fc88f 100644
--- a/test/half-open-test.js
+++ b/test/half-open-test.js
@@ -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);
+    });
+});