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

More precise solution to #771 #780

Merged
merged 4 commits into from
Apr 30, 2015

Conversation

mattrjacobs
Copy link
Contributor

Instead of try-catching all hook errors, follow this pattern:

  • For a hook at the beginning of an event, assume that a thrown exception replaces the surrounded event. For example, onExecutionStart() throwing implies that the run()/construct() method does not get run, but it looks from the outside as if it threw an exception.
  • For a hook at the end of an event, there is no such ability to replace a value, so just try-catch and log.

This allows for hooks to influence behavior without having to subclass HystrixCommand. I've also added a unit test that asserts state is properly maintained when hooks throw Exceptions

@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #113 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder

NetflixOSS » Hystrix » Hystrix-pull-requests #114 SUCCESS
This pull request looks good

mattrjacobs added a commit that referenced this pull request Apr 30, 2015
@mattrjacobs mattrjacobs merged commit 57df50b into Netflix:master Apr 30, 2015
@mattrjacobs mattrjacobs deleted the allow-hooks-to-throw-safely branch April 30, 2015 12:50
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

Successfully merging this pull request may close these issues.

2 participants