From c7793fb6b4ce25535964286a91c68b18ba18885f Mon Sep 17 00:00:00 2001 From: Lance Ball Date: Thu, 15 Apr 2021 17:12:35 -0400 Subject: [PATCH] fix: return errors from invocation filtered errors This commit reverts some of the behavior changes in https://github.com/nodeshift/opossum/pull/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: https://github.com/nodeshift/opossum/issues/564 Signed-off-by: Lance Ball --- lib/circuit.js | 14 ++++++++------ test/error-filter-test.js | 36 +++++++++++++++++++++++++++++++++--- test/half-open-test.js | 7 +++++-- 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/lib/circuit.js b/lib/circuit.js index bbbf3fb3..c94b950f 100644 --- a/lib/circuit.js +++ b/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) { diff --git a/test/error-filter-test.js b/test/error-filter-test.js index 5709a2b2..48437c14 100644 --- a/test/error-filter-test.js +++ b/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(); + }); +}); diff --git a/test/half-open-test.js b/test/half-open-test.js index 9fe594b1..c3616baf 100644 --- a/test/half-open-test.js +++ b/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');