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

Allow (selected) exception to propagate unwrapped #1283

Closed
tbvh opened this issue Jul 19, 2016 · 5 comments
Closed

Allow (selected) exception to propagate unwrapped #1283

tbvh opened this issue Jul 19, 2016 · 5 comments

Comments

@tbvh
Copy link

tbvh commented Jul 19, 2016

Using a command, and javanica in particular, adds a (hidden) source for HystrixRuntimeExceptions to the code. If the logic that's being wrapped is capable of throwing exceptions, any surrounding catch block is now useless, unless the HystrixRuntimeException is unwrapped.

In my case I've used javanica to add hystrix on top of a service layer that can throw 'business' exceptions. I want to see those, so there is no fallback implemented. Now I've wrapped every call to javanica annotated methods to unwrap the HystrixRuntimeException.

Instead I would like to be able to

  1. mark a (Runtime)Exception as intended to be thrown unwrapped.
  2. in the case of javanica, any (checked?) exception that the annotated method is declaring it can throw should be thrown unwrapped.

Nb. In both cases I intend to let these exception count against the errors of the command and to potentially trigger the circuit breaker, if one is configured.

To mark RuntimeExceptions I'm thinking along the lines of an interface like HystrixUnwrappedException. Any exception that implements this interface, should be rethrown and not be wrapped in HystrixRuntimeException, if they are caught by Hystrix.

Is there any interest in such a feature? Maybe if 1 and 2 are considered separately?

@mattrjacobs
Copy link
Contributor

Yeah, I can imagine this being useful. I'd be happy to review a PR for it. Good idea!

@ashwgupt
Copy link

ashwgupt commented Aug 11, 2016

+1 for it.

Can it then also enable one to decide upon fallback method name based upon the original (checked) exception?

That can help one to define various fallback paths for a Hystrix protected method to take based upon the error/exception received.

Or is that something already possible with the library?

@tbvh
Copy link
Author

tbvh commented Nov 6, 2016

I added a pr that fixes 1, by defining an Interface that can be implemented by exceptions. Those exceptions will be thrown as-is, not wrapped by HystrixRuntimeException. The fallback is not triggered for these exceptions.

I want to extend on this functionality for 2, by overriding decomposeException() and shouldNotBeWrapped() in the Javanica implementations.

Feedback is welcome.

@tbvh
Copy link
Author

tbvh commented Dec 14, 2016

When I wanted to pick up work on point 2, I noticed two things.

  1. The implications of the change are much larger than I originally envisioned.
    1. The behavior conflict with the behavior of the ignoreExceptions annotation argument.
    2. Because runtime exceptions are already unwrapped, basically nothing will get wrapped in the end.
  2. I was using an older version of hystrix, and can upgrade to make use of 155d4e9 to have unwrapped runtime exceptions.

Because of this, and because the merge request already covers the spirit of the issue (and will work for javanica too), I propose to leave it at that. And close this issue once the merge request is accepted.

NB. IMO point 1.ii is actually the correct behavior for an aspected solution. But I suppose this discussion needs to be held somewhere else.
The question is: does it act as an aspect circuit breaker, or does it only accomodate annotation driven Hystrix commands.
In the latter case checked exceptions are a bit of a dealbreaker since they 'leak' from the annotated method.
Therefore I would prefer unwrapped checked exceptions, since the callers are already forced (by the compiler) to handle them.

@mattrjacobs
Copy link
Contributor

Thanks @tbvh!

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