Address code review

- Make retry policy implementations private
- Remove listener subpackage
- Use LogAccessor API where appropriate
- Update nullability specification for return types
- Fix thread interruption and listener calls in RetryTemplate
- Update code style

Author: fmbenhassine

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

@@ -19,7 +19,7 @@
 /**
  * Strategy interface to define a retry execution.
  *
- * <p>Implementations do not need to be thread-safe.
+ * <p>Implementations may be stateful but do not need to be thread-safe.
  *
  * @author Mahmoud Ben Hassine
  * @since 7.0

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

@@ -16,7 +16,7 @@
 
 package org.springframework.core.retry;
 
-import org.springframework.core.retry.support.listener.CompositeRetryListener;
+import org.springframework.core.retry.support.CompositeRetryListener;
 
 /**
  * An extension point that allows to inject code during key retry phases.

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

@@ -39,7 +39,7 @@ public interface RetryOperations {
 	 * @throws RetryException thrown if the retry policy is exhausted. All attempt exceptions
 	 * should be added as suppressed exceptions to the final exception.
 	 */
-	<R> @Nullable R execute(RetryCallback<R> retryCallback) throws RetryException;
+	<R extends @Nullable Object> R execute(RetryCallback<R> retryCallback) throws RetryException;
 
 }
 

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

@@ -19,12 +19,12 @@
 import java.util.ArrayList;
 import java.util.List;
 
-import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.jspecify.annotations.Nullable;
 
+import org.springframework.core.log.LogAccessor;
+import org.springframework.core.retry.support.CompositeRetryListener;
 import org.springframework.core.retry.support.MaxRetryAttemptsPolicy;
-import org.springframework.core.retry.support.listener.CompositeRetryListener;
 import org.springframework.util.Assert;
 import org.springframework.util.backoff.BackOffExecution;
 import org.springframework.util.backoff.BackOffPolicy;
@@ -51,13 +51,13 @@
  */
 public class RetryTemplate implements RetryOperations {
 
-	protected final Log logger = LogFactory.getLog(getClass());
+	protected final LogAccessor logger = new LogAccessor(LogFactory.getLog(getClass()));
 
-	private RetryPolicy retryPolicy = new MaxRetryAttemptsPolicy();
+	protected RetryPolicy retryPolicy = new MaxRetryAttemptsPolicy();
 
-	private BackOffPolicy backOffPolicy = new FixedBackOffPolicy(1000);
+	protected BackOffPolicy backOffPolicy = new FixedBackOffPolicy(1000);
 
-	private RetryListener retryListener = new RetryListener() {
+	protected RetryListener retryListener = new RetryListener() {
 	};
 
 	/**
@@ -125,53 +125,42 @@ public void setRetryListener(RetryListener retryListener) {
 	 * are added as suppressed exceptions to the final exception.
 	 */
 	@Override
-	public <R> @Nullable R execute(RetryCallback<R> retryCallback) throws RetryException {
+	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
 		try {
-			if (logger.isDebugEnabled()) {
-				logger.debug("About to execute callback '" + callbackName + "'");
-			}
+			logger.debug("About to execute callback '" + callbackName + "'");
 			R result = retryCallback.run();
-			if (logger.isDebugEnabled()) {
-				logger.debug("Callback '" + callbackName + "' executed successfully");
-			}
+			logger.debug("Callback '" + callbackName + "' executed successfully");
 			return result;
 		}
 		catch (Throwable initialException) {
-			if (logger.isDebugEnabled()) {
-				logger.debug("Execution of callback '" + callbackName + "' failed, initiating the retry process", initialException);
-			}
+			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)) {
-				if (logger.isDebugEnabled()) {
-					logger.debug("About to retry callback '" + callbackName + "'");
-				}
+				logger.debug("About to retry callback '" + callbackName + "'");
 				try {
 					this.retryListener.beforeRetry(retryExecution);
 					R result = retryCallback.run();
 					this.retryListener.onRetrySuccess(retryExecution, result);
-					if (logger.isDebugEnabled()) {
-						logger.debug("Callback '" + callbackName + "' retried successfully");
-					}
+					logger.debug("Callback '" + callbackName + "' retried successfully");
 					return result;
 				}
 				catch (Throwable currentAttemptException) {
 					this.retryListener.onRetryFailure(retryExecution, currentAttemptException);
 					try {
 						long duration = backOffExecution.nextBackOff();
-						if (logger.isDebugEnabled()) {
-							logger.debug("Retry callback '" + callbackName + "' failed for " + currentAttemptException.getMessage() + ", backing off for " + duration + "ms");
-						}
+						logger.debug("Retry callback '" + callbackName + "' failed for " + 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);
 					}
 					suppressedExceptions.add(currentAttemptException);
@@ -181,6 +170,7 @@ public void setRetryListener(RetryListener retryListener) {
 			// 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);
 			suppressedExceptions.forEach(finalException::addSuppressed);
+			this.retryListener.onRetryPolicyExhaustion(retryExecution, finalException);
 			throw finalException;
 		}
 	}

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

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-package org.springframework.core.retry.support.listener;
+package org.springframework.core.retry.support;
 
 import java.util.LinkedList;
 import java.util.List;

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

@@ -28,14 +28,33 @@
  */
 public class MaxRetryAttemptsPolicy implements RetryPolicy {
 
-	private int maxRetryAttempts = 3;
+	/**
+	 * The default maximum number of retry attempts.
+	 */
+	public static final int DEFAULT_MAX_RETRY_ATTEMPTS = 3;
+
+	private int maxRetryAttempts = DEFAULT_MAX_RETRY_ATTEMPTS;
+
+	/**
+	 * Create a new {@link MaxRetryAttemptsPolicy} with the default maximum number of retry attempts.
+	 */
+	public MaxRetryAttemptsPolicy() {
+	}
+
+	/**
+	 * Create a new {@link MaxRetryAttemptsPolicy} with the specified maximum number of retry attempts.
+	 * @param maxRetryAttempts the maximum number of retry attempts. Must be greater than zero.
+	 */
+	public MaxRetryAttemptsPolicy(int maxRetryAttempts) {
+		setMaxRetryAttempts(maxRetryAttempts);
+	}
 
 	/**
 	 * Start a new retry execution.
 	 * @return a fresh {@link MaxRetryAttemptsPolicyExecution} ready to be used
 	 */
 	@Override
-	public MaxRetryAttemptsPolicyExecution start() {
+	public RetryExecution start() {
 		return new MaxRetryAttemptsPolicyExecution();
 	}
 
@@ -51,17 +70,17 @@ public void setMaxRetryAttempts(int maxRetryAttempts) {
 	/**
 	 * A {@link RetryExecution} based on a maximum number of retry attempts.
 	 */
-	public class MaxRetryAttemptsPolicyExecution implements RetryExecution {
+	private class MaxRetryAttemptsPolicyExecution implements RetryExecution {
 
-		protected int retryAttempts;
+		private int retryAttempts;
 
-		public MaxRetryAttemptsPolicyExecution() {
+		MaxRetryAttemptsPolicyExecution() {
 			this.retryAttempts = 0;
 		}
 
 		@Override
 		public boolean shouldRetry(Throwable throwable) {
-			return this.retryAttempts++ < maxRetryAttempts;
+			return this.retryAttempts++ < MaxRetryAttemptsPolicy.this.maxRetryAttempts;
 		}
 
 	}

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

@@ -31,14 +31,33 @@
  */
 public class MaxRetryDurationPolicy implements RetryPolicy {
 
-	private Duration maxRetryDuration = Duration.ofSeconds(3); // what would be a reasonable here?
+	/**
+	 * The default maximum retry duration.
+	 */
+	public static final Duration DEFAULT_MAX_RETRY_DURATION = Duration.ofSeconds(3);
+
+	private Duration maxRetryDuration = DEFAULT_MAX_RETRY_DURATION;
+
+	/**
+	 * Create a new {@link MaxRetryDurationPolicy} with the default maximum retry duration.
+	 */
+	public MaxRetryDurationPolicy() {
+	}
+
+	/**
+	 * Create a new {@link MaxRetryDurationPolicy} with the specified maximum retry duration.
+	 * @param maxRetryDuration the maximum retry duration. Must be positive.
+	 */
+	public MaxRetryDurationPolicy(Duration maxRetryDuration) {
+		setMaxRetryDuration(maxRetryDuration);
+	}
 
 	/**
 	 * Start a new retry execution.
 	 * @return a fresh {@link MaxRetryDurationPolicyExecution} ready to be used
 	 */
 	@Override
-	public MaxRetryDurationPolicyExecution start() {
+	public RetryExecution start() {
 		return new MaxRetryDurationPolicyExecution();
 	}
 
@@ -55,14 +74,14 @@ public void setMaxRetryDuration(Duration maxRetryDuration) {
 	/**
 	 * A {@link RetryExecution} based on a maximum retry duration.
 	 */
-	public class MaxRetryDurationPolicyExecution implements RetryExecution {
+	private class MaxRetryDurationPolicyExecution implements RetryExecution {
 
-		protected LocalDateTime retryStartTime = LocalDateTime.now();
+		private final LocalDateTime retryStartTime = LocalDateTime.now();
 
 		@Override
 		public boolean shouldRetry(Throwable throwable) {
 			Duration currentRetryDuration = Duration.between(this.retryStartTime, LocalDateTime.now());
-			return currentRetryDuration.compareTo(maxRetryDuration) <= 0;
+			return currentRetryDuration.compareTo(MaxRetryDurationPolicy.this.maxRetryDuration) <= 0;
 		}
 
 	}

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

@@ -41,28 +41,10 @@ public PredicateRetryPolicy(Predicate<Throwable> predicate) {
 
 	/**
 	 * Start a new retry execution.
-	 * @return a fresh {@link PredicateRetryPolicyExecution} ready to be used
+	 * @return a fresh ready to be used
 	 */
-	public PredicateRetryPolicyExecution start() {
-		return new PredicateRetryPolicyExecution(this.predicate);
-	}
-
-	/**
-	 * A {@link RetryExecution} based on a predicate.
-	 */
-	public static class PredicateRetryPolicyExecution implements RetryExecution {
-
-		protected final Predicate<Throwable> predicate;
-
-		public PredicateRetryPolicyExecution(Predicate<Throwable> predicate) {
-			this.predicate = predicate;
-		}
-
-		@Override
-		public boolean shouldRetry(Throwable throwable) {
-			return this.predicate.test(throwable);
-		}
-
+	public RetryExecution start() {
+		return this.predicate::test;
 	}
 
 }

spring-core/src/main/java/org/springframework/core/retry/support/listener/package-info.java

@@ -55,6 +55,7 @@
  *
  * @author Stephane Nicoll
  * @author Gary Russell
+ * @author Mahmoud Ben Hassine
  * @since 7.0
  */
 public class ExponentialBackOffPolicy implements BackOffPolicy {

spring-core/src/main/java/org/springframework/util/backoff/ExponentialBackOffPolicy.java

@@ -114,11 +114,12 @@ public String getName() {
 		};
 		MaxRetryAttemptsPolicy retryPolicy = new MaxRetryAttemptsPolicy() {
 			@Override
-			public MaxRetryAttemptsPolicyExecution start() {
-				return new MaxRetryAttemptsPolicyExecution() {
+			public RetryExecution start() {
+				return new RetryExecution() {
+					int retryAttempts;
 					@Override
 					public boolean shouldRetry(Throwable throwable) {
-						return super.shouldRetry(throwable) && throwable instanceof TechnicalException;
+						return this.retryAttempts++ < 3 && throwable instanceof TechnicalException;
 					}
 				};
 			}

spring-core/src/test/java/org/springframework/core/retry/RetryTemplateTests.java

@@ -14,16 +14,16 @@
  * limitations under the License.
  */
 
-package org.springframework.core.retry.support.listener;
+package org.springframework.core.retry.support;
 
 import java.util.Arrays;
 
 import org.junit.jupiter.api.Test;
-import org.mockito.Mockito;
 
 import org.springframework.core.retry.RetryExecution;
 import org.springframework.core.retry.RetryListener;
 
+import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
 
 /**
@@ -33,14 +33,14 @@
  */
 class ComposedRetryListenerTests {
 
-	private final RetryListener listener1 = Mockito.mock(RetryListener.class);
-	private final RetryListener listener2 = Mockito.mock(RetryListener.class);
+	private final RetryListener listener1 = mock();
+	private final RetryListener listener2 = mock();
 
 	private final CompositeRetryListener composedRetryListener = new CompositeRetryListener(Arrays.asList(listener1, listener2));
 
 	@Test
 	void beforeRetry() {
-		RetryExecution retryExecution = Mockito.mock(RetryExecution.class);
+		RetryExecution retryExecution = mock();
 		this.composedRetryListener.beforeRetry(retryExecution);
 
 		verify(this.listener1).beforeRetry(retryExecution);
@@ -50,7 +50,7 @@ void beforeRetry() {
 	@Test
 	void onSuccess() {
 		Object result = new Object();
-		RetryExecution retryExecution = Mockito.mock(RetryExecution.class);
+		RetryExecution retryExecution = mock();
 		this.composedRetryListener.onRetrySuccess(retryExecution, result);
 
 		verify(this.listener1).onRetrySuccess(retryExecution, result);
@@ -60,7 +60,7 @@ void onSuccess() {
 	@Test
 	void onFailure() {
 		Exception exception = new Exception();
-		RetryExecution retryExecution = Mockito.mock(RetryExecution.class);
+		RetryExecution retryExecution = mock();
 		this.composedRetryListener.onRetryFailure(retryExecution, exception);
 
 		verify(this.listener1).onRetryFailure(retryExecution, exception);
@@ -70,7 +70,7 @@ void onFailure() {
 	@Test
 	void onMaxAttempts() {
 		Exception exception = new Exception();
-		RetryExecution retryExecution = Mockito.mock(RetryExecution.class);
+		RetryExecution retryExecution = mock();
 		this.composedRetryListener.onRetryPolicyExhaustion(retryExecution, exception);
 
 		verify(this.listener1).onRetryPolicyExhaustion(retryExecution, exception);