From c38606610c773eed1a473203cd4f77fb754713ec Mon Sep 17 00:00:00 2001 From: Sam Brannen <104798+sbrannen@users.noreply.github.com> Date: Sat, 16 Aug 2025 16:07:37 +0200 Subject: [PATCH] Supply correct exception to RetryListener.onRetryPolicyExhaustion() Prior to this commit, RetryTemplate supplied the wrong exception to RetryListener.onRetryPolicyExhaustion(). Specifically, the execute() method in RetryTemplate supplied the final, composite RetryException to onRetryPolicyExhaustion() instead of the last exception thrown by the Retryable operation. This commit fixes that bug by ensuring that the last exception thrown by the Retryable operation is supplied to onRetryPolicyExhaustion(). Closes gh-35334 --- .../core/retry/RetryTemplate.java | 2 +- .../core/retry/RetryTemplateTests.java | 81 ++++++++++++++++--- 2 files changed, 73 insertions(+), 10 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/core/retry/RetryTemplate.java b/spring-core/src/main/java/org/springframework/core/retry/RetryTemplate.java index 4ca2b33e490..ee511807017 100644 --- a/spring-core/src/main/java/org/springframework/core/retry/RetryTemplate.java +++ b/spring-core/src/main/java/org/springframework/core/retry/RetryTemplate.java @@ -174,7 +174,7 @@ public class RetryTemplate implements RetryOperations { "Retry policy for operation '%s' exhausted; aborting execution".formatted(retryableName), exceptions.removeLast()); exceptions.forEach(finalException::addSuppressed); - this.retryListener.onRetryPolicyExhaustion(this.retryPolicy, retryable, finalException); + this.retryListener.onRetryPolicyExhaustion(this.retryPolicy, retryable, retryException); throw finalException; } } 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 96f47a2d1ef..953177c4b04 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 @@ -29,13 +29,20 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments.ArgumentSet; import org.junit.jupiter.params.provider.FieldSource; +import org.mockito.InOrder; 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; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; /** - * Integration tests for {@link RetryTemplate} and {@link RetryPolicy}. + * Integration tests for {@link RetryTemplate}, {@link RetryPolicy} and + * {@link RetryListener}. * * @author Mahmoud Ben Hassine * @author Sam Brannen @@ -44,17 +51,22 @@ import static org.junit.jupiter.params.provider.Arguments.argumentSet; */ class RetryTemplateTests { - private final RetryTemplate retryTemplate = new RetryTemplate(); - - - @BeforeEach - void configureRetryTemplate() { - var retryPolicy = RetryPolicy.builder() + private final RetryPolicy retryPolicy = + RetryPolicy.builder() .maxAttempts(3) .delay(Duration.ZERO) .build(); - retryTemplate.setRetryPolicy(retryPolicy); + private final RetryTemplate retryTemplate = new RetryTemplate(retryPolicy); + + private final RetryListener retryListener = mock(); + + private final InOrder inOrder = inOrder(retryListener); + + + @BeforeEach + void configureRetryTemplate() { + retryTemplate.setRetryListener(retryListener); } @Test @@ -68,14 +80,18 @@ class RetryTemplateTests { assertThat(invocationCount).hasValue(0); assertThat(retryTemplate.execute(retryable)).isEqualTo("always succeeds"); assertThat(invocationCount).hasValue(1); + + // RetryListener interactions: + verifyNoInteractions(retryListener); } @Test void retryWithSuccessAfterInitialFailures() throws Exception { + Exception exception = new Exception("Boom!"); AtomicInteger invocationCount = new AtomicInteger(); Retryable retryable = () -> { if (invocationCount.incrementAndGet() <= 2) { - throw new Exception("Boom!"); + throw exception; } return "finally succeeded"; }; @@ -83,6 +99,13 @@ class RetryTemplateTests { assertThat(invocationCount).hasValue(0); assertThat(retryTemplate.execute(retryable)).isEqualTo("finally succeeded"); assertThat(invocationCount).hasValue(3); + + // RetryListener interactions: + inOrder.verify(retryListener).beforeRetry(retryPolicy, retryable); + inOrder.verify(retryListener).onRetryFailure(retryPolicy, retryable, exception); + inOrder.verify(retryListener).beforeRetry(retryPolicy, retryable); + inOrder.verify(retryListener).onRetrySuccess(retryPolicy, retryable, "finally succeeded"); + inOrder.verifyNoMoreInteractions(); } @Test @@ -110,6 +133,14 @@ class RetryTemplateTests { .withCause(exception); // 4 = 1 initial invocation + 3 retry attempts assertThat(invocationCount).hasValue(4); + + // RetryListener interactions: + repeat(3, () -> { + inOrder.verify(retryListener).beforeRetry(retryPolicy, retryable); + inOrder.verify(retryListener).onRetryFailure(retryPolicy, retryable, exception); + }); + inOrder.verify(retryListener).onRetryPolicyExhaustion(retryPolicy, retryable, exception); + inOrder.verifyNoMoreInteractions(); } @Test @@ -146,6 +177,14 @@ class RetryTemplateTests { .withCause(exception); // 6 = 1 initial invocation + 5 retry attempts assertThat(invocationCount).hasValue(6); + + // RetryListener interactions: + repeat(5, () -> { + inOrder.verify(retryListener).beforeRetry(retryPolicy, retryable); + inOrder.verify(retryListener).onRetryFailure(retryPolicy, retryable, exception); + }); + inOrder.verify(retryListener).onRetryPolicyExhaustion(retryPolicy, retryable, exception); + inOrder.verifyNoMoreInteractions(); } @Test @@ -188,6 +227,15 @@ class RetryTemplateTests { )); // 3 = 1 initial invocation + 2 retry attempts assertThat(invocationCount).hasValue(3); + + // RetryListener interactions: + repeat(2, () -> { + inOrder.verify(retryListener).beforeRetry(retryPolicy, retryable); + inOrder.verify(retryListener).onRetryFailure(eq(retryPolicy), eq(retryable), any(Throwable.class)); + }); + inOrder.verify(retryListener).onRetryPolicyExhaustion( + eq(retryPolicy), eq(retryable), any(IllegalStateException.class)); + inOrder.verifyNoMoreInteractions(); } static final List includesAndExcludesRetryPolicies = List.of( @@ -241,9 +289,24 @@ class RetryTemplateTests { )); // 3 = 1 initial invocation + 2 retry attempts assertThat(invocationCount).hasValue(3); + + // RetryListener interactions: + repeat(2, () -> { + inOrder.verify(retryListener).beforeRetry(retryPolicy, retryable); + inOrder.verify(retryListener).onRetryFailure(eq(retryPolicy), eq(retryable), any(Throwable.class)); + }); + inOrder.verify(retryListener).onRetryPolicyExhaustion( + eq(retryPolicy), eq(retryable), any(CustomFileNotFoundException.class)); + inOrder.verifyNoMoreInteractions(); } + private static void repeat(int times, Runnable runnable) { + for (int i = 0; i < times; i++) { + runnable.run(); + } + } + @SafeVarargs private static final Consumer hasSuppressedExceptionsSatisfyingExactly( ThrowingConsumer... requirements) {