Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fallback function is being executed when errorFilter is applied #540

Closed
jerrin opened this issue Feb 10, 2021 · 5 comments
Closed

Fallback function is being executed when errorFilter is applied #540

jerrin opened this issue Feb 10, 2021 · 5 comments

Comments

@jerrin
Copy link

jerrin commented Feb 10, 2021

Node.js Version: v12.18.2

Operating System: macOS 10.15.7 (19H114)

Steps to Produce Error:

According to https://github.com/nodeshift/opossum#fallback, the fallback function is supposed to be executed only when the circuit is open. But, when the error filter returns true, (for eg: error filter is applied to exclude HTTP 404, errorFilter: err => err.statusCode < 500) the circuit remain closed and the fallback function is still being executed.

The issue seems to be here:

opossum/lib/circuit.js

Lines 658 to 664 in 3cb7de9

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

Would you be able to check if the fallback is supposed to be called in this scenario?
Thanks

@lance
Copy link
Member

lance commented Feb 18, 2021

Hi @jerrin. This is a good catch. It's actually working as intended, but the docs don't reflect that. A little over a year ago, this was discussed in a pull request. My original ask was to change this behavior so that fallback would not be called. But ultimately we didn't go that route because it was considered a breaking change. @lholmquist should we revisit this?

@lholmquist
Copy link
Member

This must have slipped through the email cracks, will review today

@lholmquist
Copy link
Member

It does feel a little odd that if the error filter "succceds" that we are still calling the fallback, that scenario should be considered a success.

I have no problems with doing a semver-major change to update this.

@jerrin
Copy link
Author

jerrin commented Feb 27, 2021

Thank you both for looking into this 👍

@lholmquist
Copy link
Member

Just released this fix in 6.0.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants