Fix execution order of @AfterReturning and @After methods in ReflectiveAspectJAdvisorFactory

[dafengsu7]

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 !

[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?

[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.

[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 @Around or XML to circumvent this flaw, it is best to fix it for the completeness of the AOP annotation implementation.

That basically sums it up. 👍

[xietx1995]

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.

[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.

spring-framework/spring-aop/src/test/java/org/springframework/aop/aspectj/annotation/AbstractAspectJAdvisorFactoryTests.java

Lines 511 to 525 in 33181da

	@Test
	void afterAdviceTypes() throws Exception {
	InvocationTrackingAspect aspect = new InvocationTrackingAspect();
	List<Advisor> advisors = getFixture().getAdvisors(
	new SingletonMetadataAwareAspectInstanceFactory(aspect, "exceptionHandlingAspect"));
	Echo echo = (Echo) createProxy(new Echo(), advisors, Echo.class);
	assertThat(aspect.invocations).isEmpty();
	assertThat(echo.echo(42)).isEqualTo(42);
	assertThat(aspect.invocations).containsExactly("around - start", "before", "after returning", "after", "around - end");
	aspect.invocations.clear();
	assertThatExceptionOfType(FileNotFoundException.class).isThrownBy(() -> echo.echo(new FileNotFoundException()));
	assertThat(aspect.invocations).containsExactly("around - start", "before", "after throwing", "after", "around - end");
	}

Further information can be found in #25186 and 0998bd4, 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 @Aspect class 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’s AspectJAfterAdvice, an @After advice method will effectively be invoked after any @AfterReturning or @AfterThrowing advice methods in the same aspect.

When two pieces of the same type of advice (for example, two @After advice methods) defined in the same @Aspect class 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 @Aspect class or refactor the pieces of advice into separate @Aspect classes that you can order at the aspect level via Ordered or @Order.

I am closing this PR since it has been effectively superseded by #25186.