replace flawed concurrent hashmap cache with consistent replacement

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.

14 files changed, 56 insertions, 59 deletions

diff --git a/compatibility/Context.java b/compatibility/Context.java
index 5a0f9c6..a7af740 100644
--- a/compatibility/Context.java
+++ b/compatibility/Context.java
@@ -1,6 +1,6 @@
 package cbt;
 import java.io.*;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.*;
 
 // TODO: try to reduce the number of members
 public abstract class Context{
@@ -11,8 +11,8 @@ public abstract class Context{
   public abstract Long startCompat();
   public abstract Boolean cbtHasChangedCompat();
   public abstract String scalaVersionOrNull(); // needed to propagate scalaVersion to dependendee builds
-  public abstract ConcurrentHashMap<Object,Object> persistentCache();
-  public abstract ConcurrentHashMap<Object,Object> transientCache();
+  public abstract Map<Object,Object> persistentCache();
+  public abstract Map<Object,Object> transientCache();
   public abstract File cache();
   public abstract File cbtHome();
   public abstract File cbtRootHome(); // REMOVE
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<T>{
-  ConcurrentHashMap<Object,Object> hashMap;
+  Map<Object,Object> hashMap;
 
   public JavaCache(
-    ConcurrentHashMap<Object,Object> hashMap
+    Map<Object,Object> 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<ClassLoader> classLoaderCache = new JavaCache<ClassLoader>(
-    new ConcurrentHashMap<Object,Object>()
-  );
+  private static Map<Object,Object> classLoaderCacheHashMap = new HashMap<Object,Object>();
 
   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<ClassLoader>(
-        (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<ClassLoader> classLoaderCache = new JavaCache<ClassLoader>(
+      new HashMap<Object,Object>(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;
     }
   }
 
diff --git a/stage1/ClassLoaderCache.scala b/stage1/ClassLoaderCache.scala
index 2011562..af0970e 100644
--- a/stage1/ClassLoaderCache.scala
+++ b/stage1/ClassLoaderCache.scala
@@ -1,12 +1,12 @@
 package cbt
 
 import java.net._
-import java.util.concurrent.ConcurrentHashMap
+import java.util._
 import collection.JavaConverters._
 
 case class ClassLoaderCache(
   logger: Logger,
-  private[cbt] hashMap: ConcurrentHashMap[AnyRef,AnyRef]
+  private[cbt] hashMap: java.util.Map[AnyRef,AnyRef]
 ){
   val cache = new KeyLockedLazyCache[ClassLoader]( hashMap, Some(logger) )
   override def toString = (
diff --git a/stage1/ContextImplementation.scala b/stage1/ContextImplementation.scala
index 6eb2e53..3b610c0 100644
--- a/stage1/ContextImplementation.scala
+++ b/stage1/ContextImplementation.scala
@@ -1,6 +1,5 @@
 package cbt
 import java.io._
-import java.util.concurrent.ConcurrentHashMap
 import java.lang._
 
 case class ContextImplementation(
@@ -11,8 +10,8 @@ case class ContextImplementation(
   startCompat: Long,
   cbtHasChangedCompat: Boolean,
   scalaVersionOrNull: String,
-  persistentCache: ConcurrentHashMap[AnyRef,AnyRef],
-  transientCache: ConcurrentHashMap[AnyRef,AnyRef],
+  persistentCache: java.util.Map[AnyRef,AnyRef],
+  transientCache: java.util.Map[AnyRef,AnyRef],
   cache: File,
   cbtHome: File,
   cbtRootHome: File,
diff --git a/stage1/KeyLockedLazyCache.scala b/stage1/KeyLockedLazyCache.scala
index 2602523..2047b81 100644
--- a/stage1/KeyLockedLazyCache.scala
+++ b/stage1/KeyLockedLazyCache.scala
@@ -1,7 +1,5 @@
 package cbt
 
-import java.util.concurrent.ConcurrentHashMap
-
 private[cbt] class LockableKey
 /**
 A hashMap that lazily computes values if needed during lookup.
@@ -9,7 +7,7 @@ Locking occurs on the key, so separate keys can be looked up
 simultaneously without a deadlock.
 */
 final private[cbt] class KeyLockedLazyCache[T <: AnyRef](
-  val hashMap: ConcurrentHashMap[AnyRef,AnyRef],
+  val hashMap: java.util.Map[AnyRef,AnyRef],
   logger: Option[Logger]
 ){
   def get( key: AnyRef, value: => T ): T = {
diff --git a/stage1/PoorMansProfiler.scala b/stage1/PoorMansProfiler.scala
index b7aa47d..4bc44ba 100644
--- a/stage1/PoorMansProfiler.scala
+++ b/stage1/PoorMansProfiler.scala
@@ -1,10 +1,10 @@
 /*
 // temporary debugging tool
 package cbt
-import java.util.concurrent.ConcurrentHashMap
+import java.util._
 import collection.JavaConversions._
 object PoorMansProfiler{
-  val entries = new ConcurrentHashMap[String, Long]
+  val entries = new HashMap[String, Long]
   def profile[T](name: String)(code: => T): T = {
     val before = System.currentTimeMillis
     if(!(entries containsKey name)){
diff --git a/stage1/Stage1.scala b/stage1/Stage1.scala
index cd46d6b..2f8f960 100644
--- a/stage1/Stage1.scala
+++ b/stage1/Stage1.scala
@@ -1,7 +1,7 @@
 package cbt
 
 import java.io._
-import java.util.concurrent.ConcurrentHashMap
+import java.util._
 
 import scala.collection.JavaConverters._
 
@@ -159,7 +159,7 @@ object Stage1{
     cbtHome: File,
     buildStage1: BuildStage1Result,
     start: java.lang.Long,
-    persistentCache: ConcurrentHashMap[AnyRef,AnyRef]
+    persistentCache: java.util.Map[AnyRef,AnyRef]
   ): Int = {
     val args = Stage1ArgsParser(_args.toVector)
     val logger = new Logger(args.enabledLoggers, start)
diff --git a/stage1/Stage1Lib.scala b/stage1/Stage1Lib.scala
index 8fdde54..9b48409 100644
--- a/stage1/Stage1Lib.scala
+++ b/stage1/Stage1Lib.scala
@@ -9,7 +9,6 @@ import java.nio.file.attribute.FileTime
 import javax.tools._
 import java.security._
 import java.util.{Set=>_,Map=>_,List=>_,_}
-import java.util.concurrent.ConcurrentHashMap
 import javax.xml.bind.annotation.adapters.HexBinaryAdapter
 
 // CLI interop
diff --git a/stage1/cbt.scala b/stage1/cbt.scala
index 985f619..22242d7 100644
--- a/stage1/cbt.scala
+++ b/stage1/cbt.scala
@@ -2,7 +2,6 @@ package cbt
 import java.io._
 import java.nio.file._
 import java.net._
-import java.util.concurrent.ConcurrentHashMap
 
 object `package`{
   implicit class TypeInferenceSafeEquals[T](value: T){
diff --git a/stage1/resolver.scala b/stage1/resolver.scala
index ff5ad68..1f94c7f 100644
--- a/stage1/resolver.scala
+++ b/stage1/resolver.scala
@@ -4,8 +4,6 @@ import java.nio.charset.StandardCharsets
 import java.net._
 import java.io._
 import scala.xml._
-import scala.concurrent._
-import scala.concurrent.duration._
 
 trait DependencyImplementation extends Dependency{
   implicit protected def logger: Logger
diff --git a/stage2/Stage2.scala b/stage2/Stage2.scala
index 260a46d..ab7b4fe 100644
--- a/stage2/Stage2.scala
+++ b/stage2/Stage2.scala
@@ -1,5 +1,6 @@
 package cbt
 import java.io._
+import java.util._
 
 object Stage2 extends Stage2Base{
   def getBuild(context: Context) = {
@@ -22,7 +23,7 @@ object Stage2 extends Stage2Base{
       0
     }
     val task = args.args.lift( taskIndex )
-    
+
     val context: Context = ContextImplementation(
       args.cwd,
       args.cwd,
@@ -32,7 +33,7 @@ object Stage2 extends Stage2Base{
       args.cbtHasChanged,
       null,
       args.persistentCache,
-      new java.util.concurrent.ConcurrentHashMap,
+      new HashMap,
       args.cache,
       args.cbtHome,
       args.cbtHome,
diff --git a/test/test.scala b/test/test.scala
index 56ad3b1..ae6c301 100644
--- a/test/test.scala
+++ b/test/test.scala
@@ -1,9 +1,9 @@
 package cbt
 package test
-import java.util.concurrent.ConcurrentHashMap
 import java.io.File
 import java.nio.file._
 import java.net.URL
+import java.util.{Iterator=>_,_}
 import scala.concurrent._
 import scala.concurrent.duration._
 // micro framework
@@ -115,8 +115,8 @@ object Main{
         start,
         cbtHasChanged,
         null,
-        new ConcurrentHashMap[AnyRef,AnyRef],
-        new java.util.concurrent.ConcurrentHashMap[AnyRef,AnyRef],
+        new HashMap[AnyRef,AnyRef],
+        new HashMap[AnyRef,AnyRef],
         cache,
         cbtHome,
         cbtHome,