Please review patch: Making ScriptMaker & friends thread-safe

diff --git a/src/jsrc/org/erights/e/elib/prim/SafeJ.java b/src/jsrc/org/erights/e/elib/prim/SafeJ.java
index c77f4e2..cc2aeb3 100644
--- a/src/jsrc/org/erights/e/elib/prim/SafeJ.java
+++ b/src/jsrc/org/erights/e/elib/prim/SafeJ.java
@@ -5,6 +5,7 @@ package org.erights.e.elib.prim;
 
 import java.io.IOException;
 import java.net.URL;
+import java.util.concurrent.ConcurrentHashMap;
 
 import org.erights.e.develop.assertion.T;
 import org.erights.e.develop.exception.ExceptionMgr;
@@ -14,7 +15,6 @@ import org.erights.e.elib.base.ValueThunk;
 import org.erights.e.elib.tables.ConstList;
 import org.erights.e.elib.tables.ConstMap;
 import org.erights.e.elib.tables.FlexMap;
-import org.erights.e.elib.tables.FlexSet;
 import org.erights.e.elib.tables.IdentityCacheTable;
 import org.erights.e.elib.tables.Twine;
 import org.erights.e.elib.util.ClassCache;
@@ -257,13 +257,20 @@ public final class SafeJ {
       "org.quasiliteral.text.Identifiers",
       "org.quasiliteral.text.Substituter",};
 
-    static private final FlexSet ApprovedClasses;
+    /**
+     * The names of all classes which are unconditionally approved (not those
+     * which have safej files) are keys of this map. Note that this is mutated
+     * by approve() and so it is not a ConstSet and must be thread-safe. Its
+     * elements are either in the ApprovedClassList above, or they are array
+     * types.
+     */
+    static private final ConcurrentHashMap ApprovedClasses;
 
     static {
-        int len = ApprovedClassList.length;
-        ApprovedClasses = FlexSet.fromType(String.class, len);
+        final int len = ApprovedClassList.length;
+        ApprovedClasses = new ConcurrentHashMap();
         for (int i = 0; i < len; i++) {
-            ApprovedClasses.addElement(ApprovedClassList[i], true);
+            ApprovedClasses.put(ApprovedClassList[i], ApprovedClassList[i]);
         }
     }
 
@@ -371,31 +378,40 @@ public final class SafeJ {
 //    }
 
     /**
-     *
+     * Given a class FQN, return the Term parse of the corresponding .safej 
+     * file.
      */
     static public Term getOptSafeJTerm(String fqName) {
         Twine tfqn = Twine.fromString(fqName);
-        Term optResult = (Term)SAFEJ_CACHE.fetch(tfqn, ValueThunk.NULL_THUNK);
-        if (null != optResult) {
-            return optResult;
-        }
-        String path = StringHelper.replaceAll(fqName, ".", "/") + ".safej";
-        URL optTermURL = ClassLoader.getSystemResource(path);
-        if (null == optTermURL) {
-            return null;
-        }
-        String termSrc;
-        try {
-            termSrc = URLSugar.getText(optTermURL);
-        } catch (IOException ioe) {
-            throw ExceptionMgr.asSafe(ioe);
-        }
+        
+        // NOTE: This is potentially called from any thread. We assume that the
+        // high-frequency accesses are handled by the ScriptMaker's concurrent
+        // cache, and so we can afford to have mutual exclusion in this layer
+        // of the system.
+        synchronized (SAFEJ_CACHE) {
+            Term optResult =
+                (Term)SAFEJ_CACHE.fetch(tfqn, ValueThunk.NULL_THUNK);
+            if (null != optResult) {
+                return optResult;
+            }
+            String path = StringHelper.replaceAll(fqName, ".", "/") + ".safej";
+            URL optTermURL = ClassLoader.getSystemResource(path);
+            if (null == optTermURL) {
+                return null;
+            }
+            String termSrc;
+            try {
+                termSrc = URLSugar.getText(optTermURL);
+            } catch (IOException ioe) {
+                throw ExceptionMgr.asSafe(ioe);
+            }
 // XXX Bug: Investigate why the commented out version doesn't work.
-//        Term result = (Term)TermParser.run(Twine.fromString(termSrc),
-//                                           GetSafeJQBuilder());
-        Term result = TermParser.run(Twine.fromString(termSrc));
-        SAFEJ_CACHE.put(tfqn, result);
-        return result;
+//          Term result = (Term)TermParser.run(Twine.fromString(termSrc),
+//                                             GetSafeJQBuilder());
+            Term result = TermParser.run(Twine.fromString(termSrc));
+            SAFEJ_CACHE.put(tfqn, result);
+            return result;
+        }
     }
 
     /**
@@ -411,12 +427,12 @@ public final class SafeJ {
      */
     static public boolean approve(Class clazz, boolean safe) {
         String fqName = clazz.getName();
-        if (ApprovedClasses.contains(fqName)) {
+        if (ApprovedClasses.containsKey(fqName)) {
             return true;
         }
         if (clazz.isArray()) {
             //Array types are safe
-            ApprovedClasses.addElement(fqName, true);
+            ApprovedClasses.put(fqName, fqName);
             return true;
         }
         Term optTerm = getOptSafeJTerm(fqName);
diff --git a/src/jsrc/org/erights/e/elib/prim/ScriptMaker.java b/src/jsrc/org/erights/e/elib/prim/ScriptMaker.java
index 8d55134..cfa7511 100644
--- a/src/jsrc/org/erights/e/elib/prim/ScriptMaker.java
+++ b/src/jsrc/org/erights/e/elib/prim/ScriptMaker.java
@@ -29,15 +29,12 @@ import org.erights.e.elib.tables.FlexMap;
 import org.erights.e.elib.util.ClassCache;
 
 import java.util.HashMap;
+import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * From a Java class, obtain a corresponding Script enabling its tamed behavior
  * to be invoked from ELib.
- * <p/>
- * XXX BUG: mutable static state. Fortunately, it's a semantics free cache, so
- * it doesn't actually violate capability semantics. Unfortunately, it needs to
- * be synchronized, and isn't yet.
- *
+ * 
  * @author Mark S. Miller
  */
 public class ScriptMaker {
@@ -62,11 +59,16 @@ public class ScriptMaker {
       {"java.lang.String", "org.erights.e.elib.tables.Twine"},};
 
     /**
-     * Maps fq class names to the fqName of the classes they promote to. <p>
-     * <p/>
+     * Maps fq class names to the fqName of the classes they promote to.
+     * 
+     * <p>
      * ThePromotions is initialized lazily in order to avoid possible circular
      * static initialization dependencies. Uses legacy HashMap rather than EMap
      * in order to avoid a circular dependency by way of the Equalizer.
+     * <p>
+     * Thread safety note: Reads of this HashMap are done without any
+     * synchronization, but this is safe since nothing ever mutates the map
+     * except before it is assigned to ThePromotions.
      */
     static private HashMap ThePromotions = null;
 
@@ -170,6 +172,8 @@ public class ScriptMaker {
      */
     static public Class OptSugar(Class clazz) {
         if (null == TheSugars) {
+            // thread safe because reference assignment is atomic; the
+            // worst that will happen is the cache gets cleared
             TheSugars = FlexMap.fromPairs(Sugarings, true).snapshot();
         }
         String sugarName =
@@ -180,8 +184,8 @@ public class ScriptMaker {
         try {
             return ClassCache.forName(sugarName);
         } catch (Exception ex) {
-            throw new EBacktraceException(ex,
-                                      "# sweetener not found: " + sugarName);
+            throw new EBacktraceException(ex, "# sweetener not found: "
+                                              + sugarName);
         }
     }
 
@@ -190,17 +194,22 @@ public class ScriptMaker {
      */
     static public final ScriptMaker THE_ONE = new ScriptMaker();
 
-
     /**
      * maps java classes to scripts
+     * 
+     * <p>
+     * NOTE: This is shared among threads, thus it must be a thread-safe map.
+     * However, the identity of Scripts is not significant, so our lookup-or-
+     * create need not be careful to ensure that each class's script is only
+     * created once.
      */
-    private final FlexMap myScripts;
+    private final ConcurrentHashMap myScripts;
 
     /**
      *
      */
     private ScriptMaker() {
-        myScripts = FlexMap.fromTypes(Class.class, Script.class);
+        myScripts = new ConcurrentHashMap/*<Class, Script>*/();
 
         //preload with special cases
 
@@ -235,13 +244,13 @@ public class ScriptMaker {
      *
      */
     public Script instanceScript(Class clazz) {
-        Script result = (Script)myScripts.fetch(clazz, ValueThunk.NULL_THUNK);
+        Script result = (Script)myScripts.get(clazz);
         if (null != result) {
             return result;
         }
 
         if (Callable.class.isAssignableFrom(clazz)) {
-            myScripts.put(clazz, CallableScript.THE_ONE);
+            myScripts.putIfAbsent(clazz, CallableScript.THE_ONE);
             return CallableScript.THE_ONE;
         }
 
@@ -252,7 +261,7 @@ public class ScriptMaker {
         Class optPromotion = OptPromotion(clazz);
         if (null != optPromotion) {
             inherit(vTable, optPromotion, SafeJ.ALL);
-            myScripts.put(clazz, vTable);
+            myScripts.putIfAbsent(clazz, vTable);
             return vTable;
         }
 
@@ -280,7 +289,7 @@ public class ScriptMaker {
                 SugarMethodNode.defineMembers(vTable, optSugar);
             }
         }
-        myScripts.put(clazz, vTable);
+        myScripts.putIfAbsent(clazz, vTable);
         return vTable;
     }
 }
diff --git a/src/jsrc/org/erights/e/elib/prim/StaticMaker.java b/src/jsrc/org/erights/e/elib/prim/StaticMaker.java
index 43eaaf8..115cf07 100644
--- a/src/jsrc/org/erights/e/elib/prim/StaticMaker.java
+++ b/src/jsrc/org/erights/e/elib/prim/StaticMaker.java
@@ -34,11 +34,9 @@ import org.erights.e.elib.serial.JOSSPassByConstruction;
 import org.erights.e.elib.serial.Persistent;
 import org.erights.e.elib.slot.Guard;
 import org.erights.e.elib.tables.ConstList;
-import org.erights.e.elib.tables.EMap;
 import org.erights.e.elib.tables.FlexList;
 import org.erights.e.elib.tables.FlexMap;
 import org.erights.e.elib.util.AlreadyDefinedException;
-import org.erights.e.elib.util.ClassCache;
 
 import java.io.IOException;
 import java.lang.reflect.Modifier;
diff --git a/src/jsrc/org/erights/e/elib/util/ClassCache.java b/src/jsrc/org/erights/e/elib/util/ClassCache.java
index 9fbac33..c192a08 100644
--- a/src/jsrc/org/erights/e/elib/util/ClassCache.java
+++ b/src/jsrc/org/erights/e/elib/util/ClassCache.java
@@ -19,7 +19,7 @@ Copyright (C) 1998 Electric Communities. All Rights Reserved.
 Contributor(s): ______________________________________.
 */
 
-import java.util.Hashtable;
+import java.util.concurrent.ConcurrentHashMap;
 
 /**
  * A performance optimizing hack: by hanging onto classes that are looked up by
@@ -36,11 +36,11 @@ import java.util.Hashtable;
 public class ClassCache {
 
     /**
-     * Using java.util.Hashtables instead of ELib's tables in order to avoid
+     * Using java.util.concurrent instead of ELib's tables in order to avoid
      * circular dependencies, and in order to get the thread-safety necessary
      * for static used globally shared across a JVM.
      */
-    static private final Hashtable OurCache = new Hashtable();
+    static private final ConcurrentHashMap OurCache = new ConcurrentHashMap();
 
     static {
         OurCache.put("boolean", Boolean.TYPE);
@@ -71,7 +71,7 @@ public class ClassCache {
         Class result = (Class)OurCache.get(name);
         if (result == null) {
             result = Class.forName(name);
-            OurCache.put(name, result);
+            OurCache.putIfAbsent(name, result);
         }
         return result;
     }