NPE when using LTW cache

[KimmingLau]

ltw-npe-reproducer.zip
step to reproduce:

1. unzip ltw-npe-reproducer.zip
2. mvn clean package
3. cd target; unzip ltw-npe-reproducer-1.0-SNAPSHOT-zip.zip
4. java -javaagent:aspectjweaver-1.9.21.1.jar -Daj.weaving.cache.enabled=true -Daj.weaving.cache.impl=shared -cp ltw-npe-reproducer-1.0-SNAPSHOT.jar com.foo.bar.Demo

[KimmingLau]

java.lang.NullPointerException: Cannot read the array length because "b" is null at java.base/java.io.FileOutputStream.write(FileOutputStream.java:346) at org.aspectj.weaver.tools.cache.SimpleCache$StoreableCachingMap.writeToPath(SimpleCache.java:256) at org.aspectj.weaver.tools.cache.SimpleCache$StoreableCachingMap.put(SimpleCache.java:194) at java.base/java.util.Collections$SynchronizedMap.put(Collections.java:2896) at org.aspectj.weaver.tools.cache.SimpleCache.put(SimpleCache.java:104) at org.aspectj.weaver.loadtime.Aj.preProcess(Aj.java:126) at org.aspectj.weaver.loadtime.ClassPreProcessorAgentAdapter.transform(ClassPreProcessorAgentAdapter.java:51) at java.instrument/java.lang.instrument.ClassFileTransformer.transform(ClassFileTransformer.java:244) at java.instrument/sun.instrument.TransformerManager.transform(TransformerManager.java:188) at java.instrument/sun.instrument.InstrumentationImpl.transform(InstrumentationImpl.java:610) at java.base/java.lang.ClassLoader.defineClass1(Native Method) at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1027) at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150) at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:862) at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:760) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:681) at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:639) at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188) at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:526) at java.base/java.lang.Class.forName0(Native Method) at java.base/java.lang.Class.forName(Class.java:534) at java.base/java.lang.Class.forName(Class.java:513) at java.base/sun.launcher.LauncherHelper.loadMainClass(LauncherHelper.java:797) at java.base/sun.launcher.LauncherHelper.checkAndLoadMain(LauncherHelper.java:692)

[KimmingLau]

Due to code changes in #280

[kriegaex]

Thanks. I am taking this seriously, but will be very busy with work and intercontinental travel for a few days. So please, forgive me if I get to this later than I actually want to. (I hate regression bugs!)

[kriegaex]

@KimmingLau Can you please test 1.9.22-SNAPSHOT from this repository? Thank you.

<repositories>
    <repository>
        <id>ossrh-snapshots</id>
        <url>https://oss.sonatype.org/content/repositories/snapshots</url>
        <releases>
            <enabled>false</enabled>
        </releases>
        <snapshots>
            <enabled>true</enabled>
            <updatePolicy>always</updatePolicy>
        </snapshots>
    </repository>
</repositories>

[KimmingLau]

Apreciate to do that, but actually I am confused that if SimpleCache#getAndInitialize() return null, how to distinguish between no cache and cache which indicate that the class no need to be woven? That's why I used Optional to express three possible returns from getAndInitialize() method.

[kriegaex]

You got a point there. My focus was to fix your problem, not to add an additional optimisation, given the fact that the problem solved was introduced in an optimisation step doing more than solving the problem you raised before about CDS archives. So, I was more conservative here.

Your PR did not communicate its intent by additional tests for these cases, and my simple fix also did not break any existing tests. So I thought, it was enough. Your point seems to be that the weaver is triggered, even though maybe there are unwoven classes in the cache already, testing again if they need to be woven. Am I understanding you correctly here? Please explain the rationale in more detail. Thank you.

[KimmingLau]

Your point seems to be that the weaver is triggered, even though maybe there are unwoven classes in the cache already, testing again if they need to be woven

Yes, that's my point. In the old version getAndInitialize() would return the original bytes for class that no need to be woven (which cache bytes are SAME_BYTES), this means the weaving process would not be triggered again. But now it return null in that case, means weaver would be triggered.

I feel like the behavior here seems to be different from before. Forgive me for not being very good at English and not being able to express what I mean very well.

[kriegaex]

I cherry-picked your commits, squashed them into one and modified them a little bit, also adding more tests. See 1f1d429. Thanks for your valuable contribution and reviewing my own commit.

[KimmingLau]

Thanks for the review and approval, especially the additional testing code. It's my honor to contribute to such an excellent project.