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

Should fallback be applied when circuit is closed? #196

Closed
duartemendes opened this issue May 23, 2018 · 6 comments
Closed

Should fallback be applied when circuit is closed? #196

duartemendes opened this issue May 23, 2018 · 6 comments

Comments

@duartemendes
Copy link
Contributor

This is not a bug per say, but I'm also not sure this is the correct behavior.

Shouldn't the fallback function only be applied when the circuit is open or half open?

I'm using this package to make http requests to several upstreams. And having the fallback being called when the fire action fails means that we lose the actual response from the upstream.

My suggestion is to invoke the fallback function only when the event 'reject' is also emitted. This will allow us to propagate the actual upstream response to above. Since we already have the response we might as well use it for debug and monitoring purposes.

@hugoduraes
Copy link
Contributor

@duartemendes Alternatively, what if the error was passed as a parameter to the fallback function? Would that be useful? Have a look at my PR #197

@duartemendes
Copy link
Contributor Author

Thanks @hugoduraes. I tried it out and it solves my problem.

@lance can you take a look at @hugoduraes PR?

@lance
Copy link
Member

lance commented May 24, 2018

@duartemendes it's important for the fallback function to be executed whether the circuit is closed, open or half open, otherwise there would simply be a failure. The solution provided by @hugoduraes is great, in my opinion. I will merge it and close this issue. Thanks both of you for contributing. Would you mind telling me what project you are using this in, if it's open source?

@lance
Copy link
Member

lance commented May 24, 2018

@duartemendes @hugoduraes I've just published version 1.6.0 containing this fix

@duartemendes
Copy link
Contributor Author

@lance It's not open source :)

Thanks a lot for this package and the help with this subject!

@hugoduraes
Copy link
Contributor

Thanks @lance. :)

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

No branches or pull requests

3 participants