Issue with equinox weaving on Eclipse version 2024-03 about equinox HOT 20 CLOSED

As per previous comment.

from equinox.

To the Equinox team: This is the AspectJ maintainer. I have zero knowledge about Equinox Weaving, but if you have any questions about AspectJ and the changes I mentioned in eclipse-aspectj/ajdt#57, by all means feel free to mention my user ID and ask me questions.

from equinox.

@martinlippert Do you know what could cause this?

from equinox.

@kriegaex I'm not sure what exactly is considered "changed" here something in AspectJ? Some change (wich one?) in Equinox or JDT?
As far as it comes to Equinox it is implementing the Weaving Hook Service Specification so can you mention what part of the specification is violated by Equinox here from your analysis?

from equinox.

Equinox weaving predates OSGi Weaving Hook Service Specification. The AspectJ weaving hooks into Equinox Weaving. Equinox Weaving is built upon Equinox Framework specific behavior.

The AspectJ weaving use to live with the Equinox project but since got moved out of the project. I don't recall now where they went but I think this issue belongs with that project. @martinlippert is the last developer I know that has touched the AspectJ weaving integration with Equinox Weaving.

from equinox.

@laeubi, I am reiterating: I have zero knowledge about Equinox Weaving. I do not even know if it is the root cause of the problem at all. Therefore, I am not claiming that anything is wrong there. I just made an educated guess, as LTW with plain AspectJ seems to work fine, both inside and outside of Eclipse, and I have yet to see a reproducer for the problem with a simple AspectJ LTW configuration in Eclipse.

What has changed in AspectJ that I think might have an impact on what is woven or not is in these commits:

• https://github.com/eclipse-aspectj/aspectj/pull/280/files
• https://github.com/eclipse-aspectj/aspectj/pull/289/files

The main affected weaver classes are:

• org.aspectj.weaver.loadtime.Aj
• org.aspectj.weaver.loadtime.ClassLoaderWeavingAdaptor
• org.aspectj.weaver.tools.WeavingAdaptor
• org.aspectj.weaver.tools.cache.SimpleCache

Like I said in the comment I linked to earlier:

Maybe it needs to be adjusted to recent changes (since 1.21.1, if I remember correctly) in the weaver, where in case of non-woven code null is returned instead of the original bytes.

The rationale behind the change is that now the AspectJ weaver behaves like most intrumentation agents, simply returning null for unchanged code instead of the input bytes, because that way it is easiest for the caller to find out if anything has changed at all, saving him the chore to compare two complete byte arrays.

from equinox.

Also see https://bugs.eclipse.org/bugs/show_bug.cgi?id=470000 where the aspectj weaving integration moved out of Equinox. I don't believe there is anything for Equinox to do here.

from equinox.

@tjwatson, sorry for the stupid question, but where did the AspectJ integration move to, except for the parts located in AJDT?

from equinox.

@tjwatson, sorry for the stupid question, but where did the AspectJ integration move to, except for the parts located in AJDT?

Not a stupid question. It is an answer I was searching for, but have not found (besides the discussions in https://bugs.eclipse.org/bugs/show_bug.cgi?id=470000). I am hoping @martinlippert sees this and can chime in.

from equinox.

@tjwatson, sorry for the stupid question, but where did the AspectJ integration move to, except for the parts located in AJDT?

https://bugs.eclipse.org/bugs/show_bug.cgi?id=470000#c33

The overall, AspectJ independent parts of the weaving infrastructure that we've built back then remained in Equinox, whereas the AspectJ specific implementation of the weaving moved out to the AJDT project back then.

I haven't looked at this code for years now, so can't really say anything from the top of my head. Looks like some debugging would be needed here in order to find out whether the weaver creates the additional (missing) bytecode and if so, check if the weaving infrastructure in Equinox correctly retrieves and uses this.

Since I am not actively working on this anymore, I wonder whether we should deprecate this implementation and load-time weaving of AspectJ aspects in Equinox for future releases. Opinions @tjwatson ?

from equinox.

The part that uses the home-grown weaving infrastructure in Equinox to do AspectJ weaving is here: https://github.com/eclipse-aspectj/ajdt/tree/master/org.eclipse.equinox.weaving.aspectj

from equinox.

The main implementation of the AspectJ lead-time weaving is implemented in here: https://github.com/eclipse-aspectj/ajdt/blob/master/org.eclipse.equinox.weaving.aspectj/src/org/eclipse/equinox/weaving/aspectj/loadtime/OSGiWeavingAdaptor.java - it calls and re-uses the ClassLoaderWeavingAdaptor from AspectJ.

from equinox.

Since I am not actively working on this anymore, I wonder whether we should deprecate this implementation and load-time weaving of AspectJ aspects in Equinox for future releases. Opinions @tjwatson ?

Right, I do think we need to deprecate Equinox weaving and ultimately stop building it in our quarterly releases. That is unless we get someone to step up that has time and interest to maintain these integrations.

from equinox.

@martinlippert, thanks for the hint about OSGiWeavingAdaptor. I will try to add a null guard to the weaveClass method and see if that helps to solve the problem or is totally unrelated. It is worth a try.

from equinox.

From a very high level, it looks like the weaver should have produced bytecode for this new inner class TypeBinding$AjcClosure1 as a result of weaving an aspect, but it looks like the weaving mechanism can't find that bytecode when trying to load it.

There is special handling for generated classes in the mechanism, you will stumble upon it when looking at the code. The basic idea is to keep the generated bytecodes for additional classes in memory until the JVM loads that class (as far as I remember).

from equinox.

I also vaguely remember a PR being raised a while ago, fixing problems with race conditions in this area. You might want to reach out to the reporter here #234, who did a great and very detailed job on this - just in case.

from equinox.

I think, I was able to locate the code in AJDT responsible for the problem. It is about defining classes in a different classloader, which both AspectJ and AJDT used the same method for. In AspectJ, this was updated to work with more recent JDKs already, but in AJDT there was still a copy of the old code. It is not so much a bug as a missing, but necessary adjustment to new Java runtimes.

Thanks @martinlippert and @tjwatson for providing the context information necessary for me to zero in on the problem. I think, the issue can be closed. I am going to fix the original problem in eclipse-aspectj/ajdt#57.

from equinox.

I think, I was able to locate the code in AJDT responsible for the problem. It is about defining classes in a different classloader, which both AspectJ and AJDT used the same method for. In AspectJ, this was updated to work with more recent JDKs already, but in AJDT there was still a caopy of the old code. It is not so much a bug as a missing, but necessary adjustment to new Java runtimes.

Thanks @martinlippert and @tjwatson for providing the context information necessary for me to zero in on the problem. I think, the issue can be closed. I am going to fix the original problem in eclipse-aspectj/ajdt#57.

Sounds good, thanks a lot @kriegaex for looking into this, much appreciated.

from equinox.

@martinlippert, maybe we were too optimistic to close the issue. The error still occurs, just in another situation. Would you mind reading the thread from eclipse-aspectj/ajdt#57 (comment) downwards? Sorry, quite a lot of text. Maybe you have an idea what has changed in Equinox weaving that could cause this or what AJDT needs to accomodate to to avoid the problem. Thank you.

from equinox.

@kriegaex replied on the AspectJ issue via eclipse-aspectj/ajdt#57 (comment)

from equinox.