Hystrix Javanica Unwraps and Raises Checked Exceptions

[56quarters]

Hi,

Starting with hystrix-javanica 1.5.4 (up to the latest master) it seems that HystrixCommandAspect starts unconditionally unwrapping HystrixRuntimeExceptions and re-raising the cause, no matter what. This becomes a problem when Hystrix commands time out. In this case the cause will be an instance of TimeoutException which is a checked exception. Thus callers of a method that times out get an UndeclaredThrowableException instead of a HystrixRuntimeException.

I've created a demo project with a @HystrixCommand that always times out to demonstrate. This is a Spring Boot project but the problem is unrelated to Spring Boot.

Steps

git clone https://github.com/56quarters/hystrix-demo.git
cd hystrix-demo
./mvnw test

Expected behavior

The method call raises a HystrixRuntimeException with failureType == TIMEOUT and cause of TimeoutException

Actual behavior

The method call raises an UndeclaredThrowableException with cause of TimeoutException

This seems to be related to #1285 and #1344 and caused by the changes in 155d4e9

I think the fix for this is something to the effect of changing HystrixCommandAspect.getCauseOrDefault to look like the following:

    private Throwable getCauseOrDefault(HystrixRuntimeException e, RuntimeException defaultException) {
        if (e.getCause() == null) return defaultException;
        if (e.getCause() instanceof CommandActionExecutionException) {
            CommandActionExecutionException commandActionExecutionException = (CommandActionExecutionException) e.getCause();
            return Optional.fromNullable(commandActionExecutionException.getCause()).or(defaultException);
        }
        if (e.getFailureType() != FailureType.COMMAND_EXCEPTION) return defaultException;
        return e.getCause();
    }

Note the addition of the final if to only reraise the cause when the failure is from an exception thrown by the wrapped method.

Thanks.

/cc @hroongta

[mattrjacobs]

@dmgcodevil can you take a look at this one and recommend a path forward?

[dmgcodevil]

Hi @56quarters it's hard to say that previous behavior was correct and new one is completely wrong. There is long discussion on this subject but it still not clear which design is correct. In this case your suggestion is about wrapping platform errors in HystrixRuntimeException that sounds reasonable for me. I'm fine to combine this approach with #1344 (comment)

@56quarters are you ok with the approach that I suggested in the linked comment from the separate issue ?

[56quarters]

Hi @dmgcodevil,

I think the approach outlined in the comment will work combined with wrapping platform errors in HystrixRuntimeException

Thanks! 👍

Should I leave this issue open? Or close in favor of #1344?

[dmgcodevil]

hi @56quarters , yes, close this one and lets track the progress in #1344. Thanks !