Add "throws Exception" to HystrixCommand run() method

[gdenning]

First of all, Hystrix is an incredible library; really enjoying my initial experimentation with it, and I was able to make use of it very quickly.

I wanted to suggest changing the declaration of the HystrixCommand run method from:

protected String run();

to

protected String run() throws Exception;

Currently, calling any methods inside the run implementation which throw checked exceptions require that the exception be caught and re-thrown as an unchecked exception so that Hystrix will detect a failure.

As an example, the following throws an IOException, which generates a compile-time exception within the HystrixCommand run method unless caught and re-thrown as an unchecked exception:

        URL url = new URL(targetURL);
        HttpURLConnection connection = (HttpURLConnection) url.openConnection();

[benjchristensen]

Hi Geoff, thanks for the positive feedback and getting involved.

The thought behind not having "throws Exception" on the run method was that when wrapping a method that chose to use checked exceptions that the run() method would not negate the need of the developer to handle those exceptions whereas adding "throws Exception" affectively turns checked exceptions into runtime exceptions.

Since I believe "throws Exception" can be added to run() without breaking backwards compatibility this change could be done easily, I just want to make sure the original reason for not including it is not something we should stick to.

Would you like to submit a pull request with this change and we'll invite comments from others?

[benjchristensen]

@adriancole @opuneet @mhawthorne @benschmaus @Randgalt Do any of you have opinions on this matter that you'd like to share?

I generally am not a fan of checked exceptions and am fine with this change as it removes boilerplate try/catch/rethrow when an exception is not being dealt with in a custom way but does not prevent someone from doing so - effectively the same as RuntimeException handling.

Is there any negative to doing this? The reason I am seeking for confirmation is that this is a change that once made cannot be reverted without breaking backwards compatibility and it's somewhat opinionated as opposed to purely functional.

[mhawthorne]

I am not a fan of checked exceptions, and I also usually do not like "throws Exception", however at a glance I think this change is fine. one nice thing about this approach is that by default it treats all exceptions as failures which are followed by fallback attempts, but it also still allows a command writer to manually catch the exception and handle it in a different way (with a retry, for example). so it makes commands which exhibit the default behavior easier to write, which is a good thing.

[codefromthecrypt]

+1

I thought about this, too. Basically this aligns more with the Callable
interface, something similar codebases know how to manage. At first, I
felt dirty about throwing exception, but as the caller likely needs to
parse Throwables anyway, seems harmless.

[benjchristensen]

I got a similar response from @neerajrj over email that agreed with proceeding with this change.

Thanks @mhawthorne and @adriancole for your input.

I also agree.

@gdenning, go ahead and submit the pull request and I'll accept it.

[benjchristensen]

Thank you @gdenning for submitting the change. I will release to Maven Central soon.

[benjchristensen]

I just release 1.1.7 which should show up on Maven Central in a few hours.