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.

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

Implement reliable invocation order for advice within an @Aspect #25186

Closed
1 task done
sbrannen opened this issue Jun 4, 2020 · 0 comments
Closed
1 task done

Implement reliable invocation order for advice within an @Aspect #25186

sbrannen opened this issue Jun 4, 2020 · 0 comments
Assignees
@sbrannen
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Milestone
5.2.7

Comments

@sbrannen
Copy link
Member

sbrannen commented Jun 4, 2020 •
edited

Overview

The AspectJPrecedenceComparator was designed to mimic the precedence order enforced by the AspectJ compiler with regard to multiple 'after' methods defined within the same aspect whose pointcuts match the same joinpoint. Specifically, if an aspect declares multiple @After, @AfterReturning, or @AfterThrowing advice methods whose pointcuts match the same joinpoint, such 'after' advice methods should be invoked in the reverse order in which they are declared in the source code.

When the AspectJPrecedenceComparator was introduced in Spring Framework 2.0, it achieved its goal of mimicking the AspectJ compiler since the JDK at that time (i.e., Java 5) ensured that invocations of Class#geDeclaredMethods() returned an array of methods that matched the order of declaration in the source code. However, Java 7 removed this guarantee. Consequently, in Java 7 or higher, AspectJPrecedenceComparator no longer works as it is documented or as it was designed.

PR #24673 highlights a use case where AspectJPrecedenceComparator fails to assign the highest precedence to an @After advice method (declared last in the source code). Note that an @After advice method with a precedence higher than @AfterReturning and @AfterThrowing advice methods in the same aspect will effectively be invoked last due to the try-finally implementation in AspectJAfterAdvice.invoke(MethodInvocation) which invokes proceed() in the try-block and invokeAdviceMethod() in the finally-block.

Proposal

Since Spring cannot reliably determine the source code declaration order of such advice methods without using ASM to analyze the byte code, we have decided to implement reliable invocation order for 'after' advice methods declared within a single @Aspect.

Related Issues

Deliverables

  • Implement reliable invocation order for @After, @AfterReturning, and @AfterThrowing advice methods within a single @Aspect class, by ensuring that @AfterReturning and @AfterThrowing methods are always invoked before any @After methods within the same @Aspect class.
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement labels Jun 4, 2020
@sbrannen sbrannen added this to the 5.2.7 milestone Jun 4, 2020
@sbrannen sbrannen self-assigned this Jun 4, 2020
sbrannen added a commit that referenced this issue Jun 4, 2020
@sbrannen
Test status quo for 'after' advice invocation order
63e8609 
See gh-25186
sbrannen added a commit that referenced this issue Jun 4, 2020
@sbrannen
Fix @SInCE tags
a61ab5d 
See gh-25186
sbrannen added a commit that referenced this issue Jun 5, 2020
@sbrannen
Test status quo for invocation order of all advice types
fbeecf3 
Prior to this commit we did not have tests in place to verify the status
quo for the invocation order of all advice types when declared within
a single aspect, either via the <aop:aspect> XML namespace or AspectJ
auto-proxy support.

This commit introduces such tests that demonstrate where such ordering
is broken or suboptimal.

The only test for which the advice invocation order is correct or at
least as expected is the afterAdviceTypes() test method in
ReflectiveAspectJAdvisorFactoryTests, where an AOP proxy is hand crafted
using ReflectiveAspectJAdvisorFactory without the use of Spring's
AspectJPrecedenceComparator.

See gh-25186
@sbrannen sbrannen changed the title Implement reliable invocation order for 'after' advice types within an @Aspect Implement reliable invocation order for advice within an @Aspect Jun 8, 2020
@sbrannen sbrannen mentioned this issue Jun 8, 2020
Open
sbrannen added a commit that referenced this issue Jun 8, 2020
@sbrannen
Document advice precedence in reference manual
721fd20 
This commit documents the precedence for advice declared within the
same @aspect class or within the same <aop:aspect> element.

See gh-25186
@sbrannen sbrannen mentioned this issue Jun 9, 2020
Closed
FelixFly pushed a commit to FelixFly/spring-framework that referenced this issue Aug 16, 2020
@sbrannen
Test status quo for 'after' advice invocation order
9239f90 
See spring-projectsgh-25186
FelixFly pushed a commit to FelixFly/spring-framework that referenced this issue Aug 16, 2020
@sbrannen
Fix @SInCE tags
5f5b86e 
See spring-projectsgh-25186
FelixFly pushed a commit to FelixFly/spring-framework that referenced this issue Aug 16, 2020
@sbrannen
Test status quo for invocation order of all advice types
c870586 
Prior to this commit we did not have tests in place to verify the status
quo for the invocation order of all advice types when declared within
a single aspect, either via the <aop:aspect> XML namespace or AspectJ
auto-proxy support.

This commit introduces such tests that demonstrate where such ordering
is broken or suboptimal.

The only test for which the advice invocation order is correct or at
least as expected is the afterAdviceTypes() test method in
ReflectiveAspectJAdvisorFactoryTests, where an AOP proxy is hand crafted
using ReflectiveAspectJAdvisorFactory without the use of Spring's
AspectJPrecedenceComparator.

See spring-projectsgh-25186
FelixFly pushed a commit to FelixFly/spring-framework that referenced this issue Aug 16, 2020
@sbrannen
Implement reliable advice invocation order within an @aspect
80eada4 
The AspectJPrecedenceComparator was designed to mimic the precedence
order enforced by the AspectJ compiler with regard to multiple 'after'
methods defined within the same aspect whose pointcuts match the same
joinpoint. Specifically, if an aspect declares multiple @after,
@AfterReturning, or @AfterThrowing advice methods whose pointcuts match
the same joinpoint, such 'after' advice methods should be invoked in
the reverse order in which they are declared in the source code.

When the AspectJPrecedenceComparator was introduced in Spring Framework
2.0, it achieved its goal of mimicking the AspectJ compiler since the
JDK at that time (i.e., Java 5) ensured that an invocation of
Class#geDeclaredMethods() returned an array of methods that matched the
order of declaration in the source code. However, Java 7 removed this
guarantee. Consequently, in Java 7 or higher,
AspectJPrecedenceComparator no longer works as it is documented or as
it was designed when sorting advice methods in a single @aspect class.
Note, however, that AspectJPrecedenceComparator continues to work as
documented and designed when sorting advice configured via the
<aop:aspect> XML namespace element.

PR spring-projectsgh-24673 highlights a use case where AspectJPrecedenceComparator
fails to assign the highest precedence to an @after advice method
declared last in the source code. Note that an @after advice method
with a precedence higher than @AfterReturning and @AfterThrowing advice
methods in the same aspect will effectively be invoked last due to the
try-finally implementation in AspectJAfterAdvice.invoke() which invokes
proceed() in the try-block and invokeAdviceMethod() in the
finally-block.

Since Spring cannot reliably determine the source code declaration
order of annotated advice methods without using ASM to analyze the byte
code, this commit introduces reliable invocation order for advice
methods declared within a single @aspect. Specifically, the
getAdvisors(...) method in ReflectiveAspectJAdvisorFactory now hard
codes the declarationOrderInAspect to `0` instead of using the index of
the current advice method. This is necessary since the index no longer
has any correlation to the method declaration order in the source code.
The result is that all advice methods discovered via reflection will
now be sorted only according to the precedence rules defined in the
ReflectiveAspectJAdvisorFactory.METHOD_COMPARATOR. Specifically, advice
methods within a single @aspect will be sorted in the following order
(with @after advice methods effectively invoked after @AfterReturning
and @AfterThrowing advice methods): @around, @before, @after,
@AfterReturning, @AfterThrowing.

The modified assertions in AspectJAutoProxyAdviceOrderIntegrationTests
demonstrate the concrete effects of this change.

Closes spring-projectsgh-25186
FelixFly pushed a commit to FelixFly/spring-framework that referenced this issue Aug 16, 2020
@sbrannen
Document advice precedence in reference manual
3598551 
This commit documents the precedence for advice declared within the
same @aspect class or within the same <aop:aspect> element.

See spring-projectsgh-25186
@jhoeller jhoeller mentioned this issue Dec 3, 2020
Closed
@weixiaobaoL weixiaobaoL mentioned this issue Dec 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sbrannen sbrannen

Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement
Projects
None yet
Milestone
5.2.7
Development

No branches or pull requests
1 participant
@sbrannen