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

Among multiple aspects only last one is being applied. #85

Closed
ksiczek opened this issue Sep 2, 2021 · 9 comments
Closed

Among multiple aspects only last one is being applied. #85

ksiczek opened this issue Sep 2, 2021 · 9 comments
Labels
question Further information is requested
Milestone

Comments

@ksiczek
Copy link

ksiczek commented Sep 2, 2021

Hello guys,

I have a quite large code base with multiple projects being built by Gradle and io.freefair.aspectj.post-compile-weaving plugin together with aspectj 1.9.7. We organized our aspects as separate modules or libraries with the intention to include what you need. One use case is to have @MonitoredMethod marker on methods to add some monitoring, another one @Cachable to add caching etc. Quite recently I have added another aspect that aimed to collect invocations of public methods and I noticed that even though ajc says

Join point 'method-execution(de.telekom.phoenix.common.login.model.LoginResult de.telekom.phoenix.common.login.AuthenticationManager.processLogin(atg.servlet.DynamoHttpServletRequest, de.telekom.integration.telcobusinesssuite.RedirectUrl, de.telekom.phoenix.common.login.ATGAuthentication, de.telekom.integration.telcobusinesssuite.ITbsClient))' in Type 'de.telekom.phoenix.common.login.AuthenticationManager' (AuthenticationManager.java:86) advised by before advice from 'pl.hycom.aop.PublicMethodCollectingAspect' (DeadCodeCollector-1.1.2-SNAPSHOT.jar!PublicMethodCollectingAspect.class(from PublicMethodCollectingAspect.java))
	
Join point 'method-execution(de.telekom.phoenix.common.login.model.LoginResult de.telekom.phoenix.common.login.AuthenticationManager.processLogin(atg.servlet.DynamoHttpServletRequest, de.telekom.integration.telcobusinesssuite.RedirectUrl, de.telekom.phoenix.common.login.ATGAuthentication, de.telekom.integration.telcobusinesssuite.ITbsClient))' in Type 'de.telekom.phoenix.common.login.AuthenticationManager' (AuthenticationManager.java:86) advised by around advice from 'de.telekom.phoenix.aspect.common.PerformanceMonitorSupport' (Commons-AOP-AspectJ-RC203.20200701.1.jar!PerformanceMonitorSupport.class:19(from PerformanceMonitorSupport.aj))

only the last aspect is being applied. In the above case was the PerformanceMonitorSupport connected to @MonitoredMethod. I decompiled the code and it was

@MonitoredMethod
/*     */    public LoginResult processLogin(DynamoHttpServletRequest req, RedirectUrl redirectUrl, ATGAuthentication atgAuthentication, ITbsClient tbsClient) throws LoginSecurityException {
/*     */       StaticPart var10000 = ajc$tjp_0;
/*     */       Object[] var11 = new Object[]{req, redirectUrl, atgAuthentication, tbsClient};
/*     */       JoinPoint var6 = Factory.makeJP(var10000, this, this, var11);
/*  86 */       PerformanceMonitorSupport var13 = PerformanceMonitorSupport.aspectOf();
/*     */       Object[] var12 = new Object[]{this, req, redirectUrl, atgAuthentication, tbsClient, var6};
/*     */       return (LoginResult)var13.ajc$around$de_telekom_phoenix_aspect_common_PerformanceMonitorSupport$1$5333d83b(new AjcClosure1(var12), ajc$tjp_0, var6);
/*     */    }

Is AspectJ designed to work that way? Can it be an issue with Gradle, or its plugin? If you feel it is a bug I could try to isolate it but I would like to have a short discussion first :)

@kriegaex
Copy link
Contributor

kriegaex commented Sep 2, 2021

In AspectJ, a joinpoint can be advised by multiple advice. So if you are experiencing divergent behaviour, I would definitely like to see a reproducer, ideally an MCVE. Just publish it on GitHub and link to it from here, then I can take a look.

@kriegaex
Copy link
Contributor

kriegaex commented Sep 3, 2021

I forgot to mention that for now, I think you probably rather have a configuration issue in your weaving plugin than in AspectJ, but I cannot say for sure without an example project. I also would not be surprised if, while creating the MCVE, you find out what is wrong, because this is a frequent side effect of condensing problems into MCVEs. Therefore, both of us have a chance to learn and understand something here.

@ksiczek
Copy link
Author

ksiczek commented Sep 3, 2021

Thank you @kriegaex. I have already created an MCVE but fortunately, or not, I was not able to reproduce the issue. I have also already learned that one of my aspects that is written in Java was not compiled using ajc but simply javac so it does not contain aspectOf() method and I do not know really how it could work at all. Anyway, I should have started with MCVE :) I will try to figure out what is wrong with my project and will get back to you if I find anything interesting.

@kriegaex
Copy link
Contributor

kriegaex commented Sep 3, 2021

An aspect compiled with javac could work when used in a LTW scenario or in a post-compile binary weaving step, if picked up by ajc. Changing the setup of post-compile weaving - things like inpath, aspect path - can influence the behaviour. But as I almost expected, you learned something when creating the MCVE, because it forced you to isolate the problem and inspect your settings. I am happy that you seem to be able to help yourself. 🙂

@ksiczek
Copy link
Author

ksiczek commented Sep 3, 2021

This is also interesting and mysterious to me because in MCVE I forgot to compile java aspects with ajc and, even though they were applied to the target class, I was getting NoSuchMethodError...aspectOf() in runtime. The MCVE looks like

|-- aspect1
|-- aspect2
|-- impl

where aspect1 and aspect2 are plain java libraries published to local maven repo and impl is a Gradle with java-application and [email protected] plugins. Compilation of impl worked fine (aspects are being applied) but during run I was getting the error. That is why I added ajc to aspect1 and aspect2. It helped for the runtime error, but I think you said that it should not matter, didn't you?

@kriegaex
Copy link
Contributor

kriegaex commented Sep 3, 2021

No, I didn't say that. Please let us not talk about your textual description of the MCVE, as we are only wasting our time, you explaining and I speculating. I cannot debug prose, I need to see your project. With Maven, I could help you quickly, because I use it all the time for my AspectJ projects. I never use Gradle. Probably I could help there, too, but I would need to reproduce the problem first.

In general, if you do not want to rely on unfinished aspects (compiled with javac only) to be finished during weaving, you should compile your aspect modules with ajc, properly finishing them. This is optional, you can also finish the aspects during weaving, but it makes things easier. I think that an aspect library should contain finished aspects and not rely on the user to take that step on behalf of the library provider.

Then, you also need ajc for every application module you wish to weave your aspects into, putting the aspect libraries on the aspect path this time.

Having said that, I am not going to answer any more questions without an MCVE. Please understand. Thank you.

@ksiczek
Copy link
Author

ksiczek commented Sep 3, 2021

Fair enough @kriegaex. I would appreciate it if you could have a look at https://github.com/ksiczek/org.aspectj_85 and tell me what you think about it, especially why the NoSuchMethodError is being thrown in that case. It does not mimic the original issue yet but it is rather simple and would help me better understand what should work, what might be a plugin issue, etc.

@kriegaex
Copy link
Contributor

kriegaex commented Sep 3, 2021

The MCVE helps, thank you. Because you did not follow my advice to compile the aspect modules with ajc, it is not enough to put them on the aspect path in the Gradle post-compile plugin configuration, because on the aspect path the compiler expects finished aspects in that case. When using unfinished aspects, better put them on the inpath instead (or additionally, in order to document your intent). I also recommend to define aspectjrt as a runtime dependency, because the AspectJ runtime is always needed when using compiled aspects. Your project also works without it, I am not sure why. Maybe the Freefair plugin somehow adds the dependency automatically.

The minimal change to successfully run your application is to change

aspect 'pl.example.aspects:aspect1:1.0.0-SNAPSHOT'
aspect 'pl.example.aspects:aspect2:1.0.0-SNAPSHOT'

to

inpath 'pl.example.aspects:aspect1:1.0.0-SNAPSHOT'
inpath 'pl.example.aspects:aspect2:1.0.0-SNAPSHOT'

Alternatively, use https://docs.freefair.io/gradle-plugins/5.0.1/reference/#_io_freefair_aspectj in your two aspect modules and leave the aspect on the aspect path. That would be the canonical solution.

Please also read more about the AspectJ compiler options in the AspectJ manual.

@aclement, please close this issue, because it is not really an AspectJ problem but simply a Gradle plugin configuration issue.

@ksiczek, in the future, please ask questions like these on StackOverflow, adding an aspectj tag, or on the AspectJ users mailing list. I do prefer SO, though.

@ksiczek
Copy link
Author

ksiczek commented Sep 3, 2021

Thank you @kriegaex, I learned enough to analyze the issue further on my own. I will keep in mind the point regarding SO.

@ksiczek ksiczek closed this as completed Sep 3, 2021
@kriegaex kriegaex added this to the 1.9.8 milestone Mar 21, 2022
@kriegaex kriegaex added the question Further information is requested label Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants