Handle not wrapped exceptions same way as all other

[akhomchenko]

Closes #1536

First of all I want to note that this change is breaking one as far as I can see.

While most of my motivation is in #1536 and I will put an overview and some extra thoughts here:

Hystrix had that nice idea that BadRequestException was designed to bypass fallbacks. It was a really nice feature that allowed users of Hystrix to make a difference between execution errors and some conditional (so called "expected") errors e.g. validation.

#1414 #1503 introduced that nice concept of exception unwrapping which is really cool and I'm grateful to authors to have this feature. It has only one flaw - it has broken an idea of BadRequestException. Now any exception that implements ExceptionNotWrappedByHystrix leaves no choice to user if he wants to have fallbacks available - not to use it.

This PR solves this issue.
I hope I haven't broken a lot.

So the consequences of this PR:

• users need to review their logic as all fallbacks will start to work in case of exceptions marked by ExceptionNotWrappedByHystrix

[mattrjacobs]

Thanks @gagoman .

Let me restate the cases to check that I understand the implications:

• exception thrown neither extends HystrixBadRequestException nor implements ExceptionNotWrappedByHystrix. It's eligible for fallback and the exception thrown to caller is wrapped in HystrixRuntimeException.
• exception thrown extends HystrixBadRequestException but does not implement ExceptionNotWrappedInHystrix. It's not eligible for fallback and the original exception (which extends HystrixBadRequestException) is thrown to the caller
• exception thrown implements ExceptionNotWrappedInHystrix and does not extend HystrixBadRequestException. It's eligible for fallback and the unwrapped exception is thrown to the caller in the case the fallback is not implements or otherwise fails
• exception thrown both implements ExceptionNotWrappedInHystrix and extends HystrixBadRequestException. It's not eligible for fallback and the original exception (which extends from HystrixBadRequestException) is thrown to the caller.

Those sound like the correct semantics, and what your PR implements. The piece which is broken in master is the 3rd case - it does not attempt to fallback.

Did I get that right?

[akhomchenko]

The piece which is broken in master is the 3rd case - it does not attempt to fallback
Yes, the goal of PR is to fix this.

Cases:

exception thrown neither extends HystrixBadRequestException nor implements ExceptionNotWrappedByHystrix. It's eligible for fallback and the exception thrown to caller is wrapped in HystrixRuntimeException.

tests are: testExecutionFailureWithNoFallback and testExecutionFailureWithFallbackFailure

exception thrown extends HystrixBadRequestException but does not implement ExceptionNotWrappedInHystrix. It's not eligible for fallback and the original exception (which extends HystrixBadRequestException) is thrown to the caller

There are no tests that cover extends case. Only available are: testExecutionHookSemaphoreBadRequestException and testExecutionHookSemaphoreBadRequestException

exception thrown implements ExceptionNotWrappedInHystrix and does not extend HystrixBadRequestException. It's eligible for fallback and the unwrapped exception is thrown to the caller in the case the fallback is not implements or otherwise fails

tests are: testNotWrappedExceptionWithFallback and testNotWrappedExceptionWithNoFallback

exception thrown both implements ExceptionNotWrappedInHystrix and extends HystrixBadRequestException. It's not eligible for fallback and the original exception (which extends from HystrixBadRequestException) is thrown to the caller.

as in 2nd case there are no tests that cover extends case while tests that have exception wrapped in HystrixBadRequestException are available:
testNotWrappedBadRequestWithFallback and testNotWrappedBadRequestWithNoFallback

[akhomchenko]

@mattrjacobs Any update on this? Should I add/change something? Thank you.

[mattrjacobs]

Thanks @gagoman !

[fastluca]

Hi,
in my project implemented a custom exception HttpBaseException which extends HystrixBadRequestException and implements ExceptionNotWrappedByHystrix. My command throws the HttpBaseException, I expected to receive no fallback and the exception propagated, but I received the fallback instead. What I missed to obtain this behaviour? Thank you.

[akhomchenko]

Hello.

I'm not sure about your case. Could you please provide an example?

Is this your case?

private static class BadAndUnwrapped extends HystrixBadRequestException implements ExceptionNotWrappedByHystrix {
    public BadAndUnwrapped(String message) {
        super(message);
    }
}

private static class Sample extends HystrixCommand<String> {

    protected Sample() {
        super(asKey("sample"));
    }

    @Override
    protected String run() throws Exception {
        throw new BadAndUnwrapped("expected one");
    }

    @Override
    protected String getFallback() {
        throw new RuntimeException("not expected");
    }
}

public static void main(String[] args) throws Exception {
    new Sample().run();
}

generates next output:

Exception in thread "main" hystrix.HystrixSample$BadAndUnwrapped: expected one
	at hystrix.HystrixSample$Sample.run(HystrixSample.java:82)
	at hystrix.HystrixSample.main(HystrixSample.java:92)

Hystrix version: 1.5.12

[fastluca]

Hi @gagoman, thank you for your response. Yes, it was exactly my case, I've not tried directly the run method, but the execute one on HystrixCommand.
I've the same Hystrix version: 1.5.12. Now I changed my configuration, with a custom workaround implemented, but calling the execute I've the issue described in my previous comment.

[akhomchenko]

You are right about execute. Still I can not reproduce, even by using execute. Could you please provide an example?

P.S. I would suggest to debug executeCommandAndObserve and especially final Func1<Throwable, Observable<R>> handleFallback part to see how your Exception is treated.