From e56f8afa03035140280bc8d3d878ad225def381e Mon Sep 17 00:00:00 2001 From: Christopher Vogt Date: Sun, 27 Nov 2016 16:00:58 -0500 Subject: replace flawed concurrent hashmap cache with consistent replacement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The concurrent hashmap approach to classloader caching was flawed. Assume you have two concurrently running builds A and B and projects P2 and P3 depending on project P1. And assume a time sequence where A compiles P1, then compiles P2, then P1’s sources change, then B compiles P1, then A compiles P3. At the end P2 and P3 will have different versions of P1 as their parent classloaders. This is inconsistent. The easiest way to work around this is making sure only one thread is changing the classloader cache during it’s entire run. This would mean either no concurrency or what we have done here, which is letting threads work on a copy of the cache and replace the original cache in the end using an atomic operation. This means the thread that finishes last wins, but for caching that’s fine. Worst case some things aren’t cached in a concurrent execution. This change also means that we don’t need concurrent hashmaps for the classloader cache anymore since no two theads will access the same hashmap. We still need a concurrent hashmap for the class caches inside of the classloaders as multiple threads can access the same classloaders. --- nailgun_launcher/CbtURLClassLoader.java | 26 ++++++++++---------- nailgun_launcher/JavaCache.java | 5 ++-- nailgun_launcher/NailgunLauncher.java | 42 +++++++++++++++++---------------- 3 files changed, 38 insertions(+), 35 deletions(-) (limited to 'nailgun_launcher') diff --git a/nailgun_launcher/CbtURLClassLoader.java b/nailgun_launcher/CbtURLClassLoader.java index e3d597e..43d07f4 100644 --- a/nailgun_launcher/CbtURLClassLoader.java +++ b/nailgun_launcher/CbtURLClassLoader.java @@ -23,19 +23,21 @@ public class CbtURLClassLoader extends java.net.URLClassLoader{ } public Class loadClass(String name, Boolean resolve) throws ClassNotFoundException{ //System.out.println("loadClass("+name+") on \n"+this); - if(!cache.contains(name)) - try{ - cache.put(super.loadClass(name, resolve), name); - } catch (ClassNotFoundException e){ - cache.put(Object.class, name); + synchronized( cache ){ + if(!cache.contains(name)) + try{ + cache.put(super.loadClass(name, resolve), name); + } catch (ClassNotFoundException e){ + cache.put(Object.class, name); + } + Class _class = cache.get(name); + if(_class == Object.class){ + if( name == "java.lang.Object" ) + return Object.class; + else return null; + } else { + return _class; } - Class _class = cache.get(name); - if(_class == Object.class){ - if( name == "java.lang.Object" ) - return Object.class; - else return null; - } else { - return _class; } } void assertExist(URL[] urls){ diff --git a/nailgun_launcher/JavaCache.java b/nailgun_launcher/JavaCache.java index 56730df..3ba12ab 100644 --- a/nailgun_launcher/JavaCache.java +++ b/nailgun_launcher/JavaCache.java @@ -1,15 +1,14 @@ package cbt; import java.util.*; -import java.util.concurrent.ConcurrentHashMap; import static java.io.File.pathSeparator; import static cbt.Stage0Lib.*; final class JavaCache{ - ConcurrentHashMap hashMap; + Map hashMap; public JavaCache( - ConcurrentHashMap hashMap + Map hashMap ){ this.hashMap = hashMap; } diff --git a/nailgun_launcher/NailgunLauncher.java b/nailgun_launcher/NailgunLauncher.java index 0b41888..c397810 100644 --- a/nailgun_launcher/NailgunLauncher.java +++ b/nailgun_launcher/NailgunLauncher.java @@ -4,7 +4,6 @@ import java.lang.reflect.*; import java.net.*; import java.security.*; import java.util.*; -import java.util.concurrent.ConcurrentHashMap; import static cbt.Stage0Lib.*; import static java.io.File.pathSeparator; @@ -15,9 +14,7 @@ import static java.io.File.pathSeparator; */ public class NailgunLauncher{ /** Persistent cache for caching classloaders for the JVM life time. */ - private final static JavaCache classLoaderCache = new JavaCache( - new ConcurrentHashMap() - ); + private static Map classLoaderCacheHashMap = new HashMap(); public final static SecurityManager initialSecurityManager = System.getSecurityManager(); @@ -35,7 +32,7 @@ public class NailgunLauncher{ ((File) get(context, "cbtHome")).toString(), ((File) get(context, "compatibilityTarget")).toString() + "/", new JavaCache( - (ConcurrentHashMap) get(context, "persistentCache") + (HashMap) get(context, "persistentCache") ) ); return @@ -79,28 +76,33 @@ public class NailgunLauncher{ String CBT_HOME = System.getenv("CBT_HOME"); String cache = CBT_HOME + "/cache/"; String compatibilityTarget = CBT_HOME + "/compatibility/" + TARGET; + // copy cache, so that this thread has a consistent view + // replace before returning, see below + JavaCache classLoaderCache = new JavaCache( + new HashMap(classLoaderCacheHashMap) + ); BuildStage1Result res = buildStage1( false, start, cache, CBT_HOME, compatibilityTarget, classLoaderCache ); try{ - System.exit( - (Integer) res - .classLoader - .loadClass("cbt.Stage1") - .getMethod( - "run", - String[].class, File.class, File.class, BuildStage1Result.class, - Long.class, ConcurrentHashMap.class - ) - .invoke( - null, - (Object) args, new File(cache), new File(CBT_HOME), res, - start, classLoaderCache.hashMap - ) - ); + Integer exitCode = (Integer) res + .classLoader + .loadClass("cbt.Stage1") + .getMethod( + "run", String[].class, File.class, File.class, BuildStage1Result.class, Long.class, Map.class + ).invoke( + null, (Object) args, new File(cache), new File(CBT_HOME), res, start, classLoaderCache.hashMap + ); + + System.exit( exitCode ); } catch (java.lang.reflect.InvocationTargetException e) { throw unwrapInvocationTargetException(e); + } finally { + // This replaces the cache and should be thread-safe. + // For competing threads the last one wins with a consistent cache. + // So worst case, we loose some of the cache that's replaced. + classLoaderCacheHashMap = classLoaderCache.hashMap; } } -- cgit v1.2.3