Retry following timeout

[Stephan202]

As it stands (in version 1.0.6), any exception thrown by the AttemptTimeLimiter will cause Retryer to abort. This is includes TimeoutException. To me this was very surprising and unintuitive. Let's consider the following code:

Retryer<T> retryer = RetryerBuilder.<T> newBuilder()
  .withAttemptTimeLimiter(AttemptTimeLimiters.fixedTimeLimit(1, MINUTES))
  .withWaitStrategy(WaitStrategies.fixedWait(10, SECONDS))
  .withStopStrategy(StopStrategies.stopAfterDelay(MINUTES.toMillis(15)));

The Retryer produced here will not retry an operation if it times out after one minute. I would have expected it to wait for 10 seconds and try again, until the time limit of 15 minutes was reached.

This behavior should either be fixed or documented. I believe it's a bug, though, because the documentation for RetryerBuilder#withAttemptTimeLimiter reads:

Configures the retryer to limit the duration of any particular attempt by the given duration.

One can get the desired behavior by configuring the builder with #retryIfExceptionOfType(com.google.common.util.concurrent.UncheckedTimeoutException.class), but this is unintuitive and dependent on the implementation of AttemptTimeLimiter used.

One way to proceed would be the following:

• Add to the contract of AttemptTimeLimiter that upon timeout a java.util.concurrent.TimeoutException is thrown, whereas an ExecutionException is thrown otherwise. (The ExceptionClassPredicate implementation already seems to assume that every exception originating from the original Callable is wrapped.)
• Update the FixedAttemptTimeLimit implentation accordingly.
• Modify Retryer#call to handle TimeoutException differently from ExecutionException.

Would like to hear your thoughts.

[Stephan202]

@rholder, I'm willing to open a PR with the suggestions as outline above, but before I spend time on that I'd like to hear whether you agree with the described approach in principle.

[rholder]

I agree with your assessment and would also consider it a bug, too, since the documentation seems to indicate it would affect a single Attempt without really noting that its failure is tied to any Exception that might pop out. Without explicit handling of com.google.common.util.concurrent.UncheckedTimeoutException (or maybe more loosely any RuntimeException) the Retryer effectively stops retrying instead of giving up on the attempt and trying again. Catchall retryIfException() or retryIfRuntimeException() retrying appears to mask this behavior as well.

I would be happy to review a PR of your described approach. @dirkraft was responsible for the initial implementation of the AttemptTimeLimiter. I'd like to get his feedback on this one, too.

[dirkraft]

If I understand correctly, the attempt timeout should at a minimum behave like any other exception thrown by an attempt, and be subject to the exception-related retryer configuration.