When using the @AfterReturning and @After to implement AOP function, the program always call the method annotated by @After before the method annotated by @AfterReturning. It's opposite to the normal execution order of AOP function implemented by the XML.
By tracing and debugging, I found the reason is that the annotated methods is sorted by a wrong comparator which was not constructed correctly in the process of parsing annotations. Finally, the problem was fixed by adjusting the order of parameters (annotation classes) when the comparator be constructed. It was be verified after repair and compilation. So, I created this commit . If I misunderstand something, please tell me. thanks !
Comment From: sbrannen
It's opposite to the normal execution order of AOP function implemented by the XML.
Can you please explain that in more detail?
Does the current behavior contradict existing documentation (either in Spring or AspectJ)?
In other words, what is the rationale for the proposed change?
Comment From: dafengsu7
It's opposite to the normal execution order of AOP function implemented by the XML.
Can you please explain that in more detail?
Does the current behavior contradict existing documentation (either in Spring or AspectJ)?
In other words, what is the rationale for the proposed change?
1. What is the problem?
I found this problem when using annotation AOP to implement transaction management, so to illustrate this problem, I wrote a demo of simulating transaction processing to show the problem, the complete demo can be found at:https://github.com/Dafengsu/DemoForAnnotionAOP. following is the source code of the demo:
The spring config
bean.xml
<?xml version="1.0" encoding="UTF-8"?>
<beans xmlns="http://www.springframework.org/schema/beans"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns:aop="http://www.springframework.org/schema/aop"
xmlns:context="http://www.springframework.org/schema/context"
xsi:schemaLocation="http://www.springframework.org/schema/beans
http://www.springframework.org/schema/beans/spring-beans.xsd
http://www.springframework.org/schema/aop
http://www.springframework.org/schema/aop/spring-aop.xsd
http://www.springframework.org/schema/context
http://www.springframework.org/schema/context/spring-context.xsd">
<!--package scan-->
<context:component-scan base-package="com.dafengsu"/>
<!-- TransactionManager-->
<bean id="XMLTransactionManager" class="com.dafengsu.aop.XMLTransactionManager"/>
<!--aop-->
<aop:config>
<aop:pointcut id="daoTransfer" expression="execution(* com.dafengsu.service.impl.*.*(..))"/>
<aop:aspect id="txAdvice" ref="XMLTransactionManager">
<aop:before method="beginTransaction" pointcut-ref="daoTransfer"/>
<aop:after-returning method="commit" pointcut-ref="daoTransfer"/>
<aop:after-throwing method="rollback" pointcut-ref="daoTransfer"/>
<aop:after method="release" pointcut-ref="daoTransfer"/>
</aop:aspect>
</aop:config>
<aop:aspectj-autoproxy/>
</beans>
The service Code
TransactionService.java
package com.dafengsu.service;
public interface TransactionService {
void transaction();
}
TransactionServiceImpl.java
package com.dafengsu.service.impl;
import com.dafengsu.service.TransactionService;
import org.springframework.stereotype.Service;
@Service("transactionService")
public class TransactionServiceImpl implements TransactionService {
@Override
public void transaction() {
System.out.println("....Execute transaction....");
}
}
Implemented the AOP function by the XML
XMLTransactionManager.java
package com.dafengsu.aop;
public class XMLTransactionManager {
// Indicates whether the connection is closed
private boolean isClosed = false;
public void beginTransaction() {
System.out.println("XML: beginTransaction");
}
public void commit() {
if (isClosed) {
throw new RuntimeException("XML: Connection is closed");
}
System.out.println("XML: commit...");
}
public void rollback() {
System.out.println("XML: rollback...");
}
public void release() {
isClosed = true;
System.out.println("XML: Connection released.");
}
}
Implemented the AOP function by the annotion
AnnotationTransactionManager.java
package com.dafengsu.aop;
import org.aspectj.lang.annotation.*;
import org.springframework.stereotype.Component;
@Component("annotationTransactionManager")
@Aspect
public class AnnotationTransactionManager {
//Indicates whether the connection is closed
private boolean isClosed = false;
@Pointcut("execution(* com.dafengsu.service.impl.*.*(..))")
private void daoTransfer() {
}
@Before("daoTransfer()")
public void beginTransaction() {
System.err.println("Annotation: beginTransaction");
}
@AfterReturning("daoTransfer()")
public void commit() {
if (isClosed) {
throw new RuntimeException("Annotation: Connection is closed");
}
System.err.println("Annotation: commit...");
}
@AfterThrowing("daoTransfer()")
public void rollback() {
System.err.println("Annotation: rollback...");
}
@After("daoTransfer()")
public void release() {
isClosed = true;
System.err.println("Annotation: Connection released.");
}
}
JUnit Jupiter Test
TransactionServiceTest.java
package com.dafengsu.service;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.test.context.ContextConfiguration;
import org.springframework.test.context.junit.jupiter.SpringExtension;
@ExtendWith(SpringExtension.class)
@ContextConfiguration(locations = "classpath:bean.xml")
class TransactionServiceTest {
@Autowired
private TransactionService transactionService;
@Test
void transaction() {
transactionService.transaction();
}
}
If all goes well, the method invocation order of the two AOP implementations should be the same and no exception will be thrown.
This is the result of the running before the modification
XML: beginTransaction
Annotation: beginTransaction
Annotation: Connection released.
....Execute transaction....
Annotation: rollback...
XML: rollback...
XML: Connection released.
java.lang.RuntimeException: Annotation: Connection is closed
at com.dafengsu.aop.AnnotationTransactionManager.commit
It can be seen that the order of AOP method invocations implemented by the annotations is inconsistent with the XML implementation, and an exception is thrown.
The order of the invocation we want is:
beginTransaction()transaction()commit()release()
But the calling sequence of AOP implementation is:
beginTransaction()transaction()release()commit()
Because the AOP implementation calls release() before commit(), when commit() is called, isClosed was true (set by release() method) indicating that the data connection has been closed, then an exception is thrown. And the transaction cannot be completed in a real scenario.
When I made this commited modification, recompiled and replaced the spring-aop jar package. This is the result of re-running:
XML: beginTransaction
Annotation: beginTransaction
....Execute transaction....
Annotation: commit...
Annotation: Connection released.
XML: commit...
XML: Connection released.
Everything is normal and the two implementations are consistent.
2. Does the current behavior contradict existing documentation (either in Spring or AspectJ) ？
NO!
Fragment of ReflectiveAspectJAdvisorFactory.java
static {
Comparator<Method> adviceKindComparator = new ConvertingComparator<>(
new InstanceComparator<>(
Around.class, Before.class, AfterReturning.class, AfterThrowing.class, After.class),
(Converter<Method, Annotation>) method -> {
AspectJAnnotation<?> annotation =
AbstractAspectJAdvisorFactory.findAspectJAnnotationOnMethod(method);
return (annotation != null ? annotation.getAnnotation() : null);
});
Comparator<Method> methodNameComparator = new ConvertingComparator<>(Method::getName);
METHOD_COMPARATOR = adviceKindComparator.thenComparing(methodNameComparator);
}
private List<Method> getAdvisorMethods(Class<?> aspectClass) {
final List<Method> methods = new ArrayList<>();
ReflectionUtils.doWithMethods(aspectClass, method -> {
// Exclude pointcuts
if (AnnotationUtils.getAnnotation(method, Pointcut.class) == null) {
methods.add(method);
}
}, ReflectionUtils.USER_DECLARED_METHODS);
if (methods.size() > 1) {
methods.sort(METHOD_COMPARATOR);
}
return methods;
}
Through the IDEA Findusages function, we can find that the statically constructed adviceKindComparator is only used in one place (assigned to METHOD_COMPARATOR), and METHOD_COMPARATOR is also only called in one place (methods.sort (METHOD_COMPARATOR)). Therefore, only the method ordering function at getAdvisorMethods(Class<?> aspectClass) in the process of parsing annotations is affected by this modification. It will not conflict with any other code and no need to rewrite the document.
3. what is the rationale for the proposed change?
AOP's annotation implementation should perform the correct call sequence and be consistent with the XML implementation. Although we can use @Around or XML to circumvent this flaw, it is best to fix it for the completeness of the AOP annotation implementation.
Comment From: sbrannen
Thanks so much for the very detailed response!
AOP's annotation implementation should perform the correct call sequence and be consistent with the XML implementation. Although we can use
@Aroundor XML to circumvent this flaw, it is best to fix it for the completeness of the AOP annotation implementation.
That basically sums it up. 👍
Comment From: xtttttttttx
This behavior contradicts existing documentation.
After Returning Advice
After returning advice runs when a matched method execution returns normally. You can declare it by using the @AfterReturning annotation.
After (Finally) Advice
After (finally) advice runs when a matched method execution exits. It is declared by using the @After annotation. After advice must be prepared to handle both normal and exception return conditions. It is typically used for releasing resources and similar purposes.
Comment From: sbrannen
Thanks again for the PR and for bringing this inconsistency to our attention!
After thorough analysis I determined that the cause of the undesired behavior was not in the METHOD_COMPARATOR in ReflectiveAspectJAdvisorFactory but rather in the implementation of the getAdvisors() method in ReflectiveAspectJAdvisorFactory. Specifically, passing advisors.size() as the declarationOrderInAspect to getAdvisor() is inappropriate for advice discovered via reflection in an @Aspect class on Java 7 or higher.
The proposed change in this PR actually breaks the following test which is inherited by ReflectiveAspectJAdvisorFactoryTests.
https://github.com/spring-projects/spring-framework/blob/33181daeaced56a6dfdd36c4e57f7b00135c65ff/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java#L511-L525
Further information can be found in #25186 and 0998bd49ef1c2d45a1f52202854098577d8f7a66, and a follow-up issue has been raised for advice configured via the aop XML namespace (see #25205).
Please note that the change in #25186 has already been released (today) in Spring Framework 5.2.7.
In addition, the following excerpt from the Advice Ordering section of the reference manual provides guidance with regard to the ordering of multiple advice methods within a single @Aspect class.
As of Spring Framework 5.2.7, advice methods defined in the same
@Aspectclass that need to run at the same join point are assigned precedence based on their advice type in the following order, from highest to lowest precedence:@Around,@Before,@After,@AfterReturning,@AfterThrowing. Note, however, that due to the implementation style in Spring’sAspectJAfterAdvice, an@Afteradvice method will effectively be invoked after any@AfterReturningor@AfterThrowingadvice methods in the same aspect.When two pieces of the same type of advice (for example, two
@Afteradvice methods) defined in the same@Aspectclass both need to run at the same join point, the ordering is undefined (since there is no way to retrieve the source code declaration order through reflection for javac-compiled classes). Consider collapsing such advice methods into one advice method per join point in each@Aspectclass or refactor the pieces of advice into separate@Aspectclasses that you can order at the aspect level viaOrderedor@Order.
I am closing this PR since it has been effectively superseded by #25186.