Hystrix Javanica Error Propagation does not work correctly

[ChristianLohmann]

Hi,

i'm very new to Hystrix/Javanica. But i think i found a bug in error propagation:

I have the following setup:

//J-
    @HystrixCommand(
            fallbackMethod = "getArticleModelFallback",
            ignoreExceptions = {SkuParsingFailedException.class, ModelNotFoundException.class},
            commandProperties = {
                    @HystrixProperty(name = "execution.isolation.thread.timeoutInMilliseconds", value = "15000"),
                    @HystrixProperty(name = "circuitBreaker.requestVolumeThreshold", value = "1"),
                    @HystrixProperty(name = "circuitBreaker.sleepWindowInMilliseconds", value = "20000")
            }
    )
    //J+
    public ArticleModel getArticleModel(final String modelSku) throws ModelNotFoundException {
        final ModelSku sku;
        try {
            sku = ModelSku.valueOf(modelSku);
        } catch (final SkuParsingFailedException e) {
            throw new ModelNotFoundException("Bad or incorrect SKU!", modelSku);
        }

        final ArticleModel articleModel;
        try {
            articleModel = articleWebService.getArticle(sku);
            if (articleModel == null) {
                throw new ModelNotFoundException(modelSku);
            }
        } catch (final SOAPFaultException e) {
            throw new ModelNotFoundException(modelSku);
        }

        return articleModel;
    }

The getArticleModel() method is being called by a RestController method.

When an SkuParsingFailedException is being catched, it will correctly wrap the newly thrown ModelNotFoundException into a HystrixBadRequestException. But when i try to catch any Exception with a @ControllerAdvice and @ExceptionHandlerannotated method, i'll always catch a RuntimeException with a wrapped ModelNotFoundException.

@ControllerAdvice
public class GlobalDefaultExceptionHandler {

    @ResponseStatus(HttpStatus.NOT_FOUND) // 404
    @ExceptionHandler(ModelNotFoundException.class)
    @ResponseBody
    public String handleModelNotFoundException(final ModelNotFoundException ex) {
        return ex.getMessage();
    }

    @ResponseStatus(HttpStatus.INTERNAL_SERVER_ERROR) // 500
    @ExceptionHandler(Exception.class)
    @ResponseBody
    public String handleAnyException(final Exception ex) {
        return ex.getMessage();
    }

handleModelNotFoundException()will not be called. Instead handleAnyException() will be called with a RuntimeException as parameter.

If i comment out the @HystrixCommand ignoreExceptions property and replace throw new ModelNotFoundException("Bad or incorrect SKU!", modelSku);with throw new HystrixBadRequestException("Bad or incorrect SKU given!",new ModelNotFoundException(modelSku)); then it works as expected and a ModelNotFoundException will be catched.

Can anybody help?

Thanks!

[davidkarlsen]

Which hystrix/javanica version are you running - there were changes in #241 .

[ChristianLohmann]

I'm using 1.4.0-RC4
And yes, i checked #241 before.

[ChristianLohmann]

Ok, i found the problem:

the exception will be propagated by following method in MethodExecutionAction:

private void propagateCause(Throwable throwable) {
        throw Throwables.propagate(throwable.getCause());
    }

But my custom exception is of type Exception NOT of type RuntimeException!
According to guavas javadoc, Throwables.propagate() wraps any throwable which is not an instance of RuntimeException with a RuntimeException.
Now it gets even more confusing when the newly thrown RuntimeException passes this check in AbstractHystrixCommand:

boolean isIgnorable(Throwable throwable) {
        if (ignoreExceptions == null || ignoreExceptions.length == 0) {
            return false;
        }
        for (Class<? extends Throwable> ignoreException : ignoreExceptions) {
            if (throwable.getClass().isAssignableFrom(ignoreException)) {
                return true;
            }
        }
        return false;
    }

The first item in my ignoreExceptions array is SkuParsingFailedException.class which IS of type RuntimeException. Thus if (throwable.getClass().isAssignableFrom(ignoreException)) produces a "false positive" match because RuntimeException is assignable from the superclass of SkuParsingFailedException.
That means that any exception defined as "ignorable" which is of type RuntimeException would produce a positive match.

To solve my current problem i have to make sure that i'll only throw subclasses from RuntimeException.
And defining any Non-RuntimeExceptions in ignoreExceptions doesn't make sense.

[dmgcodevil]

@ChristianLohmann Thanks for the response. I need more time to investigate the issue and make fix, I'm gonna do it as soon as possible.

looks like you'r right:

if (throwable.getClass().isAssignableFrom(ignoreException)) {
                return true;
            }

must be changed to

if ( ignoreException.isAssignableFrom(throwable.getClass())) {
                return true;
            }

So I need to look deeper into your example to make a fix. Will do it ASAP

[dmgcodevil]

Also need to rethrow original exception instead of throw Throwables.propagate(throwable);

[ChristianLohmann]

Thanks for Your fast response!

[dmgcodevil]

I create PL #334
I added more tests to check the cases you mentioned, but since your example is complicated I can't be 100% sure that I've fixed issues occured in your case. Thus I built javanica with my changes and attached jar file, you can try it until @benjchristensen not merge changes. Also I created simple diagram that describes error propogation workflow. Thanks.

download jar https://drive.google.com/file/d/0B0O7Taelg2EaYm5yYUgtSlVSUHc/view?usp=sharing

[ChristianLohmann]

Thanks, it works like expected now! The ExceptionHandler now catches the right exceptions, even if they are checked exceptions.

Waiting for the patch now ;)

[dmgcodevil]

@ChristianLohmann , I'm glad to hear it. @benjchristensen could you please merge this changes? Also, does it make sence to make same changes into 1.3.x branch ? If most people use 1.4.+ then I guess PL into main branch is enough. Thoughts?

[dmgcodevil]

My changes were merged to main branch, @ChristianLohmann can I close this issue.

[ChristianLohmann]

Yes please! Thanks again!
Am 11.11.2014 18:35 schrieb "Roman Pleshkov" notifications@github.com:

My changes were merged to main branch, @ChristianLohmann
https://github.com/ChristianLohmann can I close this issue.

—
Reply to this email directly or view it on GitHub
#332 (comment).

[dmgcodevil]

@benjchristensen assign please this issue to me or close it.