Introduce minimal retry functionality as a core framework feature #34716

fmbenhassine · 2025-04-04T05:10:18Z

This PR introduces a minimal core retry feature in Spring Framework. It is inspired by Spring Retry, but redesigned and trimmed to the bare minimum to cover most cases.

The main entry point is the RetryTemplate class, which is designed in a similar fashion to other templates provided by Spring. The package is minimal on purpose to keep its usage as simple as possible (see examples in RetryTemplateTests). It focuses only on stateless retries for now, but can be extended with other retry operations as well.

This PR also adapts the util.backoff package with some naming conventions for consistency with the new core retry feature.

sbrannen

Hi @fmbenhassine,

First and foremost, thanks for putting this PR together! 👍

I took a quick look, added a few comments, and asked a few questions (as food for thought).

And we will also discuss the proposal within the team.

Cheers,

Sam

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

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

sbrannen · 2025-04-04T08:47:33Z

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

+	 * Return the maximum number of retry attempts.
+	 * @return the maximum number of retry attempts
+	 */
+	int getMaxAttempts();


In the description of this PR, you state the following.

It is also focused only on stateless retries for now, but can be extended with other retry operations as well.

For future extension, do you envision extending the contract of RetryPolicy?

If so, how?

If not, how do you envision extending the feature set "with other retry operations"?

How do you envision a user possibly being able to implement a retry policy conditionally based on the actual exception thrown?

Yes, I don't exclude extending the contract of RetryPolicy with anything that we think essential for this first iteration. The list of exceptions to retry/exclude could be part of the retry policy contract.

By "can be extended with other retry operations" I was referring to stateful retries. Stateful retries require some sort of context (similar to the one currently in Spring Retry). But instead of overwhelming the RetryOperations interface with both stateless and stateful operations, I envision stateful retries to be defined in a specialization of RetryOperations (something like StatefulRetryOperations with additional methods dealing with a context). I am trying to follow the ISP here, as most users won't probably need stateful retires, and therefore should not be forced to implement those methods. Do you agree?

I pushed a change set that enriches the retry policy with a method that allows user to provide a predicate on the exception and specify the retry condition. I believe a predicate is more flexible than two lists of exceptions to include/exclude as it is currently in Spring Retry.

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

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

fmbenhassine · 2025-04-04T14:07:33Z

Thank you for the review, @sbrannen !

I added a couple of inline comments and I will update the PR accordingly.

fmbenhassine · 2025-04-07T06:38:16Z

I updated the PR with the following changes:

Add/Use a specific RetryException in the API instead of the generic Exception
Add RetryCallback interface with a logical name for diagnostics purposes
Improve RetryPolicy with the ability to specify which exceptions to retry based on a Predicate<Exception>

I find this new version to be more coherent and consistent with other templates in terms of structure and API. Looking forward to the team's thoughts on this!

This commit introduces a minimal core retry feature. It is inspired by Spring Retry, but redesigned and trimmed to the bare minimum to cover most cases. It also adapts the util backoff package with some naming conventions for consistency with the new core retry feature.

artembilan

This is great even if I have left so many questions.
Thanks

artembilan · 2025-04-07T18:02:28Z

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

+ * @since 7.0
+ * @see RetryOperations
+ */
+public class RetryException extends Exception {


Why this is not a NestedRuntimeException?
Why going for checked instead, please?
I thought we were talking about as minimal end-user impact as possible.
I mean on their code level: the unchecked exception is still going to be there anyway at runtime.

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

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

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

artembilan · 2025-05-27T17:20:00Z

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

+
+		@Override
+		public boolean shouldRetry(Throwable throwable) {
+			return this.retryAttempts++ < maxRetryAttempts;


I'm not familiar with Spring Framework code style, but I find this way as useful:

MaxRetryAttemptsPolicy.this.maxRetryAttempts

artembilan · 2025-05-27T17:23:51Z

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

+	/**
+	 * A {@link RetryExecution} based on a predicate.
+	 */
+	public static class PredicateRetryPolicyExecution implements RetryExecution {


Why static?
Why do you allow to use this instance outside of the policy itself?
Why we cannot use PredicateRetryPolicy.this.predicate property and lock-in the usage of this class?

artembilan · 2025-05-27T17:25:45Z

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

+ * stops when either of those two limits is reached.
+ *
+ * @author Stephane Nicoll
+ * @author Gary Russell


... and, I guess, your name as well 😄

artembilan · 2025-05-27T17:40:06Z

...g-core/src/test/java/org/springframework/core/retry/support/MaxRetryDurationPolicyTests.java

+		Thread.sleep(400);
+		assertThat(retryExecution.shouldRetry(Mockito.any(Exception.class))).isTrue();
+		Thread.sleep(400);
+		assertThat(retryExecution.shouldRetry(Mockito.any(Exception.class))).isFalse();


400 + 400 = 800, so less than 1000.
Any guarantee that this test is going to be stable?

I always find time-based tests as tricky and hard to model without some interference to the component under the test.
I cannot come up with good suggest at the moment, but if we are intended to have this test, let's consider to increase sleep timeout to fit into the maxRetryDuration.

I also don't think the Mockito.any() is correct choice for this kind of interaction:

public static <T> T any(Class<T> type) { reportMatcher(new InstanceOf.VarArgAware(type, "<any " + type.getCanonicalName() + ">")); return defaultValue(type); }

Looks like simple Mockito.mock() is enough.

artembilan · 2025-05-27T17:43:41Z

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

+	 * Set the maximum number of retry attempts.
+	 * @param maxRetryAttempts the maximum number of retry attempts. Must be greater than zero.
+	 */
+	public void setMaxRetryAttempts(int maxRetryAttempts) {


Why not constructor option?
I feel like new MaxRetryAttemptsPolicy() and new MaxRetryAttemptsPolicy(10) are more intuitive and cause less code and typing.

We might want to support both options.

sbrannen · 2025-05-28T11:58:37Z

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

+/**
+ * Strategy interface to define a retry execution.
+ *
+ * <p>Implementations do not need to be thread-safe.


... buy may be stateful?

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 4, 2025

fmbenhassine force-pushed the spring-retry branch from 92f257e to baf3703 Compare April 4, 2025 07:31

sbrannen changed the title ~~Introduce a minimal retry functionality as a core framework feature~~ Introduce minimal retry functionality as a core framework feature Apr 4, 2025

sbrannen added the in: core Issues in core modules (aop, beans, core, context, expression) label Apr 4, 2025

sbrannen requested changes Apr 4, 2025

View reviewed changes

fmbenhassine force-pushed the spring-retry branch from 9d2866c to 5facde5 Compare May 27, 2025 13:20

fmbenhassine force-pushed the spring-retry branch from 5facde5 to b7a7ca6 Compare May 27, 2025 13:22

fmbenhassine requested a review from sbrannen May 27, 2025 13:25

artembilan suggested changes May 27, 2025

View reviewed changes

sbrannen reviewed May 28, 2025

View reviewed changes

Introduce minimal retry functionality as a core framework feature #34716

Are you sure you want to change the base?

Introduce minimal retry functionality as a core framework feature #34716

Conversation

fmbenhassine commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbrannen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sbrannen Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fmbenhassine commented Apr 4, 2025

Uh oh!

fmbenhassine commented Apr 7, 2025

Uh oh!

artembilan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fmbenhassine commented Apr 4, 2025 •

edited

Loading

sbrannen Apr 4, 2025 •

edited

Loading