Cleaner error propagation in hystrix core.

[tjuchniewicz]

Fixes gh-1366.

[tjuchniewicz]

Maybe this feature should be optional and activated by property for backward compatibility.

[mukteshkrmishra]

I agree and was going to suggest that this feature should be Optional as it is not required for every use case using Hystrix.

[tjuchniewicz]

Please take a look at code changes if fix is applied in the right place. Then I will continue with implementation, tests and docs.

[mukteshkrmishra]

Will take a look in some time.

[mattrjacobs]

I understand the desire for this feature, but after reviewing it, I'm not convinced it belongs in hystrix-core. There is already an existing mechanism for picking a set of exceptions to ignore in hystrix-javanica. There's no such equivalent in hystrix-core, so I think the expectation has been established that Hystrix will always return wrapped exceptions, whether wrapping in a HystrixRuntimeException or a HystrixBadRequestException.
To me, this change in isolation introduces inconsistency between BAD_REQUEST and the other failure types. I can imagine a different world where Hystrix returns unwrapped Exceptions everywhere, but that's not what all of the code in the wild expects, and it's not clearly an improvement on the current state, IMO.

Thoughts?

[mattnelson]

-1 to this change being the new default. It would have to user enabled since it would be non-passively altering behavior. The zuul[1] route filter is expecting the exception to be HystrixRuntimeException, I've extended this class and added additional error handling that would've been broken by this change. This change feels like something that could be configured via HystrixPlugins[2] where you supply your DecomposeStrategy

[1] https://github.com/Netflix/zuul/blob/zuul-1.0.28/zuul-netflix-webapp/src/main/groovy/filters/route/ZuulNFRequest.groovy#L151
[2] https://github.com/Netflix/Hystrix/blob/v1.5.6/hystrix-core/src/main/java/com/netflix/hystrix/strategy/HystrixPlugins.java

[tjuchniewicz]

@mattrjacobs, @mattnelson thanks for your thoughts. I agree that it can't be a default. I will try with HystrixPlugins.

[rmaciej1983]

I'm not sure if it's possible with HystrixPlugins. I tried to do this in following way.
HystrixPlugins has a method registerCommandExecutionHook(). With this method you can register a HystrixCommandExecutionHook. HystrixCommandExecutionHook can catch an exception that was generated while executing a HystrixCommand.
This is my example of implementation of HystrixCommandExecutionHook:

public class MyHystrixCommandExecutionHook extends HystrixCommandExecutionHook {

    private static MyHystrixCommandExecutionHook INSTANCE = new MyHystrixCommandExecutionHook();

    private MyHystrixCommandExecutionHook() {
    }

    public static MyHystrixCommandExecutionHook getInstance() {
        return INSTANCE;
    }       

    @Override   
    public <T> Exception onError(HystrixInvokable<T> commandInstance, FailureType failureType, Exception e) {
        System.out.println("onError");      
        e.printStackTrace();
        return new Exception("My exception 2"); 
    }   

}

Note that in onError() method I replaced oryginal exception by exception with message: "My exception 2".
Example of command that thorws an exception:

public class CommandHelloWorld extends HystrixCommand<String> {

    public CommandHelloWorld() {
        super(HystrixCommandGroupKey.Factory.asKey("ExampleGroup"));
    }

    @Override
    protected String run() throws Exception {
            throw new Exception("My exception");
    }
}

Now the command has to be executed:

Hystrix.reset();        
HystrixPlugins.reset();
HystrixPlugins.getInstance().registerCommandExecutionHook(MyHystrixCommandExecutionHook.getInstance());
new CommandHelloWorld().execute(); 

First it's invoked onError() method. So stack trace of the oryginal exception is printed.
Then HystrixRuntimeException is printed. This is wrapper for oryginal exception. Message is: "My exception 2". So decomposeException() is called after onError().
I saw HystrixCommand source codes and execute() method looks like this:

public R execute() {
        try {
            return queue().get();
        } catch (Exception e) {
            throw decomposeException(e);
        }
}

Let's see qeue() method:

public Future<R> queue() {
        final Observable<R> o = toObservable();
        final Future<R> f = o.toBlocking().toFuture();
        if (f.isDone()) {
            try {
                f.get();
                return f;
            } catch (Exception e) {
                RuntimeException re = decomposeException(e);
                if (re instanceof HystrixBadRequestException) {
                    return f;
                } else if (re instanceof HystrixRuntimeException) {
                    HystrixRuntimeException hre = (HystrixRuntimeException) re;
                    if (hre.getFailureType() == FailureType.COMMAND_EXCEPTION || hre.getFailureType() == FailureType.TIMEOUT) {
                        // we don't throw these types from queue() only from queue().get() as they are execution errors
                        return f;
                    } else {
                        // these are errors we throw from queue() as they as rejection type errors
                        throw hre;
                    }
                } else {
                    throw re;
                }
            }
        }
        return f;
}

I checked that onError() is used somewhere in toObservable() method, so decomposeException() is invoked after this and, as you can see, there is nothing that is called after decomposeException().
So how to use HystrixPlugins to solve this problem?
I tried to use other methods of HystrixCommandExecutionHook, but, in case of error, everything is called before decomposeException().

[mattrjacobs]

@tjuchniewicz Have you seen the work in #1414? That looks like a way forward where users mark specific exceptions as not wanting wrapping. Since it is opt-in, it doesn't affect any existing code but makes this functionality available. Would love your thoughts on it

[tjuchniewicz]

@mattrjacobs Looks good for me. Thanks!
Closing this PR and moving my attention to #1414.