eclipse-aspectj / aspectj

Other
303 stars 86 forks source link

Problem with abstract aspects and cflow(within()) #238

Open ttho opened 1 year ago

ttho commented 1 year ago

Given the following abstract and concrete aspect:

package de.tt;

import org.aspectj.lang.JoinPoint;
import org.aspectj.lang.annotation.Aspect;
import org.aspectj.lang.annotation.Before;
import org.aspectj.lang.annotation.Pointcut;

@Aspect
public abstract class AbstractAspect {

    @Pointcut
    abstract void myPointcut();

    @Before("myPointcut()")
    public void before(JoinPoint jp) {
        System.out.println("before " + jp);
    }
}
package de.tt;

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

@Aspect
public class ConcreteAspect extends AbstractAspect {

    @Pointcut("cflow(within(de.tt.ConcreteAspect)) || cflow(within(de.tt.AbstractAspect))") 
    public void self() {
    }

    @Pointcut("!self() && call(* *(..))")
    public void myPointcut() {
    }

}

I get the following error when load-time weaving

package de.tt;

public class Main {

    public static void main(String[] args) {
        System.out.println("hello");
    }

}

Exception in thread "main" java.lang.ExceptionInInitializerError at de.tt.Main.main(Main.java:6) Caused by: java.lang.NullPointerException at de.tt.AbstractAspect.<clinit>(AbstractAspect.java:1) ... 1 more

Why is that? Interestingly, the error goes away when

Background: I am writing a very general aspect for access tracking and want to exclude any calls from my aspects. Is there a better way to achieve that?

kriegaex commented 1 year ago

Sorry for replying so late. Thanks for the problem report. I understand why you might want to use that pointcut. If your tracing aspect is the only one active and there is nothing like nested aspect execution, you could use !cflow(adviceexecution()) && call(* *(..)) as a workaround. But if you use other aspects around (not before or after) the same code you want to trace, the corresponding calls would be excluded.

The problem you are describing originates in the fact that the base aspect constructor wants to use a control flow counter from the sub aspect. That counter is static and should be initialised when the sub aspect is going through static initialisation, i.e. during class-loading. I would have expected that that would have happened already when the base aspect constructor is executing. Why this is not the case, is yet to be investigated. A workaround for now would be not to extend a base aspect but to put everything into a single aspect.

kriegaex commented 1 year ago

A better solution would be, adapted from an example in the AspectJ Programming Guide:

pointcut myAdvice():   adviceexecution() && within(AbstractAspect+);
pointcut self():       cflow(myAdvice());
pointcut myPointcut(): !self() && call(* *(..));

The addition of && within(AbstractAspect+) makes it more precise. Sorry for the native AspectJ syntax. I guess you know how to convert that to annotation syntax.

You can, of course, also merge the first two pointcuts into one:

pointcut self():       cflow(adviceexecution() && within(AbstractAspect+));
pointcut myPointcut(): !self() && call(* *(..));

Unrelated to this issue, but an idea for you: If you also want to trace constructor calls, you could do this:

pointcut myPointcut(): !self() && (call(* *(..)) || call(*.new(..)));

The trace would still miss staticinitialization, preinitialization and initialization pointcuts, if these would be of any interest, but your own pointcut is just fine for method calls.

kriegaex commented 1 year ago

I think, I am not going to do anything to cover the corner case you just uncovered, because there is a good, canonical workaround. Generated aspect byte code would get more complex and potentially slower, too. The problem with within(AbstractAspect+) is, that it matches many joinpoints, among them static class initialisation, instance pre-initialisation and initialisation. Combining it with adviceexecution() is easy and expresses more clearly what you want in this case.

@aclement, do you think this ought to be fixed or improved?

ttho commented 1 year ago

@kriegaex, sorry for late feedback. With your explanations I was able to fix my problem, thank you very much.