Revise core retry support

This commit constitutes a first pass over the new core retry support.

- Fix code and Javadoc formatting

- Polish/fix Javadoc

- Fix default FixedBackOff configuration in RetryTemplate

- Consistent logging in RetryTemplate

- Fix listener handling in CompositeRetryListener, allowing addListener()
  to work

- Polish tests

- Ensure RetryTemplateTests do not take over 30 seconds to execute

See gh-34716

Author: sbrannen

spring-core/src/main/java/org/springframework/core/retry/RetryCallback.java

@@ -17,7 +17,9 @@
 package org.springframework.core.retry;
 
 /**
- * Callback interface for a retryable piece of code. Used in conjunction with {@link RetryOperations}.
+ * Callback interface for a retryable block of code.
+ *
+ * <p>Used in conjunction with {@link RetryOperations}.
  *
  * @author Mahmoud Ben Hassine
  * @since 7.0
@@ -35,11 +37,13 @@ public interface RetryCallback<R> {
 	R run() throws Throwable;
 
 	/**
-	 * A unique logical name for this callback to distinguish retries around
-	 * business operations.
-	 * @return the name of the callback. Defaults to the class name.
+	 * A unique, logical name for this callback, used to distinguish retries for
+	 * different business operations.
+	 * <p>Defaults to the fully-qualified class name.
+	 * @return the name of the callback
 	 */
 	default String getName() {
 		return getClass().getName();
 	}
+
 }

spring-core/src/main/java/org/springframework/core/retry/RetryException.java

@@ -19,7 +19,7 @@
 import java.io.Serial;
 
 /**
- * Exception class for exhausted retries.
+ * Exception thrown when a {@link RetryPolicy} has been exhausted.
  *
  * @author Mahmoud Ben Hassine
  * @since 7.0
@@ -30,18 +30,19 @@ public class RetryException extends Exception {
 	@Serial
 	private static final long serialVersionUID = 5439915454935047936L;
 
+
 	/**
-	 * Create a new exception with a message.
-	 * @param message the exception's message
+	 * Create a new {@code RetryException} for the supplied message.
+	 * @param message the detail message
 	 */
 	public RetryException(String message) {
 		super(message);
 	}
 
 	/**
-	 * Create a new exception with a message and a cause.
-	 * @param message the exception's message
-	 * @param cause the exception's cause
+	 * Create a new {@code RetryException} for the supplied message and cause.
+	 * @param message the detail message
+	 * @param cause the root cause
 	 */
 	public RetryException(String message, Throwable cause) {
 		super(message, cause);

spring-core/src/main/java/org/springframework/core/retry/RetryExecution.java

@@ -17,7 +17,8 @@
 package org.springframework.core.retry;
 
 /**
- * Strategy interface to define a retry execution.
+ * Strategy interface to define a retry execution created for a given
+ * {@link RetryPolicy}.
  *
  * <p>Implementations may be stateful but do not need to be thread-safe.
  *

spring-core/src/main/java/org/springframework/core/retry/RetryListener.java

@@ -48,15 +48,15 @@ default void onRetrySuccess(RetryExecution retryExecution, Object result) {
 	/**
 	 * Called every time a retry attempt fails.
 	 * @param retryExecution the retry execution
-	 * @param throwable the throwable thrown by the callback
+	 * @param throwable the exception thrown by the callback
 	 */
 	default void onRetryFailure(RetryExecution retryExecution, Throwable throwable) {
 	}
 
 	/**
-	 * Called once the retry policy is exhausted.
+	 * Called if the {@link RetryPolicy} is exhausted.
 	 * @param retryExecution the retry execution
-	 * @param throwable the last throwable thrown by the callback
+	 * @param throwable the last exception thrown by the {@link RetryCallback}
 	 */
 	default void onRetryPolicyExhaustion(RetryExecution retryExecution, Throwable throwable) {
 	}

spring-core/src/main/java/org/springframework/core/retry/RetryOperations.java

@@ -19,7 +19,7 @@
 import org.jspecify.annotations.Nullable;
 
 /**
- * Main entry point to the core retry functionality. Defines a set of retryable operations.
+ * Interface specifying basic retry operations.
  *
  * <p>Implemented by {@link RetryTemplate}. Not often used directly, but a useful
  * option to enhance testability, as it can easily be mocked or stubbed.
@@ -31,15 +31,16 @@
 public interface RetryOperations {
 
 	/**
-	 * Retry the given callback (according to the retry policy configured at the implementation level)
-	 * until it succeeds or eventually throw an exception if the retry policy is exhausted.
+	 * Execute the given callback (according to the {@link RetryPolicy} configured
+	 * at the implementation level) until it succeeds, or eventually throw an
+	 * exception if the {@code RetryPolicy} is exhausted.
 	 * @param retryCallback the callback to call initially and retry if needed
-	 * @param <R> the type of the callback's result
-	 * @return the callback's result
-	 * @throws RetryException thrown if the retry policy is exhausted. All attempt exceptions
-	 * should be added as suppressed exceptions to the final exception.
+	 * @param <R> the type of the result
+	 * @return the result of the callback, if any
+	 * @throws RetryException if the {@code RetryPolicy} is exhausted; exceptions
+	 * encountered during retry attempts should be made available as suppressed
+	 * exceptions
 	 */
 	<R extends @Nullable Object> R execute(RetryCallback<R> retryCallback) throws RetryException;
 
 }
-

spring-core/src/main/java/org/springframework/core/retry/RetryPolicy.java

@@ -25,8 +25,8 @@
 public interface RetryPolicy {
 
 	/**
-	 * Start a new retry execution.
-	 * @return a fresh {@link RetryExecution} ready to be used
+	 * Start a new execution for this retry policy.
+	 * @return a new {@link RetryExecution}
 	 */
 	RetryExecution start();
 

spring-core/src/main/java/org/springframework/core/retry/RetryTemplate.java

@@ -31,52 +31,60 @@
 import org.springframework.util.backoff.FixedBackOff;
 
 /**
- * A basic implementation of {@link RetryOperations} that uses a
- * {@link RetryPolicy} and a {@link BackOff} to retry a
- * {@link RetryCallback}. By default, the callback will be called
- * 3 times with a fixed backoff of 1 second.
+ * A basic implementation of {@link RetryOperations} that invokes and potentially
+ * retries a {@link RetryCallback} based on a configured {@link RetryPolicy} and
+ * {@link BackOff} policy.
  *
- * <p>It is also possible to register a {@link RetryListener} to intercept and inject code
- * during key retry phases (before a retry attempt, after a retry attempt, etc.).
+ * <p>By default, a callback will be invoked at most 3 times with a fixed backoff
+ * of 1 second.
+ *
+ * <p>A {@link RetryListener} can be {@linkplain #setRetryListener(RetryListener)
+ * registered} to intercept and inject behavior during key retry phases (before a
+ * retry attempt, after a retry attempt, etc.).
  *
  * <p>All retry operations performed by this class are logged at debug level,
- * using "org.springframework.core.retry.RetryTemplate" as log category.
+ * using {@code "org.springframework.core.retry.RetryTemplate"} as the log category.
  *
  * @author Mahmoud Ben Hassine
+ * @author Sam Brannen
  * @since 7.0
  * @see RetryOperations
  * @see RetryPolicy
  * @see BackOff
  * @see RetryListener
+ * @see RetryCallback
  */
 public class RetryTemplate implements RetryOperations {
 
 	protected final LogAccessor logger = new LogAccessor(LogFactory.getLog(getClass()));
 
 	protected RetryPolicy retryPolicy = new MaxRetryAttemptsPolicy();
 
-	protected BackOff backOffPolicy = new FixedBackOff();
+	protected BackOff backOffPolicy = new FixedBackOff(1000, Long.MAX_VALUE);
 
 	protected RetryListener retryListener = new RetryListener() {
 	};
 
 	/**
-	 * Create a new retry template with default settings.
+	 * Create a new {@code RetryTemplate} with maximum 3 retry attempts and a
+	 * fixed backoff of 1 second.
 	 */
 	public RetryTemplate() {
 	}
 
 	/**
-	 * Create a new retry template with a custom {@link RetryPolicy}.
+	 * Create a new {@code RetryTemplate} with a custom {@link RetryPolicy} and a
+	 * fixed backoff of 1 second.
 	 * @param retryPolicy the retry policy to use
 	 */
 	public RetryTemplate(RetryPolicy retryPolicy) {
-		Assert.notNull(retryPolicy, "Retry policy must not be null");
+		Assert.notNull(retryPolicy, "RetryPolicy must not be null");
 		this.retryPolicy = retryPolicy;
 	}
 
 	/**
-	 * Create a new retry template with a custom {@link RetryPolicy} and {@link BackOff}.
+	 * Create a new {@code RetryTemplate} with a custom {@link RetryPolicy} and
+	 * {@link BackOff} policy.
 	 * @param retryPolicy the retry policy to use
 	 * @param backOffPolicy the backoff policy to use
 	 */
@@ -87,88 +95,98 @@ public RetryTemplate(RetryPolicy retryPolicy, BackOff backOffPolicy) {
 	}
 
 	/**
-	 * Set the {@link RetryPolicy} to use. Defaults to <code>MaxAttemptsRetryPolicy()</code>.
-	 * @param retryPolicy the retry policy to use. Must not be <code>null</code>.
+	 * Set the {@link RetryPolicy} to use.
+	 * <p>Defaults to {@code new MaxRetryAttemptsPolicy()}.
+	 * @param retryPolicy the retry policy to use
+	 * @see MaxRetryAttemptsPolicy
 	 */
 	public void setRetryPolicy(RetryPolicy retryPolicy) {
 		Assert.notNull(retryPolicy, "Retry policy must not be null");
 		this.retryPolicy = retryPolicy;
 	}
 
 	/**
-	 * Set the {@link BackOff} to use. Defaults to <code>FixedBackOffPolicy(Duration.ofSeconds(1))</code>.
-	 * @param backOffPolicy the backoff policy to use. Must not be <code>null</code>.
+	 * Set the {@link BackOff} policy to use.
+	 * <p>Defaults to {@code new FixedBackOff(1000, Long.MAX_VALUE))}.
+	 * @param backOffPolicy the backoff policy to use
+	 * @see FixedBackOff
 	 */
 	public void setBackOffPolicy(BackOff backOffPolicy) {
 		Assert.notNull(backOffPolicy, "BackOff policy must not be null");
 		this.backOffPolicy = backOffPolicy;
 	}
 
 	/**
-	 * Set the {@link RetryListener} to use. Defaults to a <code>NoOp</code> implementation.
-	 * If multiple listeners are needed, use a {@link CompositeRetryListener}.
-	 * @param retryListener the retry listener to use. Must not be <code>null</code>.
+	 * Set the {@link RetryListener} to use.
+	 * <p>If multiple listeners are needed, use a {@link CompositeRetryListener}.
+	 * <p>Defaults to a <em>no-op</em> implementation.
+	 * @param retryListener the retry listener to use
 	 */
 	public void setRetryListener(RetryListener retryListener) {
 		Assert.notNull(retryListener, "Retry listener must not be null");
 		this.retryListener = retryListener;
 	}
 
 	/**
-	 * Call the retry callback according to the configured retry and backoff policies.
-	 * If the callback succeeds, its result is returned. Otherwise, a {@link RetryException}
-	 * will be thrown to the caller having all attempt exceptions as suppressed exceptions.
+	 * Execute the supplied {@link RetryCallback} according to the configured
+	 * retry and backoff policies.
+	 * <p>If the callback succeeds, its result will be returned. Otherwise, a
+	 * {@link RetryException} will be thrown to the caller.
 	 * @param retryCallback the callback to call initially and retry if needed
 	 * @param <R> the type of the result
-	 * @return the result of the callback if any
-	 * @throws RetryException thrown if the retry policy is exhausted. All attempt exceptions
-	 * are added as suppressed exceptions to the final exception.
+	 * @return the result of the callback, if any
+	 * @throws RetryException if the {@code RetryPolicy} is exhausted; exceptions
+	 * encountered during retry attempts are available as suppressed exceptions
 	 */
 	@Override
 	public <R extends @Nullable Object> R execute(RetryCallback<R> retryCallback) throws RetryException {
-		Assert.notNull(retryCallback, "Retry Callback must not be null");
 		String callbackName = retryCallback.getName();
-		// initial attempt
+		// Initial attempt
 		try {
-			logger.debug(() -> "About to execute callback '" + callbackName + "'");
+			logger.debug(() -> "Preparing to execute callback '" + callbackName + "'");
 			R result = retryCallback.run();
-			logger.debug(() -> "Callback '" + callbackName + "' executed successfully");
+			logger.debug(() -> "Callback '" + callbackName + "' completed successfully");
 			return result;
 		}
 		catch (Throwable initialException) {
-			logger.debug(initialException, () -> "Execution of callback '" + callbackName + "' failed, initiating the retry process");
-			// retry process starts here
+			logger.debug(initialException,
+					() -> "Execution of callback '" + callbackName + "' failed; initiating the retry process");
+			// Retry process starts here
 			RetryExecution retryExecution = this.retryPolicy.start();
 			BackOffExecution backOffExecution = this.backOffPolicy.start();
 			List<Throwable> suppressedExceptions = new ArrayList<>();
 
 			Throwable retryException = initialException;
 			while (retryExecution.shouldRetry(retryException)) {
-				logger.debug(() -> "About to retry callback '" + callbackName + "'");
+				logger.debug(() -> "Preparing to retry callback '" + callbackName + "'");
 				try {
 					this.retryListener.beforeRetry(retryExecution);
 					R result = retryCallback.run();
 					this.retryListener.onRetrySuccess(retryExecution, result);
-					logger.debug(() -> "Callback '" + callbackName + "' retried successfully");
+					logger.debug(() -> "Callback '" + callbackName + "' completed successfully after retry");
 					return result;
 				}
 				catch (Throwable currentAttemptException) {
 					this.retryListener.onRetryFailure(retryExecution, currentAttemptException);
 					try {
 						long duration = backOffExecution.nextBackOff();
-						logger.debug(() -> "Retry callback '" + callbackName + "' failed for " + currentAttemptException.getMessage() + ", backing off for " + duration + "ms");
+						logger.debug(() -> "Retry callback '" + callbackName + "' failed due to '" +
+								currentAttemptException.getMessage() + "'; backing off for " + duration + "ms");
 						Thread.sleep(duration);
 					}
 					catch (InterruptedException interruptedException) {
 						Thread.currentThread().interrupt();
-						throw new RetryException("Unable to backoff for retry callback '" + callbackName + "'", interruptedException);
+						throw new RetryException("Unable to back off for retry callback '" + callbackName + "'",
+								interruptedException);
 					}
 					suppressedExceptions.add(currentAttemptException);
 					retryException = currentAttemptException;
 				}
 			}
-			// retry policy exhausted at this point, throwing a RetryException with the initial exception as cause and remaining attempts exceptions as suppressed
-			RetryException finalException = new RetryException("Retry policy for callback '" + callbackName + "' exhausted, aborting execution", initialException);
+			// The RetryPolicy has exhausted at this point, so we throw a RetryException with the
+			// initial exception as the cause and remaining exceptions as suppressed exceptions.
+			RetryException finalException = new RetryException("Retry policy for callback '" + callbackName +
+					"' exhausted; aborting execution", initialException);
 			suppressedExceptions.forEach(finalException::addSuppressed);
 			this.retryListener.onRetryPolicyExhaustion(retryExecution, finalException);
 			throw finalException;

spring-core/src/main/java/org/springframework/core/retry/support/CompositeRetryListener.java

@@ -25,8 +25,9 @@
 import org.springframework.util.Assert;
 
 /**
- * A composite implementation of the {@link RetryListener} interface. This class
- * is used to compose multiple listeners within a {@link RetryTemplate}.
+ * A composite implementation of the {@link RetryListener} interface.
+ *
+ * <p>This class is used to compose multiple listeners within a {@link RetryTemplate}.
  *
  * <p>Delegate listeners will be called in their registration order.
  *
@@ -35,30 +36,30 @@
  */
 public class CompositeRetryListener implements RetryListener {
 
-	private final List<RetryListener> listeners;
+	private final List<RetryListener> listeners = new LinkedList<>();
+
 
 	/**
-	 * Create a new {@link CompositeRetryListener}.
+	 * Create a new {@code CompositeRetryListener}.
 	 */
 	public CompositeRetryListener() {
-		this.listeners = new LinkedList<>();
 	}
 
 	/**
-	 * Create a new {@link CompositeRetryListener} with a list of delegates.
-	 * @param listeners the delegate listeners to register. Must not be empty.
+	 * Create a new {@code CompositeRetryListener} with the supplied list of
+	 * delegates.
+	 * @param listeners the list of delegate listeners to register; must not be empty
 	 */
 	public CompositeRetryListener(List<RetryListener> listeners) {
 		Assert.notEmpty(listeners, "RetryListener List must not be empty");
-		this.listeners = List.copyOf(listeners);
+		this.listeners.addAll(listeners);
 	}
 
 	/**
 	 * Add a new listener to the list of delegates.
-	 * @param listener the listener to add. Must not be <code>null</code>.
+	 * @param listener the listener to add
 	 */
 	public void addListener(RetryListener listener) {
-		Assert.notNull(listener, "Retry listener must not be null");
 		this.listeners.add(listener);
 	}