Improved fix for #285

1. Write SAME_BYTES to cacheMap when woven bytes are null
2. Fix TODO in SimpleCache::getAndInitialize, using Optional to help
   indicate cache hit for unwoven class
3. Improve test coverage (cache miss, cache hit for unwoven class)

Relates to #285.

Co-authored-by: Alexander Kriegisch <Alexander@Kriegisch.name>
Signed-off-by: KimmingLau <294001791@qq.com>
Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>

diff --git a/loadtime/src/main/java/org/aspectj/weaver/loadtime/Aj.java b/loadtime/src/main/java/org/aspectj/weaver/loadtime/Aj.java
index 4dbda9023ac8801a769c49f192b24be9ee9d2a05..58fd3c5577b6caf84e080444dc1fdf6b03cebdc2 100644 (file)
--- a/loadtime/src/main/java/org/aspectj/weaver/loadtime/Aj.java
+++ b/loadtime/src/main/java/org/aspectj/weaver/loadtime/Aj.java
@@ -20,6 +20,7 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.StringTokenizer;
 
@@ -104,9 +105,9 @@ public class Aj implements ClassPreProcessor {
                        synchronized (classLoader) {
 
                                if (SimpleCacheFactory.isEnabled()) {
-                                       byte[] cacheBytes= laCache.getAndInitialize(className, bytes, classLoader, protectionDomain);
-                                       if (cacheBytes!=null){
-                                                       return cacheBytes;
+                                       Optional<byte[]> cacheBytes = laCache.getAndInitialize(className, bytes, classLoader, protectionDomain);
+                                       if (cacheBytes != null){
+                                               return cacheBytes.orElse(null);
                                        }
                                }
 

diff --git a/weaver/src/main/java/org/aspectj/weaver/tools/cache/SimpleCache.java b/weaver/src/main/java/org/aspectj/weaver/tools/cache/SimpleCache.java
index 31102c96f4aed66f96e3c9f0fc1dc0485ade4e1c..d94e9ce089b7e65d065fa8f584bc667ce51ffc65 100644 (file)
--- a/weaver/src/main/java/org/aspectj/weaver/tools/cache/SimpleCache.java
+++ b/weaver/src/main/java/org/aspectj/weaver/tools/cache/SimpleCache.java
@@ -15,6 +15,7 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
 import java.util.zip.CRC32;
 
 import org.aspectj.weaver.Dump;
@@ -37,7 +38,7 @@ import org.aspectj.weaver.tools.TraceFactory;
 public class SimpleCache {
 
        private static final String SAME_BYTES_STRING = "IDEM";
-       private static final byte[] SAME_BYTES = SAME_BYTES_STRING.getBytes();
+       static final byte[] SAME_BYTES = SAME_BYTES_STRING.getBytes();
 
        private Map<String, byte[]> cacheMap;
        private boolean enabled = false;
@@ -64,7 +65,7 @@ public class SimpleCache {
                }
        }
 
-       public byte[] getAndInitialize(String classname, byte[] bytes,
+       public Optional<byte[]> getAndInitialize(String classname, byte[] bytes,
                        ClassLoader loader, ProtectionDomain protectionDomain) {
                if (!enabled) {
                        return null;
@@ -72,14 +73,12 @@ public class SimpleCache {
                byte[] res = get(classname, bytes);
 
                if (Arrays.equals(SAME_BYTES, res)) {
-                       return null;
-               } else {
-                       if (res != null) {
-                               initializeClass(classname, res, loader, protectionDomain);
-                       }
-                       return res;
+                       return Optional.empty();
+               } else if (res != null) {
+                       initializeClass(classname, res, loader, protectionDomain);
+                       return Optional.of(res);
                }
-
+               return null;
        }
 
        private byte[] get(String classname, byte bytes[]) {

diff --git a/weaver/src/test/java/org/aspectj/weaver/tools/cache/SimpleClassCacheTest.java b/weaver/src/test/java/org/aspectj/weaver/tools/cache/SimpleClassCacheTest.java
index c8cbc8245ecec06b8575f57fbf79526a2c5fe780..e74b634a66efcc177424d7b7bd59e691710d38fe 100644 (file)
--- a/weaver/src/test/java/org/aspectj/weaver/tools/cache/SimpleClassCacheTest.java
+++ b/weaver/src/test/java/org/aspectj/weaver/tools/cache/SimpleClassCacheTest.java
@@ -12,60 +12,83 @@
 
 package org.aspectj.weaver.tools.cache;
 
-import java.io.File;
-
 import junit.framework.TestCase;
 
-/**
- */
-public class SimpleClassCacheTest extends TestCase {
-       byte[] FAKE_BYTES_V1 = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
-       byte[] FAKE_BYTES_V2 = {1, 1, 2, 3, 4, 5, 6, 7, 8, 9};
+import java.io.File;
 
-       byte[] FAKE_WOVEN_BYTES_V1 = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9,10};
-       byte[] FAKE_WOVEN_BYTES_V2 = {1, 1, 2, 3, 4, 5, 6, 7, 8, 9,10};
+public class SimpleClassCacheTest extends TestCase {
+       byte[] FAKE_BYTES_V1 = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 };
+       byte[] FAKE_BYTES_V2 = { 1, 1, 2, 3, 4, 5, 6, 7, 8, 9 };
 
+       byte[] FAKE_WOVEN_BYTES_V1 = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
+       byte[] FAKE_WOVEN_BYTES_V2 = { 1, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
 
-       private SimpleCache createCache() throws Exception {
-               return new SimpleCache(System.getProperty("java.io.tmpdir"),true);
+       private SimpleCache createCache() {
+               return new SimpleCache(System.getProperty("java.io.tmpdir"), true);
        }
 
-
-       public void testCache() throws Exception {
+       public void testCache() {
                String classA = "com.generated.A";
                SimpleCache cache = createCache();
-
                cache.put(classA, FAKE_BYTES_V1, FAKE_WOVEN_BYTES_V1);
 
+               // Returned woven bytes are the original ones
+               byte[] result = cache.getAndInitialize(classA, FAKE_BYTES_V1, null, null).orElse(null);
+               assertNotNull(result);
+               for (int i = 0; i < result.length; i++)
+                       assertEquals(
+                               "Cached version byte[" + i + "] should be equal to the original woven class",
+                               result[i], FAKE_WOVEN_BYTES_V1[i]
+                       );
+
+               // Class is properly backed up
+               File f = new File(System.getProperty("java.io.tmpdir") + File.separator + "com.generated.A-1164760902");
+               assertTrue(
+                       "Class should be backed up with CRC 1164760902",
+                       f.exists()
+               );
+       }
+
+       public void testDifferentVersionCache() {
+               String classA = "com.generated.A";
+               SimpleCache cache = createCache();
+               cache.put(classA, FAKE_BYTES_V1, FAKE_WOVEN_BYTES_V1);
+               cache.put(classA, FAKE_BYTES_V2, FAKE_WOVEN_BYTES_V2);
 
-               // Test the returned woven bytes are the original one
-               byte result[] = cache.getAndInitialize(classA, FAKE_BYTES_V1, null, null);
-               for(int i = 0; i < result.length; i ++){
-                       assertEquals("Cached version byte[" +i+"] should be equal to the original woven classe",result[i],FAKE_WOVEN_BYTES_V1[i]);
-               }
+               // Returned woven bytes are the original ones for v1
+               byte[] result = cache.getAndInitialize(classA, FAKE_BYTES_V1, null, null).orElse(null);
+               assertNotNull(result);
+               for (int i = 0; i < result.length; i++)
+                       assertEquals(
+                               "Cached version v1 byte[" + i + "] should be equal to the original woven class",
+                               result[i], FAKE_WOVEN_BYTES_V1[i]
+                       );
+
+               // Returned woven bytes are the original ones for v2
+               result = cache.getAndInitialize(classA, FAKE_BYTES_V2, null, null).orElse(null);
+               assertNotNull(result);
+               for (int i = 0; i < result.length; i++)
+                       assertEquals(
+                               "Cached version v2 byte[" + i + "] should be equal to the original woven class",
+                               result[i], FAKE_WOVEN_BYTES_V2[i]
+                       );
+       }
 
-               // Assure the class is properly backed up in the backing folder
-               File f = new File (System.getProperty("java.io.tmpdir") + File.separator + "com.generated.A-1164760902");
-               assertTrue("Class should be backed up to backing folder, with te CRC:1164760902 ",f.exists());
+       public void testCacheMiss() {
+               String classA = "com.generated.A";
+               SimpleCache cache = createCache();
 
+               // Woven bytes not found in cache
+               assertNull(cache.getAndInitialize(classA, FAKE_BYTES_V1, null, null));
        }
 
-       public void testDifferentVersionCache() throws Exception {
+       public void testCacheHitUnwoven() {
                String classA = "com.generated.A";
                SimpleCache cache = createCache();
-               cache.put(classA, FAKE_BYTES_V1, FAKE_WOVEN_BYTES_V1);
-               cache.put(classA, FAKE_BYTES_V2, FAKE_WOVEN_BYTES_V2);
+               cache.put(classA, FAKE_BYTES_V1, SimpleCache.SAME_BYTES);
 
-               // Test the returned woven bytes are the original one for v1
-               byte result[] = cache.getAndInitialize(classA, FAKE_BYTES_V1, null, null);
-               for(int i = 0; i < result.length; i ++){
-                       assertEquals("Cached version v1 byte[" +i+"] should be equal to the original woven classe",result[i],FAKE_WOVEN_BYTES_V1[i]);
-               }
-
-               // Test the returned woven bytes are the original one for v2
-               result = cache.getAndInitialize(classA, FAKE_BYTES_V2, null, null);
-               for(int i = 0; i < result.length; i ++){
-                       assertEquals("Cached version v2 byte[" +i+"] should be equal to the original woven classe",result[i],FAKE_WOVEN_BYTES_V2[i]);
-               }
+               // Returned woven bytes are null, indicating an unwoven class
+               byte[] result = cache.getAndInitialize(classA, FAKE_BYTES_V1, null, null).orElse(null);
+               assertNull(result);
        }
 }