Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Introduce order for @EnableRetry #333

Merged
merged 2 commits into from
Feb 24, 2023
Merged

Introduce order for @EnableRetry #333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4d98f0
Introduce order for @EnableRetry
quaff Feb 21, 2023
4feadc1
Remove unnecessary cast
quaff Feb 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/main/java/org/springframework/retry/annotation/EnableRetry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2012-2022 the original author or authors.
* Copyright 2012-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -24,6 +24,7 @@

import org.springframework.context.annotation.EnableAspectJAutoProxy;
import org.springframework.context.annotation.Import;
import org.springframework.core.Ordered;
import org.springframework.core.annotation.AliasFor;

/**
Expand All @@ -33,7 +34,8 @@
* the annotations.
*
* @author Dave Syer
* @since 2.0
* @author Yanming Zhou
* @since 1.1
*
*/
@Target(ElementType.TYPE)
Expand All @@ -51,4 +53,13 @@
@AliasFor(annotation = EnableAspectJAutoProxy.class)
boolean proxyTargetClass() default false;

/**
* Indicate the order in which the {@link RetryConfiguration} should be applied.
* <p>
* The default is {@link Ordered#LOWEST_PRECEDENCE} in order to run after all other
* post-processors, so that it can add an advisor to existing proxies rather than
* double-proxy.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there is an error in this description. The reasoning is misleading. The RetryConfiguration advice is not applied by any post-processors AFAIK. It is just registered as a bean to be applied later by bean-factory when it creates a proxy for a bean. And it's always applied just once, so there is no danger of creating double-proxy.

@artembilan Any chance this description could be corrected? To be honest I don't know the real reason of this property to be Ordered.LOWEST_PRECEDENCE except the backward compatibility. This default is actually danger as it interferes with @Transactional annotation that is also Ordered.LOWEST_PRECEDENCE, so the final order is undefined. But 99% of the time you want the order to be @Retry -> @Transactional and not vice versa because many exceptions will be thrown only at the commit phase and could be retried only by starting a new transaction.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! We can fix that description and also make a default order as a Ordered.LOWEST_PRECEDENCE - 1 to make it working with @Transactional as you have just explained which is really makes sense as a default behavior.
I think that description was taken from the @EnableAsync which looks like does its logic slightly different than an AspectJAwareAdvisorAutoProxyCreator initiated by the @EnableAspectJAutoProxy from this @EnableRetry.

Feel free to contribute the fix!
Thank you!

*/
int order() default Ordered.LOWEST_PRECEDENCE;

}
23 changes: 19 additions & 4 deletions src/main/java/org/springframework/retry/annotation/RetryConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2006-2022 the original author or authors.
* Copyright 2006-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -41,9 +41,13 @@
import org.springframework.beans.factory.ListableBeanFactory;
import org.springframework.beans.factory.SmartInitializingSingleton;
import org.springframework.beans.factory.config.BeanDefinition;
import org.springframework.context.annotation.ImportAware;
import org.springframework.context.annotation.Role;
import org.springframework.core.OrderComparator;
import org.springframework.core.annotation.AnnotationAttributes;
import org.springframework.core.annotation.AnnotationUtils;
import org.springframework.core.type.AnnotationMetadata;
import org.springframework.lang.Nullable;
import org.springframework.retry.RetryListener;
import org.springframework.retry.backoff.Sleeper;
import org.springframework.retry.interceptor.MethodArgumentsKeyGenerator;
Expand All @@ -63,14 +67,18 @@
* @author Artem Bilan
* @author Markus Heiden
* @author Gary Russell
* @author Yanming Zhou
* @since 1.1
*
*/
@SuppressWarnings("serial")
@Role(BeanDefinition.ROLE_INFRASTRUCTURE)
@Component
public class RetryConfiguration extends AbstractPointcutAdvisor
implements IntroductionAdvisor, BeanFactoryAware, InitializingBean, SmartInitializingSingleton {
implements IntroductionAdvisor, BeanFactoryAware, InitializingBean, SmartInitializingSingleton, ImportAware {

@Nullable
protected AnnotationAttributes enableRetry;

private AnnotationAwareRetryOperationsInterceptor advice;

Expand All @@ -88,6 +96,12 @@ public class RetryConfiguration extends AbstractPointcutAdvisor

private BeanFactory beanFactory;

@Override
public void setImportMetadata(AnnotationMetadata importMetadata) {
this.enableRetry = AnnotationAttributes
.fromMap(importMetadata.getAnnotationAttributes(EnableRetry.class.getName()));
}

@Override
public void afterPropertiesSet() throws Exception {
this.retryContextCache = findBean(RetryContextCache.class);
Expand All @@ -98,8 +112,9 @@ public void afterPropertiesSet() throws Exception {
retryableAnnotationTypes.add(Retryable.class);
this.pointcut = buildPointcut(retryableAnnotationTypes);
this.advice = buildAdvice();
if (this.advice instanceof BeanFactoryAware) {
((BeanFactoryAware) this.advice).setBeanFactory(this.beanFactory);
this.advice.setBeanFactory(this.beanFactory);
if (this.enableRetry != null) {
setOrder(enableRetry.getNumber("order"));
}
}

Expand Down
23 changes: 22 additions & 1 deletion src/test/java/org/springframework/retry/annotation/EnableRetryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2006-2022 the original author or authors.
* Copyright 2006-2023 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -57,6 +57,7 @@
* @author Gary Russell
* @author Aldo Sinanaj
* @author Henning Pöttker
* @author Yanming Zhou
* @since 1.1
*/
public class EnableRetryTests {
Expand Down Expand Up @@ -105,6 +106,15 @@ public void proxyTargetClass() {
context.close();
}

@Test
public void order() {
AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(
TestOrderConfiguration.class);
RetryConfiguration config = context.getBean(RetryConfiguration.class);
assertThat(config.getOrder()).isEqualTo(1);
context.close();
}

@Test
public void marker() {
AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext(TestConfiguration.class);
Expand Down Expand Up @@ -348,6 +358,17 @@ public int getOrder() {

}

@Configuration
@EnableRetry(order = 1)
protected static class TestOrderConfiguration {

@Bean
public Service service() {
return new Service();
}

}

@Configuration
@EnableRetry
protected static class TestConfiguration {
Expand Down