You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@river.apache.org by pe...@apache.org on 2012/07/28 13:25:59 UTC

svn commit: r1366641 - in /river/jtsk/trunk: qa/harness/policy/ qa/src/com/sun/jini/qa/resources/ qa/src/com/sun/jini/test/impl/start/ qa/src/com/sun/jini/test/resources/ qa/src/com/sun/jini/test/spec/jeri/transport/resources/ src/net/jini/loader/pref/...

Author: peter_firmstone
Date: Sat Jul 28 11:25:59 2012
New Revision: 1366641

URL: http://svn.apache.org/viewvc?rev=1366641&view=rev
Log:
River-396 PreferredClassProvider Concurrency improvement.  Re-factored and simplified the weak cache, which was a little difficult to understand due to the relations between weak references and WeakHashMap.

WeakHashMap uses weak keys but what we really wanted was a ConcurrentMap that used weak values, where the value is a child ClassLoader, the key is unique to the ClassLoader so when it becomes weakly reachable the key can be removed too.

using a ConcurrentMap also eliminates any issue with synchronisation and atomic replacement of a key & value combination that has become weakly reachable.  This allows removal of the synchronised block around checking ClassLoader permissions, Concurrency in practise recommends against synchronising on external state.

Tests have been revised to include the classpath and permissions for the new libraries.

Modified:
    river/jtsk/trunk/qa/harness/policy/defaulttest.policy
    river/jtsk/trunk/qa/src/com/sun/jini/qa/resources/qaDefaults.properties
    river/jtsk/trunk/qa/src/com/sun/jini/test/impl/start/ClassLoaderTest.td
    river/jtsk/trunk/qa/src/com/sun/jini/test/impl/start/ClasspathTest.td
    river/jtsk/trunk/qa/src/com/sun/jini/test/resources/jinitest.policy
    river/jtsk/trunk/qa/src/com/sun/jini/test/spec/jeri/transport/resources/ssl.policy
    river/jtsk/trunk/src/net/jini/loader/pref/PreferredClassProvider.java
    river/jtsk/trunk/src/net/jini/security/Security.java
    river/jtsk/trunk/src/org/apache/river/api/security/PrincipalGrant.java

Modified: river/jtsk/trunk/qa/harness/policy/defaulttest.policy
URL: http://svn.apache.org/viewvc/river/jtsk/trunk/qa/harness/policy/defaulttest.policy?rev=1366641&r1=1366640&r2=1366641&view=diff
==============================================================================
--- river/jtsk/trunk/qa/harness/policy/defaulttest.policy (original)
+++ river/jtsk/trunk/qa/harness/policy/defaulttest.policy Sat Jul 28 11:25:59 2012
@@ -33,6 +33,12 @@ grant codebase "file:${com.sun.jini.jsk.
     permission java.security.AllPermission "", "";
 };
 
+grant codebase "file:${com.sun.jini.jsk.home}${/}lib${/}high-scale-lib.jar" {
+    permission java.lang.RuntimePermission "accessDeclaredMembers";
+    permission java.lang.RuntimePermission "accessClassInPackage.sun.misc";
+    permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
+};
+
 grant codebase "file:${com.sun.jini.qa.harness.harnessJar}" {
     permission java.security.AllPermission "", "";
 };

Modified: river/jtsk/trunk/qa/src/com/sun/jini/qa/resources/qaDefaults.properties
URL: http://svn.apache.org/viewvc/river/jtsk/trunk/qa/src/com/sun/jini/qa/resources/qaDefaults.properties?rev=1366641&r1=1366640&r2=1366641&view=diff
==============================================================================
--- river/jtsk/trunk/qa/src/com/sun/jini/qa/resources/qaDefaults.properties (original)
+++ river/jtsk/trunk/qa/src/com/sun/jini/qa/resources/qaDefaults.properties Sat Jul 28 11:25:59 2012
@@ -132,7 +132,7 @@ net.jini.space.JavaSpace.preparername=te
 # For the shared activation group and its SharedGroupImpl
 #
 sharedGroup.type=group
-sharedGroup.classpath=${com.sun.jini.jsk.home}$/lib$/sharedvm.jar$:${com.sun.jini.jsk.home}$/lib$/reference-collections-1.0.1.jar
+sharedGroup.classpath=${com.sun.jini.jsk.home}$/lib$/sharedvm.jar$:${com.sun.jini.jsk.home}$/lib$/reference-collections-1.0.1.jar$:${com.sun.jini.jsk.home}$/lib$/high-scale-lib.jar
 sharedGroup.starterConfiguration=<url:harness/configs/<config>/starter/starter.config>
 sharedGroup.policyfile=<url:harness/policy/defaultsharedvm.policy>
 sharedGroup.implPrefix=sharedGroupImpl
@@ -154,7 +154,7 @@ sharedGroupImpl.preparername=test.groupP
 nonActivatableGroup.type=nonactivatablegroup
 nonActivatableGroup.impl=com.sun.jini.qa.harness.NonActivatableGroupImpl
 nonActivatableGroup.component=nonActivatableGroup
-nonActivatableGroup.classpath=${com.sun.jini.qa.home}$/lib$/nonactivatablegroup.jar$:${com.sun.jini.jsk.home}$/lib$/start.jar$:${com.sun.jini.jsk.home}$/lib$/jsk-platform.jar$:${com.sun.jini.jsk.home}$/lib$/reference-collections-1.0.1.jar
+nonActivatableGroup.classpath=${com.sun.jini.qa.home}$/lib$/nonactivatablegroup.jar$:${com.sun.jini.jsk.home}$/lib$/start.jar$:${com.sun.jini.jsk.home}$/lib$/jsk-platform.jar$:${com.sun.jini.jsk.home}$/lib$/reference-collections-1.0.1.jar$:${com.sun.jini.jsk.home}$/lib$/high-scale-lib.jar
 nonActivatableGroup.codebase=http://${HOST}:${com.sun.jini.qa.port}/nonactivatablegroup-dl.jar
 nonActivatableGroup.policyfile=<url:harness/policy/defaultnonactvm.policy>
 nonActivatableGroup.serverjvmargs=-server,${nonActivatableGroup.serverjvmargs}
@@ -176,7 +176,7 @@ vmKiller.starterConfiguration=-
 #
 activationSystem.type=phoenix
 activationSystem.policyfile=<url:harness/policy/defaultphoenix.policy>
-activationSystem.classpath=${com.sun.jini.jsk.home}$/lib$/phoenix.jar
+activationSystem.classpath=${com.sun.jini.jsk.home}$/lib$/phoenix.jar$:${com.sun.jini.jsk.home}$/lib$/high-scale-lib.jar$:${com.sun.jini.jsk.home}$/lib$/reference-collections-1.0.1.jar
 activationSystem.codebase=http://${HOST}:${com.sun.jini.jsk.port}/phoenix-dl.jar http://<gethost>:${com.sun.jini.jsk.port}/jsk-dl.jar
 activationSystem.serviceConfiguration=<url:harness/configs/<config>/phoenix/phoenix.config>
 activationSystem.starterConfiguration=<url:harness/configs/<config>/starter/starter.config>

Modified: river/jtsk/trunk/qa/src/com/sun/jini/test/impl/start/ClassLoaderTest.td
URL: http://svn.apache.org/viewvc/river/jtsk/trunk/qa/src/com/sun/jini/test/impl/start/ClassLoaderTest.td?rev=1366641&r1=1366640&r2=1366641&view=diff
==============================================================================
--- river/jtsk/trunk/qa/src/com/sun/jini/test/impl/start/ClassLoaderTest.td (original)
+++ river/jtsk/trunk/qa/src/com/sun/jini/test/impl/start/ClassLoaderTest.td Sat Jul 28 11:25:59 2012
@@ -32,4 +32,4 @@ com.sun.jini.test.impl.start.ClassLoader
 com.sun.jini.test.impl.start.ClassLoaderTest2.host=master
 
 include0=start.properties
-sharedGroup.classpath=${com.sun.jini.jsk.home}$/lib$/sharedvm.jar$:${com.sun.jini.jsk.home}$/lib$/reference-collections-1.0.1.jar$:<file:lib/qa1-start-testservice-common.jar>
+sharedGroup.classpath=${com.sun.jini.jsk.home}$/lib$/sharedvm.jar$:${com.sun.jini.jsk.home}$/lib$/high-scale-lib.jar$:${com.sun.jini.jsk.home}$/lib$/reference-collections-1.0.1.jar$:<file:lib/qa1-start-testservice-common.jar>

Modified: river/jtsk/trunk/qa/src/com/sun/jini/test/impl/start/ClasspathTest.td
URL: http://svn.apache.org/viewvc/river/jtsk/trunk/qa/src/com/sun/jini/test/impl/start/ClasspathTest.td?rev=1366641&r1=1366640&r2=1366641&view=diff
==============================================================================
--- river/jtsk/trunk/qa/src/com/sun/jini/test/impl/start/ClasspathTest.td (original)
+++ river/jtsk/trunk/qa/src/com/sun/jini/test/impl/start/ClasspathTest.td Sat Jul 28 11:25:59 2012
@@ -33,7 +33,7 @@ com.sun.jini.test.impl.start.ClasspathTe
 com.sun.jini.test.impl.start.ClasspathTest2.host=master
 
 // Shared group overrides
-sharedGroup.classpath=${com.sun.jini.jsk.home}$/lib$/sharedvm.jar$:${com.sun.jini.jsk.home}$/lib$/reference-collections-1.0.1.jar$:<file:lib/qa1-start-testservice-common.jar>
+sharedGroup.classpath=${com.sun.jini.jsk.home}$/lib$/sharedvm.jar$:${com.sun.jini.jsk.home}$/lib$/high-scale-lib.jar$:${com.sun.jini.jsk.home}$/lib$/reference-collections-1.0.1.jar$:<file:lib/qa1-start-testservice-common.jar>
 sharedGroup.codebase=http://${HOST}:${com.sun.jini.jsk.port}/create-dl.jar http://${HOST}:${com.sun.jini.test.port}/qa1-start-testservice-common-dl.jar
 sharedGroup.policy=<url:harness/policy/all.policy>
 include0=start.properties

Modified: river/jtsk/trunk/qa/src/com/sun/jini/test/resources/jinitest.policy
URL: http://svn.apache.org/viewvc/river/jtsk/trunk/qa/src/com/sun/jini/test/resources/jinitest.policy?rev=1366641&r1=1366640&r2=1366641&view=diff
==============================================================================
--- river/jtsk/trunk/qa/src/com/sun/jini/test/resources/jinitest.policy (original)
+++ river/jtsk/trunk/qa/src/com/sun/jini/test/resources/jinitest.policy Sat Jul 28 11:25:59 2012
@@ -65,6 +65,12 @@ grant codebase "file:${com.sun.jini.qa.h
         "java.security.AllPermission \"\", \"\"";
 };
 
+grant codebase "file:${com.sun.jini.jsk.home}${/}lib${/}high-scale-lib.jar" {
+    permission java.lang.RuntimePermission "accessDeclaredMembers";
+    permission java.lang.RuntimePermission "accessClassInPackage.sun.misc";
+    permission java.lang.reflect.ReflectPermission "suppressAccessChecks";
+};
+
 // grants for various test services, placed here for simplicity, but
 // should probably be separated into separate, targeted policy files
 

Modified: river/jtsk/trunk/qa/src/com/sun/jini/test/spec/jeri/transport/resources/ssl.policy
URL: http://svn.apache.org/viewvc/river/jtsk/trunk/qa/src/com/sun/jini/test/spec/jeri/transport/resources/ssl.policy?rev=1366641&r1=1366640&r2=1366641&view=diff
==============================================================================
--- river/jtsk/trunk/qa/src/com/sun/jini/test/spec/jeri/transport/resources/ssl.policy (original)
+++ river/jtsk/trunk/qa/src/com/sun/jini/test/spec/jeri/transport/resources/ssl.policy Sat Jul 28 11:25:59 2012
@@ -18,6 +18,7 @@ grant codebase "file:${com.sun.jini.qa.h
     permission java.util.PropertyPermission "*", "read,write";
     permission javax.security.auth.AuthPermission "*";
     permission java.lang.RuntimePermission "createSecurityManager";
+    permission java.lang.RuntimePermission "accessClassInPackage.sun.util.logging.resources";
 };
 
 grant codebase "file:${com.sun.jini.qa.harness.testJar}" {

Modified: river/jtsk/trunk/src/net/jini/loader/pref/PreferredClassProvider.java
URL: http://svn.apache.org/viewvc/river/jtsk/trunk/src/net/jini/loader/pref/PreferredClassProvider.java?rev=1366641&r1=1366640&r2=1366641&view=diff
==============================================================================
--- river/jtsk/trunk/src/net/jini/loader/pref/PreferredClassProvider.java (original)
+++ river/jtsk/trunk/src/net/jini/loader/pref/PreferredClassProvider.java Sat Jul 28 11:25:59 2012
@@ -18,13 +18,15 @@
 
 package net.jini.loader.pref;
 
+import au.net.zeus.collection.RC;
+import au.net.zeus.collection.Ref;
+import au.net.zeus.collection.Referrer;
 import com.sun.jini.action.GetPropertyAction;
 import com.sun.jini.logging.Levels;
 import com.sun.jini.logging.LogUtil;
 import java.io.FilePermission;
 import java.io.IOException;
 import java.lang.ref.ReferenceQueue;
-import java.lang.ref.SoftReference;
 import java.lang.ref.WeakReference;
 import java.lang.reflect.Modifier;
 import java.lang.reflect.Proxy;
@@ -45,16 +47,18 @@ import java.security.PrivilegedAction;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Enumeration;
-import java.util.HashMap;
-import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.StringTokenizer;
 import java.util.WeakHashMap;
+import java.util.concurrent.ConcurrentMap;
 import java.util.logging.Logger;
 import java.util.logging.Level;
 import net.jini.loader.ClassAnnotation;
 import net.jini.loader.DownloadPermission;
+import org.apache.river.impl.net.UriString;
+import org.cliffc.high_scale_lib.NonBlockingHashMap;
 
 /**
  * An <code>RMIClassLoader</code> provider that supports preferred
@@ -80,9 +84,9 @@ import net.jini.loader.DownloadPermissio
  * loader that is not an instance of {@link ClassAnnotation} or {@link
  * URLClassLoader}.
  *
- * <h3>Common Terms and Behaviors</h3>
+ * <h3>Common Terms and Behaviours</h3>
  *
- * The following section defines terms and describes behaviors common
+ * The following section defines terms and describes behaviours common
  * to how <code>PreferredClassProvider</code> implements the abstract
  * methods of <code>RMIClassLoaderSpi</code>.  Where applicable, these
  * definitions and descriptions are relative to the instance of
@@ -274,16 +278,35 @@ public class PreferredClassProvider exte
     }
 
     /**
-     * table mapping codebase URL path and context class loader pairs
+     * table mapping codebase URI path and context class loader pairs
      * to class loader instances.  Entries hold class loaders with weak
      * references, so this table does not prevent loaders from being
-     * garbage collected.
+     * garbage collected.  This has been changed to a static table, since
+     * all PreferredClassProvider instances can share the same cache, all
+     * ClassLoaders are unique in the jvm, this prevents accidental duplication.
      */
-    private final Map<LoaderKey,LoaderEntryHolder> loaderTable = new HashMap<LoaderKey,LoaderEntryHolder>();
-
-    /** reference queue for cleared class loader entries */
-    private final ReferenceQueue<ClassLoader> refQueue = new ReferenceQueue<ClassLoader>();
-
+    private final static ConcurrentMap<LoaderKey,ClassLoader> loaderTable;
+    
+    /**
+     * URL cache is time based, we need this to be as fast as possible,
+     * every remote class to be loaded is annotated.  Tuning may be required.
+     */
+    private final static ConcurrentMap<List<URI>,URL[]> urlCache;
+    private final static ConcurrentMap<String,URI[]> uriCache;
+    
+    
+    static {
+        ConcurrentMap<Referrer<List<URI>>,Referrer<URL[]>> intern =
+                new NonBlockingHashMap<Referrer<List<URI>>,Referrer<URL[]>>();
+        urlCache = RC.concurrentMap(intern, Ref.TIME, Ref.STRONG, 10000L, 10000L);
+        ConcurrentMap<Referrer<String>,Referrer<URI[]>> intern1 =
+                new NonBlockingHashMap<Referrer<String>,Referrer<URI[]>>();
+        uriCache = RC.concurrentMap(intern1, Ref.TIME, Ref.STRONG, 1000L, 1000L);
+                ConcurrentMap<Referrer<LoaderKey>,Referrer<ClassLoader>> internal =
+                new NonBlockingHashMap<Referrer<LoaderKey>,Referrer<ClassLoader>>();
+        loaderTable = RC.concurrentMap(internal, Ref.STRONG, Ref.WEAK_IDENTITY, 200L, 200L);
+    }
+    
     /**
      * Creates a new <code>PreferredClassProvider</code>.
      *
@@ -333,6 +356,9 @@ public class PreferredClassProvider exte
 	    sm.checkCreateClassLoader();
 	}
 	this.requireDlPerm = requireDlPerm;
+        ConcurrentMap<Referrer<ClassLoader>,Referrer<PermissionCollection>> inter =
+                new NonBlockingHashMap<Referrer<ClassLoader>,Referrer<PermissionCollection>>();
+        classLoaderPerms = RC.concurrentMap(inter, Ref.WEAK_IDENTITY, Ref.STRONG, 200L, 200L);
 	initialized = true;
     }
 
@@ -346,8 +372,8 @@ public class PreferredClassProvider exte
      * Map to hold permissions needed to check the URLs of
      * URLClassLoader objects.
      */
-    private final Map<ClassLoader,PermissionCollection> classLoaderPerms 
-            = new WeakHashMap<ClassLoader,PermissionCollection>();
+    private final ConcurrentMap<ClassLoader,PermissionCollection> classLoaderPerms ;
+    //        = new WeakHashMap<ClassLoader,PermissionCollection>();
     
     /*
      * Check permissions to load from the specified loader.  The
@@ -371,22 +397,18 @@ public class PreferredClassProvider exte
 		((PreferredClassLoader) loader).checkPermissions();
 		
 	    } else {
-		PermissionCollection perms;
-		
-		synchronized (classLoaderPerms) {
-		    perms = classLoaderPerms.get(loader);
-		    if (perms == null) {
-			perms = new Permissions();
-			PreferredClassLoader.addPermissionsForURLs(
-			    urls, perms, false);
-			classLoaderPerms.put(loader, perms);
-		    }
-		}
-                synchronized (perms){
-                    Enumeration en = perms.elements();
-                    while (en.hasMoreElements()) {
-                        sm.checkPermission((Permission) en.nextElement());
-                    }
+		PermissionCollection perms = classLoaderPerms.get(loader);
+                if (perms == null) {
+                    perms = new Permissions();
+                    // long operation so we don't want to synchronize here.
+                    PreferredClassLoader.addPermissionsForURLs(
+                        urls, perms, false);
+                    perms.setReadOnly();
+                    classLoaderPerms.putIfAbsent(loader, perms);// doesn't matter if they existed.
+                }
+                Enumeration<Permission> en = perms.elements();
+                while (en.hasMoreElements()) {
+                    sm.checkPermission(en.nextElement());
                 }
 	    }
 	}
@@ -1396,8 +1418,8 @@ public class PreferredClassProvider exte
     }
 
     /**
-     * Convert a string containing a space-separated list of URLs into a
-     * corresponding array of URL objects, throwing a MalformedURLException
+     * Convert a string containing a space-separated list of URL Strings into a
+     * corresponding array of URI objects, throwing a MalformedURLException
      * if any of the URLs are invalid.  This method returns null if the
      * specified string is null.
      *
@@ -1410,26 +1432,20 @@ public class PreferredClassProvider exte
 	if (path == null) {
 	    return null;
 	}
-	synchronized (pathToURLsCache) {
-	    Object[] v = (Object[]) pathToURLsCache.get(path);
-	    if (v != null) {
-		return ((URI[])v[0]);
-	    }
-	}
+        URI[] urls = uriCache.get(path);
+        if (urls != null) return urls;
 	StringTokenizer st = new StringTokenizer(path);	// divide by spaces
-	URI[] urls = new URI[st.countTokens()];
+	urls = new URI[st.countTokens()];
 	for (int i = 0; st.hasMoreTokens(); i++) {
             try {
-                urls[i] = new URI(st.nextToken()).normalize();
+                urls[i] = new URI(UriString.parse(st.nextToken())).normalize();
             } catch (URISyntaxException ex) {
                 throw new MalformedURLException("URL's must be RFC 2396 Compliant: " 
                         + ex.getMessage());
             }
 	}
-	synchronized (pathToURLsCache) {
-	    pathToURLsCache.put(path,
-				new Object[] {urls, new SoftReference(path)});
-	}
+        URI [] existed = uriCache.putIfAbsent(path, urls);
+        if (existed != null) urls = existed;
 	return urls;
     }
     
@@ -1437,23 +1453,28 @@ public class PreferredClassProvider exte
      */
     private URL[] asURL(URI[] uris) throws MalformedURLException{
         if (uris == null) return null;
+        List<URI> uriList = Arrays.asList(uris);
+        URL [] result = urlCache.get(uriList);
+        if ( result != null) return result;
         try {
             int l = uris.length;
             URL[] urls = new URL[l];
             for (int i = 0; i < l; i++ ){
-                urls[i] = uris[i] == null ? null : uris[i].toURL(); // throws MalformedURLException
+                try {
+                    urls[i] = uris[i] == null ? null : uris[i].toURL(); // throws MalformedURLException
+                } catch (MalformedURLException e){
+                    System.err.println("MalformedURLException: " + e + "was thrown ," + uris[i]);
+                    throw e;
+                }
             }
+            URL [] existed = urlCache.putIfAbsent(uriList,urls);
+            if (existed != null) urls = existed;
             return urls;
         } catch (IllegalArgumentException ex){
             throw new MalformedURLException(ex.getMessage());
         }
     }
 
-    /** map from weak(key=string) to [URL[], soft(key)] 
-     * A Soft equality based hash map is what's required here.
-     */
-    private static Map pathToURLsCache = new WeakHashMap(5);
-
     /**
      * Return the class loader to be used as the parent for an RMI class
      * loader used in the current execution context.
@@ -1593,105 +1614,59 @@ public class PreferredClassProvider exte
 	 *     return parent;
 	 * }
 	 */
-
-	/*
-	 * Take this opportunity to remove from the table entries
-	 * whose weak references have been cleared.
-	 */
-	List<LoaderKey> toRemove = new LinkedList<LoaderKey>();
-	Object ref;
-	while ((ref = refQueue.poll()) != null) {
-	    if (ref instanceof LoaderKey)
-		toRemove.add((LoaderKey) ref);
-	    else if (ref instanceof LoaderEntry) {
-		LoaderEntry entry = (LoaderEntry) ref;
-		if (!entry.removed)	// ignore entries removed below
-		    toRemove.add(entry.key);
-	    }
-	}
-
-	LoaderKey key = new LoaderKey(uris, parent, refQueue);
-	LoaderEntryHolder holder;
-	synchronized (loaderTable) {
-	    if (!toRemove.isEmpty()) {
-		for (LoaderKey oldKey : toRemove)
-		    loaderTable.remove(oldKey);
-		toRemove.clear();
-	    }
-
-	    /*
-	     * Look up the codebase URL path and parent class loader pair
-	     * in the table of RMI class loaders.
-	     */
-	    holder = loaderTable.get(key);
-	    if (null == holder) {
-		holder = new LoaderEntryHolder();
-		loaderTable.put(key, holder);
-	    }
-	}
+        
+        /* Each LoaderKey is unique to a ClassLoader, the LoaderKey contains
+         * a weak reference to the parent ClassLoader, the parent ClassLoader
+         * will not be collected until all child ClassLoaders have been collected.
+         * 
+         * When a child ClassLoader is collected, the LoaderKey will be removed
+         * within the next 10ms.
+         */
+	LoaderKey key = new LoaderKey(uris, parent, null);
+        ClassLoader loader = loaderTable.get(key);
 
 	/*
 	 * Four possible cases:
 	 *   1) this is our first time creating this classloader
-	 *      - holder.entry is null, need to make a new entry and a new loader
+	 *      - loader is null, need to make a new entry and a new loader
 	 *   2) we made this classloader before, but it was garbage collected a long while ago
-	 *      - identical to case #1 and it was reaped by the toRemove code above
+	 *      - identical to case #1
 	 *   3) we made this classloader before, and it was garbage collected recently
-	 *      - holder.entry is non-null, but holder.entry.get() is null, very similar to case #1
+	 *      - ConcurrentMap.putIfAbsent will replace it, very similar to case #1
 	 *   4) we made this classloader before, and it's still alive (CACHE HIT)
 	 *      - just return it
 	 */
-	synchronized (holder) {
-	    LoaderEntry entry = holder.entry;
-	    ClassLoader loader;
-	    if (entry == null ||
-		(loader = entry.get()) == null)
-	    {
-		/*
-		 * If entry was in table but it's weak reference was cleared,
-		 * remove it from the table and mark it as explicitly cleared,
-		 * so that new matching entry that we put in the table will
-		 * not be erroneously removed when this entry is processed
-		 * from the weak reference queue.
-		 */
-		if (entry != null) {
-		    entry.removed = true;
-		}
-
-		/*
-		 * An existing loader with the given URL path and
-		 * parent was not found.  Perform the following steps
-		 * to obtain an appropriate loader:
-		 * 
-		 * Search for an ancestor of the parent class loader
-		 * whose export urls match the parameter URL path
-		 * 
-		 * If a matching ancestor could not be found, create a
-		 * new class loader instance for the requested
-		 * codebase URL path and parent class loader.  The
-		 * loader instance is created within an access control
-		 * context restricted to the permissions necessary to
-		 * load classes from its codebase URL path.
-		 */
-		loader = findOriginLoader(uris, parent);
-
-		if (loader == null) {
-		    loader = createClassLoader(urls, parent, requireDlPerm);
-                    /* RIVER-265
-                     * The next section of code has been moved inside this
-                     * block to avoid caching loaders found using
-                     * findOriginLoader
-                     * 
-                     * Finally, create an entry to hold the new loader with a
-                     * weak reference and store it in the table with the key.
-                     */
-                    entry = new LoaderEntry(key, loader, refQueue);
-                    holder.entry = entry;
-		}
+        if (loader == null) {
+            /*
+             * An existing loader with the given URL path and
+             * parent was not found.  Perform the following steps
+             * to obtain an appropriate loader:
+             * 
+             * Search for an ancestor of the parent class loader
+             * whose export urls match the parameter URL path
+             * 
+             * If a matching ancestor could not be found, create a
+             * new class loader instance for the requested
+             * codebase URL path and parent class loader.  The
+             * loader instance is created within an access control
+             * context restricted to the permissions necessary to
+             * load classes from its codebase URL path.
+             */
+            loader = findOriginLoader(uris, parent);
+
+            if (loader == null) {
+                loader = createClassLoader(urls, parent, requireDlPerm);
+                /* RIVER-265
+                 * The next section of code has been moved inside this
+                 * block to avoid caching loaders found using
+                 * findOriginLoader
+                 */
+                ClassLoader existed = loaderTable.putIfAbsent(key, loader);
+                if (existed != null) loader = existed;
+            }
 
-	    }
-	    return loader;
-	}
+        }
+        return loader;
     }
 
     /**
@@ -1745,6 +1720,26 @@ public class PreferredClassProvider exte
      * a parent class loader (possibly null).  The weak reference is
      * registered with "refQueue" so that the entry can be removed
      * after the loader has become unreachable.
+     * 
+     * LoaderKey used to be a combination of URL path and weak reference to a
+     * parent class loader.
+     * 
+     * It was updated to utilise URI for the following reasons:
+     * 
+     * 1. Modern environments have dynamically assigned IP addresses, URI can provide a
+     *    level on indirection for Dynamic DNS and Dynamic IP.
+     * 2. Virtual hosting is broken with URL.
+     * 4. Testing revealed that all Jini specification tests pass with URI.  
+     *    Although this doesn't eliminate the possibility of breakage in user code, 
+     *    it does provide a level of confidence that indicates the benefits
+     *    outweigh any disadvantages.  Illegal characters are escaped prior
+     *    to parsing; to maximise compatibility and minimise deployment issues.
+     * 5. Sun bug ID 4434494 states: 
+     *      However, to address URI parsing in general, we introduced a new
+     *      class called URI in Merlin (jdk1.4). People are encouraged to use 
+     *      URI for parsing and URI comparison, and leave URL class for 
+     *      accessing the URI itself, getting at the protocol handler, 
+     *      interacting with the protocol etc.
      **/
     private static class LoaderKey extends WeakReference<ClassLoader> {
 	private final URI[] uris;
@@ -1767,11 +1762,9 @@ public class PreferredClassProvider exte
 	}
 
 	public boolean equals(Object obj) {
-	    if (obj == this) {
-		return true;
-	    } else if (!(obj instanceof LoaderKey)) {
-		return false;
-	    }
+	    if (obj == this) return true;
+	    if (!(obj instanceof LoaderKey)) return false;
+	    if (hashCode() != obj.hashCode()) return false;
 	    LoaderKey other = (LoaderKey) obj;
 	    ClassLoader parent;
 	    return (nullParent ? other.nullParent
@@ -1781,38 +1774,6 @@ public class PreferredClassProvider exte
 	}
     }
 
-    /**
-     * Loader table value: a weak reference to a class loader.  The
-     * weak reference is registered with "refQueue" so that the entry
-     * can be removed after the loader has become unreachable.
-     **/
-    private static class LoaderEntry extends WeakReference<ClassLoader> {
-	public final LoaderKey key;	// for efficient removal
-
-	/**
-	 * set to true if the entry has been removed from the table
-	 * because it has been replaced, so it should not be attempted
-	 * to be removed again
-         * 
-         * REMINDER: 13th Dec 2011 - Peter Firmstone Commented:
-         * removed is mutated while holding LoaderEntryHolder's
-         * lock, once it becomes unreachable, the queue accesses this variable
-         * without synchronisation or volatility, by the time this object
-         * is ready for collection , it is unlikely that it's state will be
-         * modified.  If however at some point we change PreferredClassProvider's
-         * synchronisation strategy we need to revisit this.
-	 */
-	public boolean removed = false;
-
-	public LoaderEntry(LoaderKey key, ClassLoader loader, ReferenceQueue<ClassLoader> refQueue) {
-	    super(loader, refQueue);
-	    this.key = key;
-	}
-    }
-    private static class LoaderEntryHolder {
-	public LoaderEntry entry;
-    }
-
     private static ClassLoader getClassLoader(final Class c) {
 	return AccessController.doPrivileged(
 	    new PrivilegedAction<ClassLoader>() {

Modified: river/jtsk/trunk/src/net/jini/security/Security.java
URL: http://svn.apache.org/viewvc/river/jtsk/trunk/src/net/jini/security/Security.java?rev=1366641&r1=1366640&r2=1366641&view=diff
==============================================================================
--- river/jtsk/trunk/src/net/jini/security/Security.java (original)
+++ river/jtsk/trunk/src/net/jini/security/Security.java Sat Jul 28 11:25:59 2012
@@ -558,6 +558,16 @@ public final class Security {
      * Unlike Subject.doAs, existing ProtectionDomains are not replaced unless
      * they implement SubjectDomain.
      * <p>
+     * Policy grants to Principals only are implied when run as the Subject, 
+     * combinations of Principal, CodeSource URL and Certificates never imply 
+     * this Subjects Principals as it is treated independently of CodeSource 
+     * policy grants, nor do any such grants imply any of the ProtectionDomains
+     * that represent code on the call stack, since these ProtectionDomains are
+     * never replaced with ProtectionDomains containing the Subject Principals.
+     * <p>
+     * The SubjectDomainCombiner used treats CodeSource and Principal grants
+     * as separate concerns.
+     * <p>
      * If a policy provider is installed that recognises SubjectDomain, then
      * Subjects who's principals are mutated are effective immediately.
      * <p>
@@ -577,11 +587,41 @@ public final class Security {
     }
     
     /**
-     * 
-     * @param <T>
-     * @param subject
-     * @param action
-     * @return
+     * Performs work as a particular Subject in the presence of untrusted code,
+     * for distributed systems.
+     * <p>
+     * This method retrieves the current Thread AccessControlContext and
+     * using a SubjectDomainCombiner subclass, prepends a new ProtectionDomain
+     * implementing SubjectDomain, containing the Principals of the Subject, a 
+     * CodeSource with a null URL and null Certificate array, with no
+     * Permission and a null ClassLoader.
+     * <p>
+     * Unlike Subject.doAs, existing ProtectionDomains are not replaced unless
+     * they implement SubjectDomain.
+     * <p>
+     * Policy grants to Principals only are implied when run as the Subject, 
+     * combinations of Principal, CodeSource URL and Certificate grants never imply 
+     * this Subjects Principals as it is treated independently of CodeSource 
+     * policy grants, nor do any such grants imply any of the ProtectionDomains
+     * that represent code on the call stack, since these ProtectionDomains are
+     * never replaced with ProtectionDomains containing the Subject Principals.
+     * <p>
+     * The SubjectDomainCombiner subclass used treats CodeSource and Principal grants
+     * as separate concerns.
+     * <p>
+     * The SubjectDomainCombiner subclass implementation
+     * is package private and can only be accessed through SubjectDomainCombiner
+     * public methods.
+     * <p>
+     * If a policy provider is installed that recognises SubjectDomain, then
+     * Subjects who's principals are mutated are effective immediately.
+     * <p>
+     * No AuthPermission is required to call this method.
+     * <p>
+     * @param subject  The Subject the work will be performed as, may be null.
+     * @param action  The code to be run as the Subject.
+     * @return   The value returned by the PrivilegedAction's run() method.
+     * @throws  NullPointerException if action is null;
      * @throws PrivilegedActionException 
      */
     public static <T> T doAs(final Subject subject,
@@ -592,6 +632,29 @@ public final class Security {
         return AccessController.doPrivileged(action, combine(acc, subject));
     }
     
+    /**
+     * Perform work as a particular Subject in the presence of untrusted code
+     * for distributed systems.
+     * 
+     * This method behaves exactly as Security.doAs, except that instead of
+     * retrieving the current Threads <code>AccessControlContext</code>, 
+     * it uses the provided <code>SecurityContext</code>. If the provided 
+     * <code>SecurityContext</code> is null this method instantiates a new
+     * <code>AccessControlContext</code> with an empty array of ProtectionDomains.
+     * 
+     * Unlike Security.doAs which doesn't require any privileges, this method 
+     * requires the same Permission as Subject.doAsPrivileged to execute.
+     * 
+     * @param subject  The Subject the work will be performed as, may be null.
+     * @param action  The code to be run as the Subject.
+     * @param context  The SecurityContext to be tied to the specific action
+     * and subject.
+     * @return   The value returned by the PrivilegedAction's run() method.
+     * @throws NullPointerException  if the specified PrivilegedExceptionAction 
+     * is null.
+     * @throws SecurityException  if the caller doesn't have permission to call
+     * this method.
+     */
     public static <T> T doAsPrivileged(final Subject subject,
 			final java.security.PrivilegedAction<T> action,
 			final SecurityContext context) {
@@ -602,6 +665,31 @@ public final class Security {
         return AccessController.doPrivileged(act, combine(acc, subject));
     }
     
+     /**
+     * Perform work as a particular Subject in the presence of untrusted code
+     * for distributed systems.
+     * 
+     * This method behaves exactly as Security.doAs, except that instead of
+     * retrieving the current Threads <code>AccessControlContext</code>, 
+     * it uses the provided <code>SecurityContext</code>.  If the provided 
+     * <code>SecurityContext</code> is null this method instantiates a new
+     * <code>AccessControlContext</code> with an empty array of ProtectionDomains.
+     * 
+     * Unlike Security.doAs which doesn't require any privileges, this method 
+     * requires the same Permission as Subject.doAsPrivileged to execute.
+     * 
+     * @param subject  The Subject the work will be performed as, may be null.
+     * @param action  The code to be run as the Subject.
+     * @param context  The SecurityContext to be tied to the specific action
+     * and subject.
+     * @return   The value returned by the PrivilegedAction's run() method.
+     * @throws NullPointerException  if the specified PrivilegedExceptionAction 
+     * is null.
+     * @throws SecurityException  if the caller doesn't have permission to call
+     * this method.
+     * @throws PrivilegedActionException  if the PrivilegedActionException.run
+     * method throws a checked exception.
+     */
     public static <T> T doAsPrivileged(final Subject subject,
 			final java.security.PrivilegedExceptionAction<T> action,
 			final SecurityContext context) throws PrivilegedActionException {

Modified: river/jtsk/trunk/src/org/apache/river/api/security/PrincipalGrant.java
URL: http://svn.apache.org/viewvc/river/jtsk/trunk/src/org/apache/river/api/security/PrincipalGrant.java?rev=1366641&r1=1366640&r2=1366641&view=diff
==============================================================================
--- river/jtsk/trunk/src/org/apache/river/api/security/PrincipalGrant.java (original)
+++ river/jtsk/trunk/src/org/apache/river/api/security/PrincipalGrant.java Sat Jul 28 11:25:59 2012
@@ -25,22 +25,27 @@ import java.net.MalformedURLException;
 import java.net.URI;
 import java.net.URISyntaxException;
 import java.net.URL;
+import java.security.AccessController;
 import java.security.AllPermission;
 import java.security.CodeSigner;
 import java.security.CodeSource;
 import java.security.Permission;
 import java.security.Principal;
+import java.security.PrivilegedAction;
 import java.security.ProtectionDomain;
 import java.security.UnresolvedPermission;
 import java.security.cert.Certificate;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.ConcurrentModificationException;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Set;
 import java.util.TreeSet;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 /**
  *
@@ -173,6 +178,7 @@ class PrincipalGrant extends PermissionG
      * Utility Method, really belongs somewhere else, but CodeSource subclasses use it.
      * @param codeSource
      * @return
+     * @deprecated  will be removed when CodeSource based grants are removed.
      */
     @Deprecated
     CodeSource normalizeCodeSource(CodeSource codeSource) {
@@ -217,10 +223,50 @@ class PrincipalGrant extends PermissionG
 	return implies(hasPrincipals);
     }
     
-    protected Principal[] getPrincipals(ProtectionDomain pd){
+    Principal[] getPrincipals(final ProtectionDomain pd){
         if (pd instanceof SubjectDomain){
-            Set<Principal> pals = ((SubjectDomain) pd).getSubject().getPrincipals();
-            return pals.toArray(new Principal[pals.size()]);
+            final Set<Principal> principals = ((SubjectDomain) pd).getSubject().getPrincipals();
+            // Synchronisation would prevent modification during array creation,
+            // but it also prevents multi read,
+            // lets use an iterator, catch ConcurrentModificationException 
+            // (which should seldom happen) sleep momentarily and try again.
+            Principal[] result = null;
+            Iterator<Principal> it = null;
+            // This minimal synchronization ensures that array size will
+            // be correct if ConcurrentModificationException is not thrown.
+            synchronized (principals){
+                result = new Principal[principals.size()];
+                it = principals.iterator();
+            }
+            boolean retry = true;
+            while (retry){
+                try {
+                    int i = 0;
+                    while (it.hasNext()){
+                        result[i] = it.next();
+                        i++;
+                    }
+                    return result;
+                } catch ( ConcurrentModificationException e){
+                    try {
+                        // sleep for modifications to finish = back off.
+                        Thread.currentThread().sleep(20L);
+                        synchronized(principals){  // try again
+                            result = new Principal[principals.size()];
+                            it = principals.iterator();
+                        }
+                    } catch (InterruptedException ex) {
+                        // ProtectionDomain.getPrincipals() instead.
+                        retry = false;
+                        Thread.currentThread().interrupt(); // restore interrupt.
+                    }
+                } catch ( ArrayIndexOutOfBoundsException e) {
+                    retry = false;
+                    // ProtectionDomain.getPrincipals() instead.
+                    System.err.println("ArrayIndexOutOfBoundsException occured during iteration of Subject Principals");
+                    e.printStackTrace(System.err);
+                }
+            }
         }
         return pd.getPrincipals();
     }