+1

When designing this thing, I had two options for dealing with exceptions in the get/run methods:

1. Wrap only checked exceptions in FailsafeException and re-throw
2. Wrap all exceptions in FailsafeException and re-throw

I chose option 2. Why? Mostly for consistency. I felt it was more straightforward to know that any failure from a runnable/callable will be wrapped in FailsafeException than to sometimes wrap an exception, depending on whether it is checked. FWIW: the consistency of always wrapping things is similar to what the JDK's executor framework does with things like Future.get() where all callable/runnable failures are wrapped in ExecutionException.

It's somewhat of a tough call with either option. That said: I'm open to hearing arguments that option 1 would be better :)

The current behavior (wrap everything) is particularly annoying when one wants to access the details of an exception and often results in patterns like:

try {
    String result = Failsafe.with(retryPolicy).get(() -> {
        throw new AbcRuntimeException("Exception that should not lead to a retry");
    });
} catch (FailsafeException e) {
    if (e.getCause instanceof AbcRuntimeException) {
        AbcRuntimeException abcE = (AbcRuntimeException) e.getCause;
        doSomething(abcE.getDef(), abcE.getXyz());
    } else {
        throw e;
    }
}

with the proposed change this could be simplified to

try {
    String result = Failsafe.with(retryPolicy).get(() -> {
        throw new AbcRuntimeException("Exception that should not lead to a retry");
    });
} catch (AbcRuntimeException e) {
    doSomething(abcE.getDef(), abcE.getXyz());
}

which is much nicer to both read and write!

@axelfontaine Good point. As an API author yourself, do you think it's confusing to say RuntimeExceptions are wrapped in FailsafeException and checked exceptions are not?

Thanks to @axelfontaine for actually making my point properly. My main goal was to be able to distinguish between the exceptions I want to retry on from other exceptions.

@jhalterman I understand the consistency argument - personally I find checked exceptions quite difficult to deal with and avoid them, so I ignored them, but I realise that sometimes you have no choice (e.g. when dealing with libraries).

An alternative solution that would allow you to wrap exceptions would be to have two kind of FailsafeExceptions:

1. FailsafeInvocationTargetException
2. FailsafeRetriesFailedException

This would have the added benefit that the latter one could have the number of unsuccessful retries and potentially even a list of the errors (which is interesting if there are different errors that can occur).

@jhalterman Not at all. Simply document clearly that runtime exception flow straight through and legacy checked exceptions are wrapped.

The other nice thing about that behavior is that it makes Failsafe much less intruding. Have a method call that could fail and may need retrying? Wrap that one line with Failsafe. Done.

After all you already return the return value unwrapped. Why not do the same for (runtime) exceptions?

To elaborate on my earlier +1, as @axelfontaine said this would make Failsafe much less intrusive which is a key point. When using a library you don't want it to introduce extra boilerplates like the FailsafeException.getCause() instanceof ...

My main goal was to be able to distinguish between the exceptions I want to retry on from other exceptions.

@fleipold This is a use case I've been careful to support since early on, when retries are exceeded versus when a non-retryable failure occurs. The approach I created for handling this is via listeners. Communicating different events/scenarios using event listeners felt more appropriate than encoding varied event info in return values or exceptions. See the onRetriesExceeded method in particular. Let me know if that stuff doesn't support your use case and we can talk further.

The other nice thing about that behavior is that it makes Failsafe much less intruding. Have a method call that could fail and may need retrying? Wrap that one line with Failsafe. Done.

@axelfontaine This argument hits home big time since it's completely in line with how I want Failsafe to be used - simple and lightweight. I'm sold.

I'll make this change and have it in the next release :)

1.0.0 has been released with this change (should be in central shortly).