From a97bb76a14c0f8d0712c9db57de81643adb1666a Mon Sep 17 00:00:00 2001 From: Alexander Kriegisch Date: Mon, 8 Apr 2024 10:17:42 +0200 Subject: [PATCH] SimpleCache: structural refactoring, better docs - Add some code comments and javadoc for SimpleCache::getAndInitialize - Add TODO to migrate from using ClassLoader::defineClass to the class definition strategy used in ClassLoaderWeavingAdaptor Signed-off-by: Alexander Kriegisch --- .../weaver/tools/cache/SimpleCache.java | 247 +++++++++--------- 1 file changed, 130 insertions(+), 117 deletions(-) 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 d94e9ce08..bbe793e28 100644 --- a/weaver/src/main/java/org/aspectj/weaver/tools/cache/SimpleCache.java +++ b/weaver/src/main/java/org/aspectj/weaver/tools/cache/SimpleCache.java @@ -10,6 +10,8 @@ import java.io.ObjectInputStream; import java.io.ObjectOutputStream; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Method; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; import java.security.ProtectionDomain; import java.util.Arrays; import java.util.Collections; @@ -22,7 +24,6 @@ import org.aspectj.weaver.Dump; import org.aspectj.weaver.tools.Trace; import org.aspectj.weaver.tools.TraceFactory; - /******************************************************************************* * Copyright (c) 2012 Contributors. * All rights reserved. @@ -38,9 +39,9 @@ import org.aspectj.weaver.tools.TraceFactory; public class SimpleCache { private static final String SAME_BYTES_STRING = "IDEM"; - static final byte[] SAME_BYTES = SAME_BYTES_STRING.getBytes(); + static final byte[] SAME_BYTES = SAME_BYTES_STRING.getBytes(StandardCharsets.UTF_8); - private Map cacheMap; + private final Map cacheMap; private boolean enabled = false; // cache for generated classes @@ -57,35 +58,55 @@ public class SimpleCache { if (enabled) { String generatedCachePath = folder + File.separator + GENERATED_CACHE_SUBFOLDER; - File f = new File (generatedCachePath); - if (!f.exists()){ + File f = new File(generatedCachePath); + if (!f.exists()) { f.mkdir(); } - generatedCache = Collections.synchronizedMap(StoreableCachingMap.init(generatedCachePath,0)); + generatedCache = Collections.synchronizedMap(StoreableCachingMap.init(generatedCachePath, 0)); } } - public Optional getAndInitialize(String classname, byte[] bytes, - ClassLoader loader, ProtectionDomain protectionDomain) { + /** + * Get bytes for given class from cache. If necessary, define and initialise the class first. + * + * @param classname name of class to be retrieved from the cache + * @param bytes class bytes (used to calculate cache key) + * @param loader class loader + * @param protectionDomain protection domain + * + * @return {@code null}, if the cache is disabled or if it contains no entry for the given class. An + * {@code Optional} value, if the cache knows about the class. The optional will be empty, if the cache entry + * represents and unwoven class, i.e. its bytes are identical to the original bytes. + */ + @SuppressWarnings("OptionalAssignedToNull") + public Optional getAndInitialize( + String classname, + byte[] bytes, + ClassLoader loader, + ProtectionDomain protectionDomain + ) + { if (!enabled) { + // Cache disabled return null; } byte[] res = get(classname, bytes); - if (Arrays.equals(SAME_BYTES, res)) { + // Cache hit: unwoven class return Optional.empty(); - } else if (res != null) { + } + if (res != null) { + // Cache hit: woven class initializeClass(classname, res, loader, protectionDomain); return Optional.of(res); } + // Cache miss return null; } - private byte[] get(String classname, byte bytes[]) { + private byte[] get(String classname, byte[] bytes) { String key = generateKey(classname, bytes); - - byte[] res = cacheMap.get(key); - return res; + return cacheMap.get(key); } public void put(String classname, byte[] origbytes, byte[] wovenbytes) { @@ -107,57 +128,57 @@ public class SimpleCache { checksum.update(bytes); long crc = checksum.getValue(); classname = classname.replace("/", "."); - return new String(classname + "-" + crc); + return classname + "-" + crc; } private static class StoreableCachingMap extends HashMap { - private String folder; + + // TODO: This class extends a raw HashMap, but instances of this class are assigned to fields + // Map cacheMap and Map generatedCache without casts. However, we cannot + // simply declare 'extends HashMap', because 'put' writes String values (paths) when given + // byte[] ones, while 'get' geturns byte[] ones, which is inconsistent. I.e., superficially the class behaves + // like a Map, while not really being one. This is ugly and hard to understand. + + private final String folder; private static final String CACHENAMEIDX = "cache.idx"; private long lastStored = System.currentTimeMillis(); - private static int DEF_STORING_TIMER = 60000; //ms - private int storingTimer; + private static final int DEF_STORING_TIMER = 60000; //ms + private final int storingTimer; private transient Trace trace; - private void initTrace(){ + + private void initTrace() { trace = TraceFactory.getTraceFactory().getTrace(StoreableCachingMap.class); } -// private StoreableCachingMap(String folder) { -// this.folder = folder; -// initTrace(); -// } - - private StoreableCachingMap(String folder, int storingTimer){ + private StoreableCachingMap(String folder, int storingTimer) { this.folder = folder; initTrace(); this.storingTimer = storingTimer; } public static StoreableCachingMap init(String folder) { - return init(folder,DEF_STORING_TIMER); - + return init(folder, DEF_STORING_TIMER); } public static StoreableCachingMap init(String folder, int storingTimer) { File file = new File(folder + File.separator + CACHENAMEIDX); if (file.exists()) { - try { - ObjectInputStream in = new ObjectInputStream( - new FileInputStream(file)); + try (ObjectInputStream in = new ObjectInputStream(Files.newInputStream(file.toPath()))) { // Deserialize the object StoreableCachingMap sm = (StoreableCachingMap) in.readObject(); sm.initTrace(); - in.close(); return sm; - } catch (Exception e) { + } + catch (Exception e) { Trace trace = TraceFactory.getTraceFactory().getTrace(StoreableCachingMap.class); trace.error("Error reading Storable Cache", e); } } - return new StoreableCachingMap(folder,storingTimer); + return new StoreableCachingMap(folder, storingTimer); } @@ -170,11 +191,13 @@ public class SimpleCache { return SAME_BYTES; } return readFromPath(path); - } else { + } + else { return null; } - } catch (IOException e) { - trace.error("Error reading key:"+obj.toString(),e); + } + catch (IOException e) { + trace.error("Error reading key:" + obj.toString(), e); Dump.dumpWithException(e); } return null; @@ -183,102 +206,94 @@ public class SimpleCache { @Override public Object put(Object key, Object value) { try { - String path = null; + String path; byte[] valueBytes = (byte[]) value; if (Arrays.equals(valueBytes, SAME_BYTES)) { path = SAME_BYTES_STRING; - } else { + } + else { path = writeToPath((String) key, valueBytes); } Object result = super.put(key, path); storeMap(); return result; - } catch (IOException e) { - trace.error("Error inserting in cache: key:"+key.toString() + "; value:"+value.toString(), e); + } + catch (IOException e) { + trace.error("Error inserting in cache: key:" + key + "; value:" + value.toString(), e); Dump.dumpWithException(e); } return null; } - public void storeMap() { long now = System.currentTimeMillis(); - if ((now - lastStored ) < storingTimer){ + if ((now - lastStored) < storingTimer) { return; } - File file = new File(folder + File.separator + CACHENAMEIDX);; - try { - ObjectOutputStream out = new ObjectOutputStream( - new FileOutputStream(file)); + File file = new File(folder + File.separator + CACHENAMEIDX); + try (ObjectOutputStream out = new ObjectOutputStream(Files.newOutputStream(file.toPath()))) { // Deserialize the object out.writeObject(this); - out.close(); lastStored = now; - } catch (Exception e) { - trace.error("Error storing cache; cache file:"+file.getAbsolutePath(), e); + } + catch (Exception e) { + trace.error("Error storing cache; cache file:" + file.getAbsolutePath(), e); Dump.dumpWithException(e); } } private byte[] readFromPath(String fullPath) throws IOException { - FileInputStream is = null ; - try{ - is = new FileInputStream(fullPath); + try ( + FileInputStream is = new FileInputStream(fullPath); + ByteArrayOutputStream buffer = new ByteArrayOutputStream() + ) { + int nRead; + byte[] data = new byte[16384]; + while ((nRead = is.read(data, 0, data.length)) != -1) { + buffer.write(data, 0, nRead); + } + buffer.flush(); + return buffer.toByteArray(); } - catch (FileNotFoundException e){ - //may be caused by a generated class that has been stored in generated cache but not saved at cache folder - System.out.println("FileNotFoundExceptions: The aspectj cache is corrupt. Please clean it and reboot the server. Cache path:"+this.folder ); + catch (FileNotFoundException e) { + // May be caused by a generated class that has been stored in generated cache but not saved in cache folder + System.out.println("FileNotFoundExceptions: The aspectj cache is corrupt. Please clean it and reboot the server. Cache path:" + this.folder); e.printStackTrace(); return null; } - ByteArrayOutputStream buffer = new ByteArrayOutputStream(); - - int nRead; - byte[] data = new byte[16384]; - - while ((nRead = is.read(data, 0, data.length)) != -1) { - buffer.write(data, 0, nRead); - } - - buffer.flush(); - is.close(); - return buffer.toByteArray(); - } private String writeToPath(String key, byte[] bytes) throws IOException { String fullPath = folder + File.separator + key; - FileOutputStream fos = new FileOutputStream(fullPath); - fos.write(bytes); - fos.flush(); - fos.close(); + try (FileOutputStream fos = new FileOutputStream(fullPath)) { + fos.write(bytes); + fos.flush(); + } return fullPath; } } - private void initializeClass(String className, byte[] bytes, - ClassLoader loader, ProtectionDomain protectionDomain) { - String[] generatedClassesNames = getGeneratedClassesNames(className,bytes); - + private void initializeClass( + String className, byte[] bytes, + ClassLoader loader, ProtectionDomain protectionDomain + ) + { + String[] generatedClassesNames = getGeneratedClassesNames(className, bytes); if (generatedClassesNames == null) { return; } for (String generatedClassName : generatedClassesNames) { - byte[] generatedBytes = get(generatedClassName, bytes); - if (protectionDomain == null) { defineClass(loader, generatedClassName, generatedBytes); - } else { - defineClass(loader, generatedClassName, generatedBytes, - protectionDomain); } - + else { + defineClass(loader, generatedClassName, generatedBytes, protectionDomain); + } } - } private String[] getGeneratedClassesNames(String className, byte[] bytes) { @@ -301,7 +316,8 @@ public class SimpleCache { byte[] storedBytes = generatedCache.get(key); if (storedBytes == null) { generatedCache.put(key, generatedClassName.getBytes()); - } else { + } + else { String storedClasses = new String(storedBytes); storedClasses += GENERATED_CACHE_SEPARATOR + generatedClassName; generatedCache.put(key, storedClasses.getBytes()); @@ -312,61 +328,58 @@ public class SimpleCache { private Method defineClassWithProtectionDomainMethod = null; private void defineClass(ClassLoader loader, String name, byte[] bytes) { - - Object clazz = null; - try { if (defineClassMethod == null) { + // TODO: Replace by class definition strategy used in ClassLoaderWeavingAdaptor defineClassMethod = ClassLoader.class.getDeclaredMethod( - "defineClass", new Class[] { String.class, - bytes.getClass(), int.class, int.class }); + "defineClass", + String.class, bytes.getClass(), int.class, int.class + ); + defineClassMethod.setAccessible(true); } - defineClassMethod.setAccessible(true); - clazz = defineClassMethod.invoke(loader, new Object[] { name, - bytes, 0, bytes.length}); - } catch (InvocationTargetException e) { + defineClassMethod.invoke(loader, name, bytes, 0, bytes.length); + } + catch (InvocationTargetException e) { if (e.getTargetException() instanceof LinkageError) { e.printStackTrace(); - } else { - System.out.println("define generated class failed" - + e.getTargetException()); } - } catch (Exception e) { + else { + System.out.println("define generated class failed" + e.getTargetException()); + } + } + catch (Exception e) { e.printStackTrace(); Dump.dumpWithException(e); } } - private void defineClass(ClassLoader loader, String name, byte[] bytes, - ProtectionDomain protectionDomain) { - - Object clazz = null; - + private void defineClass(ClassLoader loader, String name, byte[] bytes, ProtectionDomain protectionDomain) { try { - // System.out.println(">> Defining with protection domain " + name + - // " pd=" + protectionDomain); if (defineClassWithProtectionDomainMethod == null) { - defineClassWithProtectionDomainMethod = ClassLoader.class - .getDeclaredMethod("defineClass", new Class[] { - String.class, bytes.getClass(), int.class, - int.class, ProtectionDomain.class }); + // TODO: Replace by class definition strategy used in ClassLoaderWeavingAdaptor + defineClassWithProtectionDomainMethod = ClassLoader.class.getDeclaredMethod( + "defineClass", + String.class, bytes.getClass(), int.class, int.class, ProtectionDomain.class + ); + defineClassWithProtectionDomainMethod.setAccessible(true); } - defineClassWithProtectionDomainMethod.setAccessible(true); - clazz = defineClassWithProtectionDomainMethod.invoke(loader, - new Object[] { name, bytes, 0, - bytes.length, protectionDomain }); - } catch (InvocationTargetException e) { + defineClassWithProtectionDomainMethod.invoke(loader, name, bytes, 0, bytes.length, protectionDomain); + } + catch (InvocationTargetException e) { if (e.getTargetException() instanceof LinkageError) { e.printStackTrace(); // is already defined (happens for X$ajcMightHaveAspect // interfaces since aspects are reweaved) // TODO maw I don't think this is OK and - } else { + } + else { e.printStackTrace(); } - }catch (NullPointerException e) { - System.out.println("NullPointerException loading class:"+name+". Probabily caused by a corruput cache. Please clean it and reboot the server"); - } catch (Exception e) { + } + catch (NullPointerException e) { + System.out.println("NullPointerException loading class:" + name + ". Probabily caused by a corruput cache. Please clean it and reboot the server"); + } + catch (Exception e) { e.printStackTrace(); Dump.dumpWithException(e); } -- 2.39.5