Ability to specify Fallback for RetryPolicy directly

[paulius-p]

When the RetryPolicy fails I need to throw a specific exception. Currently to achieve this I've configured it like this:

Failsafe
        .with(
            Fallback.<TaskResult>ofException(e -> new FailedTaskException())
                .handle(SomeRetryException.class)
                .handleResultIf(Objects::isNull)
                .handleResultIf(TaskResult::isRunning),
            new RetryPolicy<TaskResult>()
                .handle(SomeRetryException.class)
                .handleResultIf(Objects::isNull)
                .handleResultIf(TaskResult::isRunning)
                .withMaxAttempts(-1) // unlimited
                .withDelay(retryRate)
                .withMaxDuration(retryTimeout))
        .getAsync(this::someLongTask);

This leads to some duplication, as you need to repeat the same conditions for the global Fallback. It would be great to have some shorthand method to achieve this on the RetryPolicy itself. Ex.: RetryPolicy.withFallback(Fallback)

[whiskeysierra]

You could extract those and aggregate them into one parameter each:

• a List<Class<? extends Throwable>> for handle
• a single predicate for `handleResultIf

var exceptions = Lists.of(SomeRetryException.class);
// or combine them using Predicate.or(..)
var predicate = result -> result == null || result.isRunning;

Failsafe
        .with(
            Fallback.<TaskResult>ofException(e -> new FailedTaskException())
                .handle(exceptions)
                .handleResultIf(predicate),
            new RetryPolicy<TaskResult>()
                .handle(exceptions)
                .handleResultIf(predicate)
                .withMaxAttempts(-1) // unlimited
                .withDelay(retryRate)
                .withMaxDuration(retryTimeout))
        .getAsync(this::someLongTask);

[jhalterman]

Having RetryPolicy and Fallback be separate policies allows for handling any type of situation, including where you might want your RetryPolicy to handle one exception type and your Fallback to handle another. But I understand how in cases like this where the "handling" config is the same, it's annoying.

I don't think I want to couple policies to each other such as retryPolicy.withFallback, but I'm wondering if we could maybe define handling logic separate from the rest of the policies, since the handling methods are already defined in FailurePolicy which the policy impls extend. So right now we could do something like (untested):

<T extends PolicyHandler<T>> addHandlers(T policy) {
  return policy.handle(SomeRetryException.class)
     .handleResultIf(Objects::isNull)
     .handleResultIf(TaskResult::isRunning);
}

Failsafe.with(
  addHandlers(Fallback.<TaskResult>ofException(e -> new FailedTaskException())),
  addHandlers(new RetryPolicy<TaskResult>()
    .withMaxAttempts(-1) // unlimited
    .withDelay(retryRate)
    .withMaxDuration(retryTimeout)))
...

Is that decent? The other thought I have is to allow defining handlers at the FailsafeExecutor level, but that brings up a bunch of other questions such as how we should merge those handlers with existing handlers in policies, etc., etc, and would get messy.

[jessecoyle]

This has lead to very confusing behavior for us. Developers wrote the code equivalent to this

Failsafe
        .with(
            Fallback.<TaskResult>ofException(e -> new FailedTaskException()),
            new RetryPolicy<TaskResult>()
                .handle(SomeRetryException.class)
                .handleResultIf(Objects::isNull)
                .handleResultIf(TaskResult::isRunning)
                .withMaxAttempts(-1) // unlimited
                .withDelay(retryRate)
                .withMaxDuration(retryTimeout))
        .getAsync(this::someLongTask);

The assumption was that if the retries all failed, the fallback would be used. The very odd behavior is that when these policies are chained, the retry policy hands a result to the fallback policy that is success:Boolean=false, failure:Throwable=null. The policy then bases choices on failure and ignores success.

It's very surprising that the fallback gets used for any exception, but no noted negative results.

I think the RetryPolicy should have a flag to create something like MaxRetryExceededException to set as the failure in this case. I also, separately think this should be the default behavior.

Having to repeat conditions in the common case is not just extra code. It's confusing to the point of making it very easy to create bugs.

[Tembrel]

Issue #213 is another example of a user being surprised by this behavior. (In that case, there's also a surprise about how the ordering of policies affects behavior.)

[whiskeysierra]

the retry policy hands a result to the fallback policy that is success:Boolean=false, failure:Throwable=null. The policy then bases choices on failure and ignores success

That is indeed odd. As a default behavior (not specifying any failure/result predicate on the fallback policy should, imho, consider non successful results as worthy of a fallback.

I have the impression that result predicates were introduced later and that initially decisions were solely based on exceptions:

Non-result is not a failure right now:

https://github.com/jhalterman/failsafe/blob/3608292adec4a39794f9e8729b4b52748a42f39a/src/main/java/net/jodah/failsafe/PolicyExecutor.java#L109-L120

https://github.com/jhalterman/failsafe/blob/3608292adec4a39794f9e8729b4b52748a42f39a/src/main/java/net/jodah/failsafe/PolicyExecutor.java#L61-L75