AspectJ Handling of Multiple Matching Advices

Question

I am using AspectJ in Java to log the calls to some methods. I've looked online but couldn't manage to find an answer to this:

What happens when two @Around advices match for a method?

Specifically, I am using two @Around advices, like this:

@Around("condition1() && condition2() && condition3()")
public Object around(ProceedingJoinPoint point) {
    return around(point, null);
}

@Around("condition1() && condition2() && condition3() && args(request)")
public Object around(ProceedingJoinPoint point, Object request) {
    ...
    result = (Result) point.proceed();
    ...
}

Will this result in point.proceed() being called twice (having the actual method called twice) if both of these advices match?

In general, unless AspectJ can reason symbolically that one condition subsumes the other, it can't eliminate the need for both. If it *could* do such reasoning, you could argue both ways for what it should do. (I don't happen to know). I assume that since have these two advices, that you actually already know the answer by virtue of running it. Why don't you tell us? — Ira Baxter, Feb 24 '16 at 10:48

kriegaex · Accepted Answer · 2016-02-28T18:09:59.300

Your approach is highly problematic because you manually call one advice from another one. This is not how AOP should be applied. Please let AspectJ decide which advices to execute based on their respective pointcuts. The way you delegate from one advice to another you could even call an advice which would not match by itself. Example in plain AspectJ without Spring (works the same in Spring AOP, though):

Java driver application:

package de.scrum_master.app;

public class Application {
    private static void doSomething() {
        System.out.println("Doing something");
    }

    public static void main(String[] args) {
        doSomething();
    }
}

Aspect:

package de.scrum_master.aspect;

import org.aspectj.lang.ProceedingJoinPoint;
import org.aspectj.lang.annotation.Around;
import org.aspectj.lang.annotation.Aspect;

@Aspect
public class MyBogusAspect {
    @Around("execution(* doSomething(..))")
    public Object matchingAdvice(ProceedingJoinPoint thisJoinPoint) {
        System.out.println("matching advice called on joinpoint " + thisJoinPoint);
        return nonMatchingAdvice(thisJoinPoint);
    }

    @Around("execution(* doSomethingElse(..))")
    public Object nonMatchingAdvice(ProceedingJoinPoint thisJoinPoint) {
        System.out.println("non-matching advice called on joinpoint " + thisJoinPoint);
        return thisJoinPoint.proceed();
    }
}

Console log:

matching advice called on joinpoint execution(void de.scrum_master.app.Application.doSomething())
non-matching advice called on joinpoint execution(void de.scrum_master.app.Application.doSomething())
Doing something

Can you see how unhealthy your approach is? An advice which otherwise would not match is called by a matching one. This yields some really unexpected behaviour IMO. Please don't do it!!!

Now as for your original question about multiple matching advice, this is how you should do it:

Modified aspect:

package de.scrum_master.aspect;

import org.aspectj.lang.ProceedingJoinPoint;
import org.aspectj.lang.annotation.Around;
import org.aspectj.lang.annotation.Aspect;

@Aspect
public class MyBetterAspect {
    @Around("execution(* doSomething(..))")
    public Object matchingAdvice(ProceedingJoinPoint thisJoinPoint) {
        System.out.println(">>> matching advice on " + thisJoinPoint);
        Object result = thisJoinPoint.proceed();
        System.out.println("<<< matching advice on " + thisJoinPoint);
        return result;
    }

    @Around("execution(* doSomething(..))")
    public Object anotherMatchingAdvice(ProceedingJoinPoint thisJoinPoint) {
        System.out.println(">>> another matching advice on " + thisJoinPoint);
        Object result = thisJoinPoint.proceed();
        System.out.println("<<< another matching advice on " + thisJoinPoint);
        return result;
    }
}

New console log:

>>> matching advice on execution(void de.scrum_master.app.Application.doSomething())
>>> another matching advice on execution(void de.scrum_master.app.Application.doSomething())
Doing something
<<< another matching advice on execution(void de.scrum_master.app.Application.doSomething())
<<< matching advice on execution(void de.scrum_master.app.Application.doSomething())

As you can see, AspectJ or Spring AOP wrap multiple matching advice like onion skins around joinpoints and only the innermost proceed() calls the actual joinpoint while the outer layers call the inner ones, making sure that each joinpoint is executed only once. There is no need for you trying to be smarter than the AOP framework, possibly causing damage (see my first example).

One more thing: If multiple aspects have matching pointcuts, you can influence their order of execution via @DeclarePrecedence in AspectJ, but within a single aspect you have no influence on the execution order or at least you should not rely on it. In Spring AOP you can use the @Order annotation in order to determine aspect precedence, but the order is also undefined for multiple advice from the same aspect, see also the Spring manual.

Update 2016-02-28, 18:30 CET, after some discussion in comments:

Okay, we extend the driver class a little bit so we can test some more:

package de.scrum_master.app;

public class Application {
    private static void doSomething() {
        System.out.println("Doing something");
    }

    private static String doSomethingElse(String text) {
        System.out.println("Doing something else");
        return text;
    }

    private static int doAnotherThing(int i, int j, int k) {
        System.out.println("Doing another thing");
        return (i + j) * k;
    }

    public static void main(String[] args) {
        doSomething();
        doSomethingElse("foo");
        doAnotherThing(11, 22, 33);
    }
}

Now, binding the first parameter in AspectJ is as easy as args(request, ..) which works for one or more parameters. The only exception is zero parameters, in which case the pointcut would not fire. So either I end up with something similar to what you did:

package de.scrum_master.aspect;

import org.aspectj.lang.ProceedingJoinPoint;
import org.aspectj.lang.annotation.Around;
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.Pointcut;

@Aspect
public class BoundFirstParameterAspect {
    @Pointcut("execution(* do*(..))")
    public static void myPointcut() {}

    @Around("myPointcut()")
    public Object matchingAdvice(ProceedingJoinPoint thisJoinPoint) {
        return anotherMatchingAdvice(thisJoinPoint, null);
    }

    @Around("myPointcut() && args(request, ..)")
    public Object anotherMatchingAdvice(ProceedingJoinPoint thisJoinPoint, Object request) {
        System.out.println(">>> another matching advice on " + thisJoinPoint);
        Object result = thisJoinPoint.proceed();
        System.out.println("<<< another matching advice on " + thisJoinPoint);
        return result;
    }
}

Which is makes the same advice fire twice and thus causes an overhead, even though the original method is only called once, but you can see the overhead in the log:

>>> another matching advice on execution(void de.scrum_master.app.Application.doSomething())
Doing something
<<< another matching advice on execution(void de.scrum_master.app.Application.doSomething())
>>> another matching advice on execution(String de.scrum_master.app.Application.doSomethingElse(String))
>>> another matching advice on execution(String de.scrum_master.app.Application.doSomethingElse(String))
Doing something else
<<< another matching advice on execution(String de.scrum_master.app.Application.doSomethingElse(String))
<<< another matching advice on execution(String de.scrum_master.app.Application.doSomethingElse(String))
>>> another matching advice on execution(int de.scrum_master.app.Application.doAnotherThing(int, int, int))
>>> another matching advice on execution(int de.scrum_master.app.Application.doAnotherThing(int, int, int))
Doing another thing
<<< another matching advice on execution(int de.scrum_master.app.Application.doAnotherThing(int, int, int))
<<< another matching advice on execution(int de.scrum_master.app.Application.doAnotherThing(int, int, int))

You can easily recognise how double advices are fired for each joinpoint.

Alternatively, you can bind the parameter during runtime, which is not very elegant and incurs a little runtime penalty, but works perfectly well:

package de.scrum_master.aspect;

import org.aspectj.lang.ProceedingJoinPoint;
import org.aspectj.lang.annotation.Around;
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.Pointcut;

@Aspect
public class BoundFirstParameterAspect {
    @Pointcut("execution(* do*(..))")
    public static void myPointcut() {}

    @Around("myPointcut()")
    public Object matchingAdvice(ProceedingJoinPoint thisJoinPoint) {
        System.out.println(">>> matching advice on " + thisJoinPoint);
        Object[] args = thisJoinPoint.getArgs();
        Object request =  args.length > 0 ? args[0] : null;
        System.out.println("First parameter = " + request);
        Object result = thisJoinPoint.proceed();
        System.out.println("<<< matching advice on " + thisJoinPoint);
        return result;
    }
}

This avoids double advice execution as well as code duplication and yields the following console output:

>>> matching advice on execution(void de.scrum_master.app.Application.doSomething())
First parameter = null
Doing something
<<< matching advice on execution(void de.scrum_master.app.Application.doSomething())
>>> matching advice on execution(String de.scrum_master.app.Application.doSomethingElse(String))
First parameter = foo
Doing something else
<<< matching advice on execution(String de.scrum_master.app.Application.doSomethingElse(String))
>>> matching advice on execution(int de.scrum_master.app.Application.doAnotherThing(int, int, int))
First parameter = 11
Doing another thing
<<< matching advice on execution(int de.scrum_master.app.Application.doAnotherThing(int, int, int))

Last, but not least, you can have two slightly different pointcuts - one with empty args() and one with args(request, ..) - both of which can delegate parameter handling, logging and exception handling to a helper method in order to avoid duplication, as I said in one of my comments:

package de.scrum_master.aspect;

import org.aspectj.lang.ProceedingJoinPoint;
import org.aspectj.lang.annotation.Around;
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.Pointcut;

@Aspect
public class BoundFirstParameterAspect {
    @Pointcut("execution(* do*(..))")
    public static void myPointcut() {}

    @Around("myPointcut() && args()")
    public Object myAdvice(ProceedingJoinPoint thisJoinPoint) {
        return myAdviceHelper(thisJoinPoint, null);
    }

    @Around("myPointcut() && args(request, ..)")
    public Object myAdviceWithParams(ProceedingJoinPoint thisJoinPoint, Object request) {
        return myAdviceHelper(thisJoinPoint, request);
    }

    private Object myAdviceHelper(ProceedingJoinPoint thisJoinPoint, Object request) {
        System.out.println(">>> matching advice on " + thisJoinPoint);
        System.out.println("First parameter = " + request);
        Object result = thisJoinPoint.proceed();
        System.out.println("<<< matching advice on " + thisJoinPoint);
        return result;
    }
}

The console log should be exactly the same as the previous one.

Update 2:

Well, I just realised that the empty args() trick would also apply to your original idea and avoid double execution as well as the helper method:

package de.scrum_master.aspect;

import org.aspectj.lang.ProceedingJoinPoint;
import org.aspectj.lang.annotation.Around;
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.Pointcut;

@Aspect
public class BoundFirstParameterAspect {
    @Pointcut("execution(* do*(..))")
    public static void myPointcut() {}

    @Around("myPointcut() && args()")
    public Object myAdvice(ProceedingJoinPoint thisJoinPoint) {
        return myAdviceWithParams(thisJoinPoint, null);
    }

    @Around("myPointcut() && args(request, ..)")
    public Object myAdviceWithParams(ProceedingJoinPoint thisJoinPoint, Object request) {
        System.out.println(">>> matching advice on " + thisJoinPoint);
        System.out.println("First parameter = " + request);
        Object result = thisJoinPoint.proceed();
        System.out.println("<<< matching advice on " + thisJoinPoint);
        return result;
    }
}

This is acceptable as well as elegant because it does not generate byte code twice per joinpoint. The two pointcuts are mutually exclusive, so this is a good thing to do. I recommend this solution.

Well I did feel it was a code smell but really that is exactly what I want to do. I want to call that around method for methods with any number of parameters, but if there is only 1 parameter specified, I want it to put it in the request object. Do you know of any better way of achieving this without duplicating code? Removing code duplication is really the main reason we started using AspectJ. — Vlad Schnakovszki, Feb 28 '16 at 11:50
Yes, I do know a better way. But a few more questions first: If there are zero parameters, should the advice also fire? If there are more than one parameters, should the first one still be bound? Should the first parameter always be bound or only if it matches a specific type? And what about your fear of duplication? How much code is there in the actual advice? Just a few lines or much more? Why not factor it out into a normal method? One more time: **Never** call one advice from another one, I proved to you with my first example how unealthy that is. — kriegaex, Feb 28 '16 at 13:19
The method should fire regardless of the number of parameters, but if only 1 parameter is present, regardless of its type, it needs to be placed in the variable to be used. As for the method, it's doing exception handling as well so it can't really be factored out and really shouldn't be duplicated, for all the reasons code duplication is bad. — Vlad Schnakovszki, Feb 28 '16 at 15:11
That second update does contain exactly what I was looking for. Thank you! I've marked it as the answer and made a note in mine pointing to yours. — Vlad Schnakovszki, Feb 29 '16 at 13:59
Thank you very much. One caveat though: With the delegate solution you cannot differentiate between no args and the first arg being null. Take care, this could cause problems or at least ambiguity. — kriegaex, Feb 29 '16 at 20:32

score 0 · Answer 2 · edited May 23 '17 at 11:58

Please see kriegaex's answer for more details. Leaving this here for completeness.

I ended up implementing a dummy project to test this. The answer is that the method will only be executed once. I've implemented the following:

Aspects.java:

package base;

import org.aspectj.lang.ProceedingJoinPoint;
import org.aspectj.lang.annotation.Around;
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.Pointcut;

@Aspect
public class Aspects {
    @Pointcut("@annotation(java.lang.Deprecated)")
    public void hasErrorResponseMethod() {
    }

    @Around("hasErrorResponseMethod()")
    public Object around(ProceedingJoinPoint point) throws Throwable {
        System.out.println("In CHILD advice.");
        return around(point, null);
    }

    @Around("hasErrorResponseMethod() && args(request)")
    public Object around(ProceedingJoinPoint point, Object request) throws Throwable {
        System.out.println("In PARENT advice with request " + request);
        return point.proceed();
    }
}

Methods.java:

package base;

public class Methods {
    private static int COUNT = 1;
    @Deprecated
    public int method1(String param) {
        System.out.println(">>> In method1! Param: " + param);
        return COUNT++;
    }
}

applicationContext.xml:

<beans xmlns="http://www.springframework.org/schema/beans"
       xmlns:context="http://www.springframework.org/schema/context"
       xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
       xmlns:aop="http://www.springframework.org/schema/aop"
       xsi:schemaLocation="http://www.springframework.org/schema/beans
    http://www.springframework.org/schema/beans/spring-beans-3.0.xsd
    http://www.springframework.org/schema/context
    http://www.springframework.org/schema/context/spring-context.xsd
    http://www.springframework.org/schema/aop
    http://www.springframework.org/schema/aop/spring-aop-3.0.xsd">

    <context:component-scan base-package="base" annotation-config="true" />

    <aop:aspectj-autoproxy />

    <bean id="logAspect" class="base.Aspects"/>

    <bean id="methods" class="base.Methods"/>
    <bean id="main" class="base.Main"/>

</beans>

Main.java:

package base;

import org.springframework.context.ApplicationContext;
import org.springframework.context.support.ClassPathXmlApplicationContext;

public class Main {
    public static void main(String[] args) {
        ApplicationContext context = new ClassPathXmlApplicationContext("applicationContext.xml");

        Methods methods = (Methods) context.getBean("methods");
        System.out.println("<<< Result: " + Methods.method1("REQUEST_VALUE"));
    }
}

The output is the following:

In PARENT advice with request REQUEST_VALUE
In CHILD advice.
In PARENT advice with request null
>>> In method1! Param: REQUEST_VALUE
<<< Result: 1

As you can see, the method is only called once, by the what-appears-to-be-more-particular advice. It would still be great to know how AspectJ decides which one of the two will call it.

Please **never** use aspects like this with one advice calling another one! See my own answer, proving the possibly unwanted side-effects and explaining what to do instead. — kriegaex, Feb 28 '16 at 10:34

AspectJ Handling of Multiple Matching Advices

2 Answers2