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.

Sign up for GitHub

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

Jump to bottom

LTW sometimes does not work with jboss-modules class loader and around advice #279

Closed
urisimchoni opened this issue Feb 6, 2024 · 0 comments
Closed

LTW sometimes does not work with jboss-modules class loader and around advice #279

urisimchoni opened this issue Feb 6, 2024 · 0 comments
Assignees
@kriegaex
Labels
bug Something isn't working enhancement New feature or request
Milestone
1.9.21.1

Comments

@urisimchoni
Copy link
Contributor

When trying to apply LTW to DataSource classes on WildFly application server, we see that sometimes weaving silently fails. The two "ingredients" for this to happen appear to be:

  • More than one datasource of the same driver exists, and
  • Using an Around advice on DataSource method

Enabling debugging and weaveInfo shows that the advice is applied, yet the actual advice code does not run, as if weaving did not occur.

After some digging, the root cause appears to be as follows:

  • WildFly is using jboss-modules class loading system, which loads classes using org.jboss.modules.ModuleClassLoader class.
  • ModuleClassLoader does not lock anything while loading classes. It could very well call defineClass() to load the same class into the same class loader in two threads in parallel. Obviously one of the calls would fail in that case, but it recovers from this.
  • With two DataSource instances present, WildFly sets them up in two executor jobs that can run in parallel on different threads, hence causing defineClass() to be called on the DataSource class twice in parallel.
  • While defineClass() runs, AspectJ LTW agent is invoked to retransform the loaded class - so in our case it's invoked twice in parallel on the same weaver instance.
  • Usually, AspectJ has no problem weaving the class "twice", meaning that in each weaving operation AspectJ is receiving the original class bytes and is returning the modified class bytes, and it doesn't care how many times it happens or what happens in other threads.
  • However, with Around advice, an inner class is being created. AspectJ would not want to create the inner class twice.
  • So AspectJ has a mechanism to guard against weaving a class with an associated generated class twice on the same weaver.
  • The bug is that when such a "collision" is detected, AspectJ avoids weaving again (to avoid creating the generated class), but is returning the original class bytes and not the woven bytes.
  • Because all this is running in parallel in two threads, sometimes the thread that got to weave the class second (and therefore is seeing not-woven class) makes it first to installing the class in the ClassLoader, and so this un-woven class "wins".

Tested on versions 29 and 30 with several Java-17 distributions on Linux.

A reproducer project with a mock class loader can be found here: https://gitlab.com/urisimchoni/aspectj-parallel-issue
@urisimchoni urisimchoni mentioned this issue Feb 6, 2024
Merged
@kriegaex kriegaex self-assigned this Feb 7, 2024
@kriegaex kriegaex added the enhancement New feature or request label Feb 7, 2024
@kriegaex kriegaex added this to the 1.9.21.1 milestone Feb 7, 2024
@kriegaex kriegaex added the bug Something isn't working label Feb 7, 2024
kriegaex added a commit to urisimchoni/aspectj that referenced this issue Feb 8, 2024
@kriegaex @urisimchoni
IT reproducing GitHub issue 279
0c28ce6 
Test "same class woven concurrently in parallel-capable classloader"
reproduces eclipse-aspectj#279. Originally taken and modified from:
https://gitlab.com/urisimchoni/aspectj-parallel-issue

Co-authored-by: Uri Simchoni <urisimchoni@gmail.com>
Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex pushed a commit to urisimchoni/aspectj that referenced this issue Feb 8, 2024
@urisimchoni @kriegaex
Change value of woven class in generatedClasses map
cf2e7c4 
Store the woven class and not a generated (inner) class as value of the
woven class name. This has no effect in this commit, because the value
is not consulted at all, but will be used later on.

Relates to eclipse-aspectj#279.
kriegaex pushed a commit to urisimchoni/aspectj that referenced this issue Feb 8, 2024
@urisimchoni @kriegaex
Use previous weaving result if classes were generated during weaving
da31a21 
A parallel-capable class loader might load the same class multiple times
in multiple threads, see
https://docs.oracle.com/javase/7/docs/technotes/guides/lang/cl-mt.html.

In this scenario, the JVM instrumentation mechanism invokes the AspectJ
weaver twice on the same class. In simple weaving scenarios, this would
just cause weaving logic to run multiple times and produce the same
result for each thread, but if the weaver is generating new classes,
it is undesirable to generate those classes multiple times.

To avoid this, the generatedClasses map in WeavingAdaptor keeps track of
classes for which inner classes were generated and avoids re-weaving
them. However, before this change, if a class was found in the generated
map, the weaver would return the un-woven bytes, which would cause
AspectJ not to work for some of the threads.

This change returns the woven bytes instead of re-running weaving.

Fixes eclipse-aspectj#279.
kriegaex pushed a commit to urisimchoni/aspectj that referenced this issue Feb 8, 2024
@urisimchoni @kriegaex
Remove WeavingAdaptor.couldWeave()
9f71bb1 
WeavingAdaptor.couldWeave() tested two things: whether this specific
class (by its name) should be excluded from weaving, and whether the
class has generated classes associated with it (in which case we avoid
weaving to avoid re-creating those generated classes). However, if the
class has generated classes associated, couldWeave() is not called at
all because of the new wovenWithGeneratedClass() test, so we have
only the name test which is now called directly.

Relates to eclipse-aspectj#279.
kriegaex pushed a commit to urisimchoni/aspectj that referenced this issue Feb 8, 2024
@urisimchoni @kriegaex
Store only weaved class names in the generatedClasses map
faa36ca 
The generatedClasses map contained both keys of woven classes that had
generated classes associated with them, and the keys of the generated
classes themselves. It seems like this map is never consulted for the
generated class key - the generated class is generated from within the
context of woven class loading / retransformation, and thus no transform
event is generated for it by the JVM. Because of that, we do not need to
guard against re-weaving it. Other uses of generatedClasses map are for
full/empty tests, where the woven class key is sufficient. This change
simplifies deletion of a class because we do not have to look for its
associated generated classes.

Relates to eclipse-aspectj#279.
kriegaex added a commit to urisimchoni/aspectj that referenced this issue Feb 8, 2024
@kriegaex
Factor out some anonymous IWeaveRequestor classes to inner classes
6117924 
Drive-by cosmetics in the context of eclipse-aspectj#279.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit to urisimchoni/aspectj that referenced this issue Feb 8, 2024
@kriegaex @urisimchoni
IT reproducing GitHub issue 279
cd8e8a1 
Test "same class woven concurrently in parallel-capable classloader"
reproduces eclipse-aspectj#279. Originally taken and modified from:
https://gitlab.com/urisimchoni/aspectj-parallel-issue

Co-authored-by: Uri Simchoni <urisimchoni@gmail.com>
Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex pushed a commit to urisimchoni/aspectj that referenced this issue Feb 8, 2024
@urisimchoni @kriegaex
Change value of woven class in generatedClasses map
0f7b2e7 
Store the woven class and not a generated (inner) class as value of the
woven class name. This has no effect in this commit, because the value
is not consulted at all, but will be used later on.

Relates to eclipse-aspectj#279.
kriegaex pushed a commit to urisimchoni/aspectj that referenced this issue Feb 8, 2024
@urisimchoni @kriegaex
Use previous weaving result if classes were generated during weaving
499eee1 
A parallel-capable class loader might load the same class multiple times
in multiple threads, see
https://docs.oracle.com/javase/7/docs/technotes/guides/lang/cl-mt.html.

In this scenario, the JVM instrumentation mechanism invokes the AspectJ
weaver twice on the same class. In simple weaving scenarios, this would
just cause weaving logic to run multiple times and produce the same
result for each thread, but if the weaver is generating new classes,
it is undesirable to generate those classes multiple times.

To avoid this, the generatedClasses map in WeavingAdaptor keeps track of
classes for which inner classes were generated and avoids re-weaving
them. However, before this change, if a class was found in the generated
map, the weaver would return the un-woven bytes, which would cause
AspectJ not to work for some of the threads.

This change returns the woven bytes instead of re-running weaving.

Fixes eclipse-aspectj#279.
kriegaex pushed a commit to urisimchoni/aspectj that referenced this issue Feb 8, 2024
@urisimchoni @kriegaex
Remove WeavingAdaptor.couldWeave()
ae96aff 
WeavingAdaptor.couldWeave() tested two things: whether this specific
class (by its name) should be excluded from weaving, and whether the
class has generated classes associated with it (in which case we avoid
weaving to avoid re-creating those generated classes). However, if the
class has generated classes associated, couldWeave() is not called at
all because of the new wovenWithGeneratedClass() test, so we have
only the name test which is now called directly.

Relates to eclipse-aspectj#279.
kriegaex pushed a commit to urisimchoni/aspectj that referenced this issue Feb 8, 2024
@urisimchoni @kriegaex
Store only weaved class names in the generatedClasses map
8a4aa03 
The generatedClasses map contained both keys of woven classes that had
generated classes associated with them, and the keys of the generated
classes themselves. It seems like this map is never consulted for the
generated class key - the generated class is generated from within the
context of woven class loading / retransformation, and thus no transform
event is generated for it by the JVM. Because of that, we do not need to
guard against re-weaving it. Other uses of generatedClasses map are for
full/empty tests, where the woven class key is sufficient. This change
simplifies deletion of a class because we do not have to look for its
associated generated classes.

Relates to eclipse-aspectj#279.
kriegaex added a commit to urisimchoni/aspectj that referenced this issue Feb 8, 2024
@kriegaex
Factor out some anonymous IWeaveRequestor classes to inner classes
703d50b 
Drive-by cosmetics in the context of eclipse-aspectj#279.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Feb 8, 2024
@kriegaex @urisimchoni
IT reproducing GitHub issue 279
f9c692f 
Test "same class woven concurrently in parallel-capable classloader"
reproduces #279. Originally taken and modified from:
https://gitlab.com/urisimchoni/aspectj-parallel-issue

Co-authored-by: Uri Simchoni <urisimchoni@gmail.com>
Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex pushed a commit that referenced this issue Feb 8, 2024
@urisimchoni @kriegaex
Change value of woven class in generatedClasses map
5cdff3b 
Store the woven class and not a generated (inner) class as value of the
woven class name. This has no effect in this commit, because the value
is not consulted at all, but will be used later on.

Relates to #279.
kriegaex pushed a commit that referenced this issue Feb 8, 2024
@urisimchoni @kriegaex
Remove WeavingAdaptor.couldWeave()
82da670 
WeavingAdaptor.couldWeave() tested two things: whether this specific
class (by its name) should be excluded from weaving, and whether the
class has generated classes associated with it (in which case we avoid
weaving to avoid re-creating those generated classes). However, if the
class has generated classes associated, couldWeave() is not called at
all because of the new wovenWithGeneratedClass() test, so we have
only the name test which is now called directly.

Relates to #279.
kriegaex pushed a commit that referenced this issue Feb 8, 2024
@urisimchoni @kriegaex
Store only weaved class names in the generatedClasses map
8e98580 
The generatedClasses map contained both keys of woven classes that had
generated classes associated with them, and the keys of the generated
classes themselves. It seems like this map is never consulted for the
generated class key - the generated class is generated from within the
context of woven class loading / retransformation, and thus no transform
event is generated for it by the JVM. Because of that, we do not need to
guard against re-weaving it. Other uses of generatedClasses map are for
full/empty tests, where the woven class key is sufficient. This change
simplifies deletion of a class because we do not have to look for its
associated generated classes.

Relates to #279.
kriegaex added a commit that referenced this issue Feb 8, 2024
@kriegaex
Factor out some anonymous IWeaveRequestor classes to inner classes
dbce4ca 
Drive-by cosmetics in the context of #279.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
@kriegaex kriegaex mentioned this issue Feb 12, 2024
Open
kriegaex added a commit that referenced this issue Feb 12, 2024
@kriegaex
ajc1921.xml: Add AspectJ version hint to #279 test case
3c2458a 
"same class woven concurrently in parallel-capable classloader".

Relates to #279.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
kriegaex added a commit that referenced this issue Apr 8, 2024
@kriegaex
Revert WeavingAdaptor generated class map optimisation
8c88314 
This was introduced in commit 8a4aa03 of PR #278 contribution as part
of the #279 fix. The contributor thought that the generated closure
class entries were never used, but in fact AJDT class OSGiWeavingAdaptor
relies on the presence of those entries.

To the best of my present knowledge, it looks as if this change was the
root cause of eclipse-aspectj/ajdt#57.
Therefore, I reverted it, simultaneously refactoring Iterator::remove
usage to delete entries from the map to Collection::removeIf.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
This was referenced Apr 8, 2024
Merged
Closed
kriegaex added a commit that referenced this issue Apr 8, 2024
@kriegaex
Revert WeavingAdaptor generated class map optimisation
966326e 
This was introduced in commit 8a4aa03 of PR #278 contribution as part
of the #279 fix. The contributor thought that the generated closure
class entries were never used, but in fact AJDT class OSGiWeavingAdaptor
relies on the presence of those entries.

To the best of my present knowledge, it looks as if this change was the
root cause of eclipse-aspectj/ajdt#57.
Therefore, I reverted it, simultaneously refactoring Iterator::remove
usage to delete entries from the map to Collection::removeIf.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kriegaex kriegaex

Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
1.9.21.1
Development

No branches or pull requests
2 participants
@kriegaex @urisimchoni