fix: return errors from invocation filtered errors (#567)

This commit reverts some of the behavior changes in
#556. In that PR, the intent
was to avoid calling the fallback function when an an invocation error
is filtered by a user supplied `errorFilter` function. It did
accomplish that, however, it also changed how a function resolves
in the case of a filtered error. Previously, even though the error
was filtered, the function invocation would return the error to the
caller. With #556, this changed so that the caller instead receives
simply a truthy value.

This commit reverts that behavior so that the error is returned.

While it may seem counter intuitive to return an error when the it
was filtered by a user supplied `errorFilter` function, there are good
reasons to do so. It provides the caller with error information at the
point of invocation instead of in an event listener which may be
disconnected from the invocation code path.

The purpose of `errorFilter` is simply to prevent filtered errors from
causing the circuit to open. But the fact is that the function invocation
failed, and providing this to the user at the point of failure is better
usability in my opinion.

Plus, it's what we've always been doing, and I think the change to returning
a truthy value was really just a side effect of not calling the fallback
function. My preference would be to minimize the breaking changes in 6.x,
and this PR helps to accomplish that (albeit 6.0 will be a weird bump in
the road).

Fixes: #564

Signed-off-by: Lance Ball <lball@redhat.com>

lib/circuit.js

@@ -655,18 +655,20 @@ class CircuitBreaker extends EventEmitter {
 function handleError (error, circuit, timeout, args, latency, resolve, reject) {
   clearTimeout(timeout);
 
-  let errorFiltered;
-  if ((errorFiltered = circuit.options.errorFilter(error, ...args))) {
+  if (circuit.options.errorFilter(error, ...args)) {
+    // The error was filtered, so emit 'success'
     circuit.emit('success', error, latency);
-    resolve(errorFiltered);
   } else {
+    // Error was not filtered, so emit 'failure'
     fail(circuit, error, args, latency);
 
-    // Only call the fallback function if there errorFilter doesn't succeed
+    // Only call the fallback function if errorFilter doesn't succeed
+    // If the fallback function succeeds, resolve
     const fb = fallback(circuit, error, args);
-    if (fb) resolve(fb);
-    else reject(error);
+    if (fb) return resolve(fb);
   }
+  // In all other cases, reject
+  reject(error);
 }
 
 function fallback (circuit, err, args) {

test/error-filter-test.js

@@ -2,7 +2,7 @@
 
 const test = require('tape');
 const CircuitBreaker = require('../');
-const { failWithCode } = require('./common');
+const { failWithCode, passFail } = require('./common');
 
 const options = {
   errorThresholdPercentage: 1,
@@ -17,8 +17,9 @@ test('Bypasses failure stats if errorFilter returns true - Should return a succe
 
   const breaker = new CircuitBreaker(failWithCode, options);
   breaker.fire(400)
-    .then(returnValue => {
-      t.equal(returnValue, true, 'return value is the value of the errorFilter function');
+    .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);
@@ -76,3 +77,32 @@ test('Provides invocation parameters to error filter', t => {
       t.end();
     });
 });
+
+test('A successful errorFilter should not invoke the fallback function', t => {
+  t.plan(1);
+  const breaker = new CircuitBreaker(passFail,
+    {
+      errorThresholdPercentage: 1,
+      errorFilter: _ => {
+        t.ok(true, 'Error filter invoked');
+        return true;
+      },
+      fallback: _ => t.fail('Fallback function should not be called')
+    });
+  breaker.fire(-1).catch(_ => t.end());
+});
+
+test('A successful errorFilter should still return the error from function invocation', t => {
+  t.plan(2);
+  const breaker = new CircuitBreaker(passFail,
+    {
+      errorThresholdPercentage: 1,
+      errorFilter: _ => t.ok(true, 'Error filter invoked'),
+      fallback: _ => t.fail('Fallback function should not be called')
+    });
+  breaker.fire(-1).catch(err => {
+    console.error(err);
+    t.equal(err, 'Error: -1 is < 0');
+    t.end();
+  });
+});

test/half-open-test.js

@@ -48,7 +48,7 @@ test('When half-open, the circuit only allows one request through', t => {
 });
 
 test('When half-open, a filtered error should close the circuit', t => {
-  t.plan(6);
+  t.plan(7);
   const options = {
     errorThresholdPercentage: 1,
     resetTimeout: 100,
@@ -69,7 +69,10 @@ test('When half-open, a filtered error should close the circuit', t => {
         t.ok(breaker.halfOpen, 'should be halfOpen after timeout');
         t.ok(breaker.pendingClose, 'should be pending close after timeout');
         breaker
-          .fire(400) // pass with a filtered error
+          .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 passing with filtered error');