Problems not handling Error classes

[luan-cestari]

Hi,

I'm using Hystrix in several projects in my organization (pretty nice idea and framework, thank you and congratulations by the way) but when I started to test some behaviors I felt quite surprised by the way it handles java.lang.Error classes. In summary, I thought that it should only wrap or/and re-throw to fallback method Exceptions. Is it this way for propose? Isnt it a dangerous default approach? Do you have recommendations about this case? I will give as much as details I found so I could make clear what I think, but might be very verbose.

So, in the fallback, I notice that point when I saw that /HystrixCommand.html#getFailedExecutionException() returned a java.lang.Throwable. So I tried to investigate more and I create a simple project with some tests to check the behavior when the run() method throw an Error and as I mentioned Hystrix doesn't consider a big problem and the test runs fine (output from the tests ).

About the java.lang.error, as Java lang spec says in its documentation, the Error classes indicate serious problems and we should not catch it (like, it can be an OoME or ThreadDeath or others)

I also tried to see any related issue and the most similar one was this one (but it isn't the same thing as this issue) but lead me a good diagram about how Hystrix work about Exceptions/Errors. So I head to the documentation to find all the details, so I read about ErrorPropagation with Fallback mechanism ( also the patterns that you guys recommend, and my case is the cache via network case ). I saw a little bit about the source code and I think the point that needs to change would be this one .

Thank you in advance.

Note maybe the picture from this comment should be available in the wiki by the way

[mattrjacobs]

Thanks for the feedback, @luan-cestari . To make sure I understand, the current state of things is that a java.lang.Error is handled as any other Throwable would be (i.e. may enter a fallback flow), and you're surprised by that and think it should immediately propagate out? Did I summarize your comment accurately?

[luan-cestari]

Yeah, It can be an OutOfMemory problem that the Hystrix could wrap and if the caller is not aware of this behavior (which I think is a bit strange by default), the client could just swallow the exception (maybe someone would just log it and re-thrown the wrapper exceptions, for example)

[mattrjacobs]

Here's how RxJava handles this: https://github.com/ReactiveX/RxJava/blob/1.x/src/main/java/rx/exceptions/Exceptions.java#L76

Some java.lang.Errors are fatal, but not all. The code referenced above makes a distinction between them. If similar code was used in Hystrix, then those that are fatal could be propagated without a fallback, and the rest using the usual flow.

Does that sound better to you, @luan-cestari ? Also, thanks for raising the issue - this seems like a useful change.

[luan-cestari]

Sounds perfect to me. Nice implementation on RxJava, I thought the source of the problem could be on that implementation (which was made one year ago (ReactiveX/RxJava@2a4e9c6 by the way) but I realized that is a sort of util method that the Observer would put int he chain of the stream to only throw (and stop the stream) if it is one of those exceptions, and these things would be implemented somewhere in HystrixCommand, right?

Thank you for the cooperation and understating

[mattrjacobs]

@luan-cestari Can you take a look at the linked PR?

One thing to note is that OOME is not treated as unrecoverable. I am using the specific logic that was discussed in the linked RxJava PR (ReactiveX/RxJava#748 (comment)) and the 4 that are treated as unrecoverable are:

• StackOverflowError
• VirtualMachineError
• ThreadDeath
• LinkageError

[mattrjacobs]

I'm merging this in now. If there are further pieces of work around java.lang.Error handling, please either re-open this issue, or create a new one.

[luan-cestari]

Sorry @mattrjacobs , I didn't see the notification about the PR you made.

I just made a quick review on it and I got just one question on isUnrecoverable method (

Hystrix/hystrix-core/src/main/java/com/netflix/hystrix/AbstractCommand.java

Lines 857 to 864 in 302fa70

	Throwable cause = t.getCause();
	if (cause instanceof StackOverflowError) {
	return true;
	} else if (t instanceof VirtualMachineError) {
	return true;
	} else if (t instanceof ThreadDeath) {
	return true;
	} else if (t instanceof LinkageError) {

). I noticed that first you check using the cause of the exception ('t.getCause()') and then the exception itself ('t') , is that right? I thought that maybe you have an specific use case which only the StackOverflowError would need this (which would make sense as you have a conditional for StackOverflowError and another for VirtualMachineError which StackOverflowError is a subclass , and as they evaluate a different Throwable object , that seems right https://docs.oracle.com/javase/8/docs/api/java/lang/VirtualMachineError.html )

[mattrjacobs]

Yes, stupid bug on my part. Of course, only the StackOverflowError branch was tested. Thanks for the review - I appreciate it.

[mattrjacobs]

Fixed in #741 - thanks again for the careful review

[luan-cestari]

welcome =)