From 2489cced0fba3f84d938eaf182dfe305cfea2e90 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Wed, 20 Aug 2025 23:15:40 +0200 Subject: [PATCH] Expose RetryException#getRetryCount() and accept maxAttempts(0) Closes gh-35351 Closes gh-35362 --- .../ReactiveRetryInterceptorTests.java | 20 ++++++ .../resilience/RetryInterceptorTests.java | 25 ++++++++ .../core/retry/DefaultRetryPolicy.java | 5 +- .../core/retry/RetryException.java | 11 +++- .../core/retry/RetryPolicy.java | 39 +++++------ .../retry/MaxAttemptsRetryPolicyTests.java | 13 ++++ .../core/retry/RetryPolicyTests.java | 10 +-- .../core/retry/RetryTemplateTests.java | 64 +++++++++++-------- 8 files changed, 130 insertions(+), 57 deletions(-) diff --git a/spring-context/src/test/java/org/springframework/resilience/ReactiveRetryInterceptorTests.java b/spring-context/src/test/java/org/springframework/resilience/ReactiveRetryInterceptorTests.java index ee7e8f17930..ff81f8ca107 100644 --- a/spring-context/src/test/java/org/springframework/resilience/ReactiveRetryInterceptorTests.java +++ b/spring-context/src/test/java/org/springframework/resilience/ReactiveRetryInterceptorTests.java @@ -189,6 +189,26 @@ class ReactiveRetryInterceptorTests { assertThat(target.counter.get()).isEqualTo(2); } + @Test + void adaptReactiveResultWithZeroAttempts() { + // Test minimal retry configuration: maxAttempts=1, delay=0, jitter=0, multiplier=1.0, maxDelay=0 + MinimalRetryBean target = new MinimalRetryBean(); + ProxyFactory pf = new ProxyFactory(); + pf.setTarget(target); + pf.addAdvice(new SimpleRetryInterceptor( + new MethodRetrySpec((m, t) -> true, 0, Duration.ZERO, Duration.ZERO, 1.0, Duration.ZERO))); + MinimalRetryBean proxy = (MinimalRetryBean) pf.getProxy(); + + // Should execute only 1 time, because maxAttempts=0 means initial call only + assertThatIllegalStateException() + .isThrownBy(() -> proxy.retryOperation().block()) + .satisfies(isRetryExhaustedException()) + .havingCause() + .isInstanceOf(IOException.class) + .withMessage("1"); + assertThat(target.counter.get()).isEqualTo(1); + } + @Test void adaptReactiveResultWithZeroDelayAndJitter() { // Test case where delay=0 and jitter>0 diff --git a/spring-context/src/test/java/org/springframework/resilience/RetryInterceptorTests.java b/spring-context/src/test/java/org/springframework/resilience/RetryInterceptorTests.java index ab0a4feea7e..f981bfae4ad 100644 --- a/spring-context/src/test/java/org/springframework/resilience/RetryInterceptorTests.java +++ b/spring-context/src/test/java/org/springframework/resilience/RetryInterceptorTests.java @@ -194,6 +194,31 @@ class RetryInterceptorTests { assertThat(target.counter).isEqualTo(6); } + @Test + void withPostProcessorForClassWithZeroAttempts() { + Properties props = new Properties(); + props.setProperty("delay", "10"); + props.setProperty("jitter", "5"); + props.setProperty("multiplier", "2.0"); + props.setProperty("maxDelay", "40"); + props.setProperty("limitedAttempts", "0"); + + GenericApplicationContext ctx = new GenericApplicationContext(); + ctx.getEnvironment().getPropertySources().addFirst(new PropertiesPropertySource("props", props)); + ctx.registerBeanDefinition("bean", new RootBeanDefinition(AnnotatedClassBeanWithStrings.class)); + ctx.registerBeanDefinition("bpp", new RootBeanDefinition(RetryAnnotationBeanPostProcessor.class)); + ctx.refresh(); + AnnotatedClassBeanWithStrings proxy = ctx.getBean(AnnotatedClassBeanWithStrings.class); + AnnotatedClassBeanWithStrings target = (AnnotatedClassBeanWithStrings) AopProxyUtils.getSingletonTarget(proxy); + + assertThatIOException().isThrownBy(proxy::retryOperation).withMessage("3"); + assertThat(target.counter).isEqualTo(3); + assertThatIOException().isThrownBy(proxy::otherOperation); + assertThat(target.counter).isEqualTo(4); + assertThatIOException().isThrownBy(proxy::overrideOperation); + assertThat(target.counter).isEqualTo(5); + } + @Test void withEnableAnnotation() throws Exception { AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(); diff --git a/spring-core/src/main/java/org/springframework/core/retry/DefaultRetryPolicy.java b/spring-core/src/main/java/org/springframework/core/retry/DefaultRetryPolicy.java index c9f073a96e0..796718b34e7 100644 --- a/spring-core/src/main/java/org/springframework/core/retry/DefaultRetryPolicy.java +++ b/spring-core/src/main/java/org/springframework/core/retry/DefaultRetryPolicy.java @@ -45,7 +45,6 @@ class DefaultRetryPolicy implements RetryPolicy { private final BackOff backOff; - DefaultRetryPolicy(Set> includes, Set> excludes, @Nullable Predicate predicate, BackOff backOff) { @@ -59,8 +58,8 @@ class DefaultRetryPolicy implements RetryPolicy { @Override public boolean shouldRetry(Throwable throwable) { - return this.exceptionFilter.match(throwable) && - (this.predicate == null || this.predicate.test(throwable)); + return (this.exceptionFilter.match(throwable) && + (this.predicate == null || this.predicate.test(throwable))); } @Override diff --git a/spring-core/src/main/java/org/springframework/core/retry/RetryException.java b/spring-core/src/main/java/org/springframework/core/retry/RetryException.java index 694cd04465b..eef16f68ef1 100644 --- a/spring-core/src/main/java/org/springframework/core/retry/RetryException.java +++ b/spring-core/src/main/java/org/springframework/core/retry/RetryException.java @@ -28,6 +28,7 @@ import java.util.Objects; * exceptions}. * * @author Mahmoud Ben Hassine + * @author Juergen Hoeller * @since 7.0 * @see RetryOperations */ @@ -51,8 +52,16 @@ public class RetryException extends Exception { * Get the last exception thrown by the {@link Retryable} operation. */ @Override - public final synchronized Throwable getCause() { + public final Throwable getCause() { return Objects.requireNonNull(super.getCause()); } + /** + * Return the number of retry attempts, or 0 if no retry has been attempted + * after the initial invocation at all. + */ + public int getRetryCount() { + return getSuppressed().length; + } + } diff --git a/spring-core/src/main/java/org/springframework/core/retry/RetryPolicy.java b/spring-core/src/main/java/org/springframework/core/retry/RetryPolicy.java index 16b6ee2473e..3e6bd5cf27f 100644 --- a/spring-core/src/main/java/org/springframework/core/retry/RetryPolicy.java +++ b/spring-core/src/main/java/org/springframework/core/retry/RetryPolicy.java @@ -82,12 +82,13 @@ public interface RetryPolicy { * Create a {@link RetryPolicy} configured with a maximum number of retry attempts. *

The returned policy uses a fixed backoff of {@value Builder#DEFAULT_DELAY} * milliseconds. - * @param maxAttempts the maximum number of retry attempts; must be greater than zero + * @param maxAttempts the maximum number of retry attempts; + * must be positive (or zero for no retry) * @see Builder#maxAttempts(long) * @see FixedBackOff */ static RetryPolicy withMaxAttempts(long maxAttempts) { - assertMaxAttemptsIsPositive(maxAttempts); + assertMaxAttemptsIsNotNegative(maxAttempts); return builder().backOff(new FixedBackOff(Builder.DEFAULT_DELAY, maxAttempts)).build(); } @@ -100,14 +101,9 @@ public interface RetryPolicy { } - private static void assertMaxAttemptsIsPositive(long maxAttempts) { - Assert.isTrue(maxAttempts > 0, - () -> "Invalid maxAttempts (%d): must be greater than zero.".formatted(maxAttempts)); - } - - private static void assertIsPositive(String name, Duration duration) { - Assert.isTrue((!duration.isNegative() && !duration.isZero()), - () -> "Invalid %s (%dms): must be greater than zero.".formatted(name, duration.toMillis())); + private static void assertMaxAttemptsIsNotNegative(long maxAttempts) { + Assert.isTrue(maxAttempts >= 0, + () -> "Invalid maxAttempts (%d): must be positive or zero for no retry.".formatted(maxAttempts)); } private static void assertIsNotNegative(String name, Duration duration) { @@ -115,6 +111,11 @@ public interface RetryPolicy { () -> "Invalid %s (%dms): must be greater than or equal to zero.".formatted(name, duration.toMillis())); } + private static void assertIsPositive(String name, Duration duration) { + Assert.isTrue((!duration.isNegative() && !duration.isZero()), + () -> "Invalid %s (%dms): must be greater than zero.".formatted(name, duration.toMillis())); + } + /** * Fluent API for configuring a {@link RetryPolicy} with common configuration @@ -146,13 +147,13 @@ public interface RetryPolicy { private @Nullable BackOff backOff; - private long maxAttempts; + private @Nullable Long maxAttempts; private @Nullable Duration delay; private @Nullable Duration jitter; - private double multiplier; + private @Nullable Double multiplier; private @Nullable Duration maxDelay; @@ -191,12 +192,12 @@ public interface RetryPolicy { *

The supplied value will override any previously configured value. *

You should not specify this configuration option if you have * configured a custom {@link #backOff(BackOff) BackOff} strategy. - * @param maxAttempts the maximum number of retry attempts; must be - * greater than zero + * @param maxAttempts the maximum number of retry attempts; + * must be positive (or zero for no retry) * @return this {@code Builder} instance for chained method invocations */ public Builder maxAttempts(long maxAttempts) { - assertMaxAttemptsIsPositive(maxAttempts); + assertMaxAttemptsIsNotNegative(maxAttempts); this.maxAttempts = maxAttempts; return this; } @@ -399,18 +400,18 @@ public interface RetryPolicy { public RetryPolicy build() { BackOff backOff = this.backOff; if (backOff != null) { - boolean misconfigured = (this.maxAttempts != 0) || (this.delay != null) || (this.jitter != null) || - (this.multiplier != 0) || (this.maxDelay != null); + boolean misconfigured = (this.maxAttempts != null || this.delay != null || this.jitter != null || + this.multiplier != null || this.maxDelay != null); Assert.state(!misconfigured, """ The following configuration options are not supported with a custom BackOff strategy: \ maxAttempts, delay, jitter, multiplier, or maxDelay."""); } else { ExponentialBackOff exponentialBackOff = new ExponentialBackOff(); - exponentialBackOff.setMaxAttempts(this.maxAttempts > 0 ? this.maxAttempts : DEFAULT_MAX_ATTEMPTS); + exponentialBackOff.setMaxAttempts(this.maxAttempts != null ? this.maxAttempts : DEFAULT_MAX_ATTEMPTS); exponentialBackOff.setInitialInterval(this.delay != null ? this.delay.toMillis() : DEFAULT_DELAY); exponentialBackOff.setMaxInterval(this.maxDelay != null ? this.maxDelay.toMillis() : DEFAULT_MAX_DELAY); - exponentialBackOff.setMultiplier(this.multiplier > 1 ? this.multiplier : DEFAULT_MULTIPLIER); + exponentialBackOff.setMultiplier(this.multiplier != null ? this.multiplier : DEFAULT_MULTIPLIER); if (this.jitter != null) { exponentialBackOff.setJitter(this.jitter.toMillis()); } diff --git a/spring-core/src/test/java/org/springframework/core/retry/MaxAttemptsRetryPolicyTests.java b/spring-core/src/test/java/org/springframework/core/retry/MaxAttemptsRetryPolicyTests.java index d7e559e9592..202e3849492 100644 --- a/spring-core/src/test/java/org/springframework/core/retry/MaxAttemptsRetryPolicyTests.java +++ b/spring-core/src/test/java/org/springframework/core/retry/MaxAttemptsRetryPolicyTests.java @@ -54,6 +54,18 @@ class MaxAttemptsRetryPolicyTests { assertThat(backOffExecution.nextBackOff()).isEqualTo(STOP); } + @Test + void maxAttemptsZero() { + var retryPolicy = RetryPolicy.builder().maxAttempts(0).delay(Duration.ZERO).build(); + var backOffExecution = retryPolicy.getBackOff().start(); + var throwable = mock(Throwable.class); + + assertThat(retryPolicy.shouldRetry(throwable)).isTrue(); + assertThat(backOffExecution.nextBackOff()).isEqualTo(STOP); + assertThat(retryPolicy.shouldRetry(throwable)).isTrue(); + assertThat(backOffExecution.nextBackOff()).isEqualTo(STOP); + } + @Test void maxAttemptsAndPredicate() { var retryPolicy = RetryPolicy.builder() @@ -115,6 +127,7 @@ class MaxAttemptsRetryPolicyTests { private static class CustomNumberFormatException extends NumberFormatException { } + @SuppressWarnings("serial") private static class CustomFileSystemException extends FileSystemException { diff --git a/spring-core/src/test/java/org/springframework/core/retry/RetryPolicyTests.java b/spring-core/src/test/java/org/springframework/core/retry/RetryPolicyTests.java index 8f86350fc08..fac425dbf33 100644 --- a/spring-core/src/test/java/org/springframework/core/retry/RetryPolicyTests.java +++ b/spring-core/src/test/java/org/springframework/core/retry/RetryPolicyTests.java @@ -65,12 +65,9 @@ class RetryPolicyTests { @Test void withMaxAttemptsPreconditions() { - assertThatIllegalArgumentException() - .isThrownBy(() -> RetryPolicy.withMaxAttempts(0)) - .withMessage("Invalid maxAttempts (0): must be greater than zero."); assertThatIllegalArgumentException() .isThrownBy(() -> RetryPolicy.withMaxAttempts(-1)) - .withMessage("Invalid maxAttempts (-1): must be greater than zero."); + .withMessageStartingWith("Invalid maxAttempts (-1)"); } @Test @@ -115,12 +112,9 @@ class RetryPolicyTests { @Test void maxAttemptsPreconditions() { - assertThatIllegalArgumentException() - .isThrownBy(() -> RetryPolicy.builder().maxAttempts(0)) - .withMessage("Invalid maxAttempts (0): must be greater than zero."); assertThatIllegalArgumentException() .isThrownBy(() -> RetryPolicy.builder().maxAttempts(-1)) - .withMessage("Invalid maxAttempts (-1): must be greater than zero."); + .withMessageStartingWith("Invalid maxAttempts (-1)"); } @Test diff --git a/spring-core/src/test/java/org/springframework/core/retry/RetryTemplateTests.java b/spring-core/src/test/java/org/springframework/core/retry/RetryTemplateTests.java index c3259e10871..4e3ec1c11e1 100644 --- a/spring-core/src/test/java/org/springframework/core/retry/RetryTemplateTests.java +++ b/spring-core/src/test/java/org/springframework/core/retry/RetryTemplateTests.java @@ -32,9 +32,6 @@ import org.junit.jupiter.params.provider.Arguments.ArgumentSet; import org.junit.jupiter.params.provider.FieldSource; import org.mockito.InOrder; -import org.springframework.util.backoff.BackOff; -import org.springframework.util.backoff.FixedBackOff; - import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.junit.jupiter.params.provider.Arguments.argumentSet; @@ -51,16 +48,13 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; * * @author Mahmoud Ben Hassine * @author Sam Brannen + * @author Juergen Hoeller * @since 7.0 * @see RetryPolicyTests */ class RetryTemplateTests { - private final RetryPolicy retryPolicy = - RetryPolicy.builder() - .maxAttempts(3) - .delay(Duration.ZERO) - .build(); + private final RetryPolicy retryPolicy = RetryPolicy.builder().maxAttempts(3).delay(Duration.ZERO).build(); private final RetryTemplate retryTemplate = new RetryTemplate(retryPolicy); @@ -104,7 +98,8 @@ class RetryTemplateTests { .isThrownBy(() -> retryTemplate.execute(retryable)) .withMessageMatching("Retry policy for operation '.+?' exhausted; aborting execution") .withCause(exception) - .satisfies(throwable -> assertThat(throwable.getSuppressed()).isEmpty()); + .satisfies(throwable -> assertThat(throwable.getSuppressed()).isEmpty()) + .satisfies(throwable -> assertThat(throwable.getRetryCount()).isZero()); // RetryListener interactions: inOrder.verify(retryListener).onRetryPolicyExhaustion(retryPolicy, retryable, exception); @@ -112,19 +107,8 @@ class RetryTemplateTests { } @Test - void retryWithInitialFailureAndZeroRetriesBackOffPolicy() { - RetryPolicy retryPolicy = new RetryPolicy() { - - @Override - public boolean shouldRetry(Throwable throwable) { - return true; - } - - @Override - public BackOff getBackOff() { - return new FixedBackOff(10, 0); // Zero retries - } - }; + void retryWithInitialFailureAndZeroRetriesFixedBackOffPolicy() { + RetryPolicy retryPolicy = RetryPolicy.withMaxAttempts(0); RetryTemplate retryTemplate = new RetryTemplate(retryPolicy); retryTemplate.setRetryListener(retryListener); @@ -137,7 +121,31 @@ class RetryTemplateTests { .isThrownBy(() -> retryTemplate.execute(retryable)) .withMessageMatching("Retry policy for operation '.+?' exhausted; aborting execution") .withCause(exception) - .satisfies(throwable -> assertThat(throwable.getSuppressed()).isEmpty()); + .satisfies(throwable -> assertThat(throwable.getSuppressed()).isEmpty()) + .satisfies(throwable -> assertThat(throwable.getRetryCount()).isZero()); + + // RetryListener interactions: + inOrder.verify(retryListener).onRetryPolicyExhaustion(retryPolicy, retryable, exception); + verifyNoMoreInteractions(retryListener); + } + + @Test + void retryWithInitialFailureAndZeroRetriesBackOffPolicyFromBuilder() { + RetryPolicy retryPolicy = RetryPolicy.builder().maxAttempts(0).build(); + + RetryTemplate retryTemplate = new RetryTemplate(retryPolicy); + retryTemplate.setRetryListener(retryListener); + Exception exception = new RuntimeException("Boom!"); + Retryable retryable = () -> { + throw exception; + }; + + assertThatExceptionOfType(RetryException.class) + .isThrownBy(() -> retryTemplate.execute(retryable)) + .withMessageMatching("Retry policy for operation '.+?' exhausted; aborting execution") + .withCause(exception) + .satisfies(throwable -> assertThat(throwable.getSuppressed()).isEmpty()) + .satisfies(throwable -> assertThat(throwable.getRetryCount()).isZero()); // RetryListener interactions: inOrder.verify(retryListener).onRetryPolicyExhaustion(retryPolicy, retryable, exception); @@ -282,7 +290,8 @@ class RetryTemplateTests { .satisfies(hasSuppressedExceptionsSatisfyingExactly( suppressed1 -> assertThat(suppressed1).isExactlyInstanceOf(FileNotFoundException.class), suppressed2 -> assertThat(suppressed2).isExactlyInstanceOf(IOException.class) - )); + )) + .satisfies(throwable -> assertThat(throwable.getRetryCount()).isEqualTo(2)); // 3 = 1 initial invocation + 2 retry attempts assertThat(invocationCount).hasValue(3); @@ -344,7 +353,8 @@ class RetryTemplateTests { .satisfies(hasSuppressedExceptionsSatisfyingExactly( suppressed1 -> assertThat(suppressed1).isExactlyInstanceOf(IOException.class), suppressed2 -> assertThat(suppressed2).isExactlyInstanceOf(IOException.class) - )); + )) + .satisfies(throwable -> assertThat(throwable.getRetryCount()).isEqualTo(2)); // 3 = 1 initial invocation + 2 retry attempts assertThat(invocationCount).hasValue(3); @@ -366,8 +376,9 @@ class RetryTemplateTests { } @SafeVarargs - private static final Consumer hasSuppressedExceptionsSatisfyingExactly( + private static Consumer hasSuppressedExceptionsSatisfyingExactly( ThrowingConsumer... requirements) { + return throwable -> assertThat(throwable.getSuppressed()).satisfiesExactly(requirements); } @@ -376,6 +387,7 @@ class RetryTemplateTests { private static class CustomFileNotFoundException extends FileNotFoundException { } + /** * Custom {@link RuntimeException} that implements {@link #equals(Object)} * and {@link #hashCode()} for use in assertions that check for equality.