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

ExposeInvocationInterceptor doesn't make a best effort to be first in execution order [SPR-12351] #16956

Closed
spring-projects-issues opened this issue Oct 20, 2014 · 4 comments
Assignees
Labels
in: core type: enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

@spring-projects-issues spring-projects-issues commented Oct 20, 2014

Juan Martin Sotuyo Dodero opened SPR-12351 and commented

ExposeInvocationInterceptor, which is automatically added to the chain when using an AspectJ aspect, should run first, but doesn't make enough efforts to do so. If this does not occur, an exception will be thrown at runtime saying not all parameters could be bound (which may be misleading to the developer, since the number of arguments being match does not coincide with those he set in the advice; and no reference to ExposeInvocationInterceptor is ever made; not even on the official documentation http://docs.spring.io/spring/docs/4.1.x/spring-framework-reference/html/aop.html#aop-ataspectj-advice-ordering )

First of all, it's implementation of @Ordered doesn't set Ordered.HIGHEST_PRECEDENCE, but Ordered.HIGHEST_PRECEDENCE + 1 (which, since a lower value is translated into higher precedence, means "second").

Also of note, is that even setting it to Ordered.HIGHEST_PRECEDENCE would not guarantee it to run first when the other aspect has Ordered.HIGHEST_PRECEDENCE. This could be attoned by using PriorityOrdered instead of Ordered in ExposeInvocationInterceptor. Doing so would guarantee the interceptor runs first to every other aspect, except maybe those that also are PriorityOrdered with Ordered.HIGHEST_PRECEDENCE.


Affects: 4.1.1

Referenced from: commits fb08644

@spring-projects-issues
Copy link
Collaborator Author

@spring-projects-issues spring-projects-issues commented Oct 20, 2014

Juergen Hoeller commented

As of 4.1.2, ExposeInvocationInterceptor declares itself as PriorityOrdered now. An option for another interceptor to override with PriorityOrdered and HIGHEST_PRECEDENCE is still left as an escape hatch; note that no regular interceptor will ever implemented PriorityOrdered, so this should be alright.

Juergen

@spring-projects-issues spring-projects-issues added type: enhancement in: core labels Jan 11, 2019
@spring-projects-issues spring-projects-issues added this to the 4.1.2 milestone Jan 11, 2019
@KaimingWan
Copy link

@KaimingWan KaimingWan commented Jan 20, 2021

This bug still exists. The fix not take affect. My SpringBoot version is 5.2.9. Question in stackoverflow describe the issue. Can we recheck the problem?

@kriegaex
Copy link

@kriegaex kriegaex commented Apr 20, 2021

I can confirm that this is still a problem, e.g. in this situation where JoinPoint::proceed is called asynchronously:

@Aspect
@Component
@Order(value = 1)
public class PollableStreamListenerAspect {
  private final ExecutorService executor = Executors.newFixedThreadPool(1);
  private volatile boolean paused = false;

  @Around("@annotation(de.scrum_master.spring.q67155048.PollableStreamListener)")
  public void receiveMessage(ProceedingJoinPoint joinPoint) throws Throwable {
    System.out.println("Receiving message");
    if (!paused) {
      // The separate thread is not busy with a previous message, so process this message:
      Runnable runnable = () -> {
        try {
          paused = true;
          joinPoint.proceed();
        }
        catch (Throwable throwable) {
          throwable.printStackTrace();
        }
        finally {
          paused = false;
        }
      };
      executor.submit(runnable);
    }
    else {
      System.out.println("Re-queue");
    }
  }
}

Without the @Order annotation the problem does not occur. With the annotation, I can only fix it like this: @Order(value = Ordered.HIGHEST_PRECEDENCE). But what if I do not want the highest precedence for this aspect?


Update: See here for a full MCVE reproducing the problem.

@kriegaex
Copy link

@kriegaex kriegaex commented Apr 22, 2021

Update to my previous comment: This workaround breaks as soon as the aspect advice binds parameters via args(myParameter), because then you immediately hit this problem:

Exception in thread "main" java.lang.IllegalStateException: Required to bind 2 arguments, but only bound 1 (JoinPointMatch was NOT bound in invocation)
	at org.springframework.aop.aspectj.AbstractAspectJAdvice.argBinding(AbstractAspectJAdvice.java:605)
	at org.springframework.aop.aspectj.AbstractAspectJAdvice.invokeAdviceMethod(AbstractAspectJAdvice.java:633)
	at org.springframework.aop.aspectj.AspectJAroundAdvice.invoke(AspectJAroundAdvice.java:70)
	at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:175)
	at org.springframework.aop.framework.CglibAopProxy$DynamicAdvisedInterceptor.intercept(CglibAopProxy.java:688)
	at de.scrum_master.spring.q67155048.MyComponent$$EnhancerBySpringCGLIB$$ecce5ea2.submitDeletion(<generated>)
	at de.scrum_master.spring.q67155048.DemoApplication.doStuff(DemoApplication.java:21)
	at de.scrum_master.spring.q67155048.DemoApplication.main(DemoApplication.java:13)

This can be avoided by using a precedence value other than HIGHEST_PRECEDENCE, but that is exactly the value needed in order to avoid the problem discussed above. That is a first class a dilemma, IMO. I know that I could just switch to AspectJ and not be bothered with this kind of Spring AOP limitations anymore, but I am trying to help someone on StackOverflow dealing with this situation in Spring AOP. This is really bad, there should be a solution for it, and I am afraid that closing this issue in the past was not it.


Update: Again, please see my updated StackOverflow answer for a complete sample application. Just copy the variant of application files and aspects you need in order to reproduce both problems and also my workaround, using manual parameter binding via JoinPoint.getArgs(). Personally, I do not like using that, but for the moment it is the only thing that works.

P.S.: The aspect implementing PriorityOrdered does not help at all. With highest precedence, again I see the error discussed in this comment, with any other precedence the error discussed in the previous one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core type: enhancement
Projects
None yet
Development

No branches or pull requests

4 participants