Introduce `beforeRetry` callback with `RetryState` argument

[michaldo]

Hi,

there is a missing functionality in RetryListener to satisfy the following:

-> to not omit retry event, log every retry attempt with counter value, but only if retry is really planned.
If exception is not retryable, there is no need to log anything because exception will be handled by call owner.
If exception will not be retried due max retry exhausted, there is no need to log anything for the same reason.

To sum up, I don't want retry event sneak unnoticed, but I don't want overload when I know exception will be handled by call owner and no action is required

Although I'm warned few times not being polite, I'm not satisfied by implementation of #35940

General method onRetryableExecution, called even on first invocation, prior to any failure, does not align well with a problem-aware approach. I prefer fine-grained method with full context provided.

If you accept change RetryListener API, please consider add RetryState to beforeRetry() . beforeRetry() is natural handler for problem-aware and RetryState exposes failure counter
If not, please consider add information if retry is planned to RetryState and I will use it in onRetryableExecution to make no action when retry will not happen

PS. I asked https://stackoverflow.com/questions/79872397/spring-7-retrylistener but no one responded.

[jhoeller]

As you noted on Stack Overflow, in onRetryableExecution, you could check retryPolicy.shouldRetry(retryState.getLastException()) to find out whether it is worth retrying - but even then, the algorithm might still run into a timeout or a sleep interruption. If you need a deterministic indication when the current exception will not be propagated to the user, you can only find out late.

beforeRetry may meet the semantic constraints there but this callback comes late after the back-off, so you're not actually going to log the preceding exception at the time that it occurs - but rather right before the next attempt which is usually a couple of seconds later. I suppose that can be ok for journal-like log history if you accept timestamps that do not match the actual occurrence of the exception. However, it is not going to meet expectations for a live log observer.

For the same reason, we cannot expose a definitive will-be-retried state in the onRetryableExecution-provided RetryState: That callback happens right after an invocation attempt, and at that time we could just give an estimate about potential retrying (similar to your shouldRetry code) but we won't definitively know until the next back-off has passed without hitting a timeout or interruption.

If you are willing to accept that tradeoff for your specific purposes, your logging will have to happen at the time of beforeRetry indeed. We could introduce an overloaded beforeRetry method with a RetryState argument to facilitate that then, rather then trying to expose a non-definitive signal in RetryState itself.

[michaldo]

but we won't definitively know until the next back-off has passed without hitting a timeout or interruption

I treat interruption as a force majeure and not care.
Timeout: RetryTemplate calls checkIfTimeoutExceeded() after failure, not after sleep. Therefore timeout is known in advance

After discussion I lost my self confidence.
Shortcomings of beforeRetry() was no access to retry counter. But I didn't considered a delay between (past) failure and (current) retry.
More natural to me is immediate action after failure rather than delay to just before retry attempt.
Commit ad19c1a is OK, but adding information if retry is planned to RetryState is also worth to consider.

[jhoeller]

The problem with RetryState exposure of a further scheduled attempt is that it does not fit with the current algorithm and its abstracted delegates: We only know whether we are going to try once more after we asked RetryPolicy#shouldRetry as well as a timeout check and then BackOffExecution#nextBackOff - with the latter being stateful, so we can't easily check in advance.

Frankly, I rather advise to perform a custom retryPolicy.shouldRetry(retryState.getLastException()) check in onRetryableExecution and ignore all other constraints for that logging purpose, or simply to log all exceptions unconditionally for live log purposes. Or for journalling, log the last exception in beforeRetry before a new attempt begins.