Current behavior of the errorFilter option

[mNalon]

Node.js Version:

v12.21.0

Operating System:

macOS

Steps to Produce Error:

In the last release, when I have an action that returns a rejected promise with an error that I want to filter, the output of the fire method becomes a resolved promise with the truthy value returned by the errorFilter callback option. Is this intentional?

I believe this is happening because of the change added by this line

opossum/lib/circuit.js

Line 661 in 7a920bf

resolve(errorFiltered);

If this change was intentional, how can I get the error thrown by the action in the case of an error that is being filtered by the errorFilter?

[lholmquist]

this change was made intentionally. The thought was that if there is an error, but you have a error filter that passes, then that really isn't an error and the the fallback function shouldn't be called, which was happening before the change was made.

The error should still be thrown if the error filter fails, if that doesn't happen, then there is a bug somewhere

[mNalon]

Hi, @lholmquist, thank you for replying quickly!

In my case, I don't have a fallback function. I used to use the filterError only to ignore some errors and don't consider them to open the circuit. But still, I depend on these errors being exposed by the fire method to take the correct actions out of the circuit breaker logic.

Do you have any suggestions on how I could keep this behavior?

[lance]

But still, I depend on these errors being exposed by the fire method to take the correct actions out of the circuit breaker logic.

I'm not clear on what you mean by "these errors being exposed by the fire method". You do still have access to the errors themselves in your errorFilter() function.

You also receive these errors when listening for success events.

Would it help for the resolved promise to contain both a truthy value, and the caught exceptions? E.g.

function handleError (error, circuit, timeout, args, latency, resolve, reject) {
  let errorFiltered;
  if ((errorFiltered = circuit.options.errorFilter(error, ...args))) {
    circuit.emit('success', error, latency);
    // Provide the error
    resolve({ err: error, filtered: errorFiltered});
  }
  // ... etc
}

[mNalon]

Hi, @lance, thank you for your reply!

What I meant was related to the fire function.

In the previous release, this code:

cb = new CircuitBreaker(
  () => Promise.reject('error that should not increment the failure statistic'), 
  { errorFilter: (err) => true } // filtering everything
)

cb.fire().then(console.log).catch(console.error)

resulted in the console.error exhibiting the error message, because the fire function returned a rejected promise with the error, even for filtered errors (when we return true in the errorFilter, for example).

Right now we have the console.log exhibiting the truthy value returned by the errorFilter callback.

Using your suggestion, to be able to get the error using the returned promise by the fire function, and treat this as an error outside of the circuit breaker logic, I should do something like this:

cb = new CircuitBreaker(
  () => Promise.reject('error that should not increment the failure statistic'), 
  { 
    errorFilter: (error) => {
      // if error shoud not increment the failure statistic
      return true

      // else 
      return false
    } 
  } 
)

cb.fire().
  then((result) => {
    if(result.err) {
      throw resul.err
    }
    // logic to deal with a successful action
  })
  .catch((error) =>  {
    // logic to deal with a failed action
  })

Is it right?

I couldn't analyze the code deeply, but it seems we are increasing the responsibility of the errorFilter, aren't we?

Does the purpose of the errorFilter is to act as a manager to change the response of the action too?

[lance]

I see your point @mNalon...

@lholmquist in #540 the issue was that the fallback function was being called even when the error filter was applied and the error was filtered. The change in #556 correctly addressed this by eliminating that call. But it also changed the logic of resolve/reject for these errors. Now, if an error is filtered, it's always resolved. Whereas before, it was almost always rejected. It only resolved when the fallback function was called and was successful.

This is tricky logic because of all the possible cases. I am leaning towards saying the change in #556 was in error regarding promise resolution. I propose we consider modifying the handleError() function to restore the promise resolution behavior as it was. E.g.

function handleError (error, circuit, timeout, args, latency, resolve, reject) {
  clearTimeout(timeout);

  let errorFiltered;
  if ((errorFiltered = circuit.options.errorFilter(error, ...args))) {
    circuit.emit('success', error, latency);
  } else {
    fail(circuit, error, args, latency);

    // Only call the fallback function if there errorFilter doesn't succeed
    const fb = fallback(circuit, error, args);
    if (fb) resolve(fb);
    return;
  }
  reject(error);
}

And then quickly release a 6.1.0. What do you think?

[lholmquist]

It seems a little weird that we would emit a success, but also return a rejected promise.

treat this as an error outside of the circuit breaker logic

I'm also sort of confused by this statement. If you aren't treating this as an error in the CB logic, then why would you need to treat it as an error outside the CB logic? Maybe using the error filter isn't the right approach here? 🤷

[lance]

It seems a little weird that we would emit a success, but also return a rejected promise.

It does, I agree. But let me try and explain the thinking. The errorFilter function has a singular purpose, and that is to allow developers to filter errors from the statistics so that expected errors will not affect the open/close logic. Whether or not the error is filtered from the stats, the fact is that the function invocation did produce an error. My proposed change above would actually bring us in-line with where we were before 6.0.0. Prior to this release, we emitted success and rejected the promise every time - unless a fallback function succeeded.

opossum/lib/circuit.js

Lines 655 to 667 in 1a3b0b4

	function handleError (error, circuit, timeout, args, latency, resolve, reject) {
	clearTimeout(timeout);
	if (circuit.options.errorFilter(error, ...args)) {
	circuit.emit('success', error, latency);
	} else {
	fail(circuit, error, args, latency);
	}
	const fb = fallback(circuit, error, args);
	if (fb) resolve(fb);
	else reject(error);
	}

[lholmquist]

released as 6.0.1