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

Fix LTW with a parallel-capable class loader and generated classes #278

Merged
merged 6 commits into from
Feb 8, 2024
Merged

Fix LTW with a parallel-capable class loader and generated classes #278

merged 6 commits into from
Feb 8, 2024

Commits on Feb 8, 2024

  1. IT reproducing GitHub issue 279 

    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 @urisimchoni
    kriegaex and urisimchoni committed Feb 8, 2024
    Configuration menu
    Copy the full SHA
    cd8e8a1 View commit details
    Browse the repository at this point in the history

  2. Change value of woven class in generatedClasses map 

    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.
    @urisimchoni @kriegaex
    urisimchoni authored and kriegaex committed Feb 8, 2024
    Configuration menu
    Copy the full SHA
    0f7b2e7 View commit details
    Browse the repository at this point in the history

  3. Use previous weaving result if classes were generated during weaving 

    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.
    @urisimchoni @kriegaex
    urisimchoni authored and kriegaex committed Feb 8, 2024
    Configuration menu
    Copy the full SHA
    499eee1 View commit details
    Browse the repository at this point in the history

  4. Remove WeavingAdaptor.couldWeave() 

    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.
    @urisimchoni @kriegaex
    urisimchoni authored and kriegaex committed Feb 8, 2024
    Configuration menu
    Copy the full SHA
    ae96aff View commit details
    Browse the repository at this point in the history

  5. Store only weaved class names in the generatedClasses map 

    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.
    @urisimchoni @kriegaex
    urisimchoni authored and kriegaex committed Feb 8, 2024
    Configuration menu
    Copy the full SHA
    8a4aa03 View commit details
    Browse the repository at this point in the history

  6. Factor out some anonymous IWeaveRequestor classes to inner classes 

    Drive-by cosmetics in the context of eclipse-aspectj#279.

Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>
    @kriegaex
    kriegaex committed Feb 8, 2024
    Configuration menu
    Copy the full SHA
    703d50b View commit details
    Browse the repository at this point in the history