Successful error filter shouldn't invoke the fallback function #556
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lance
approved these changes
Mar 17, 2021
lance
added a commit
to lance/opossum
that referenced
this pull request
Apr 15, 2021
This commit reverts some of the behavior changes in nodeshift#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 nodeshift#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: nodeshift#564 Signed-off-by: Lance Ball <lball@redhat.com>
lholmquist
pushed a commit
that referenced
this pull request
Apr 15, 2021
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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This address #540 and is a semver change