river-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From peter_firmst...@apache.org
Subject svn commit: r1213641 - in /river/jtsk/trunk/src: com/sun/jini/action/GetPropertyAction.java net/jini/loader/pref/PreferredClassProvider.java
Date Tue, 13 Dec 2011 10:36:44 GMT
Author: peter_firmstone
Date: Tue Dec 13 10:36:43 2011
New Revision: 1213641

URL: http://svn.apache.org/viewvc?rev=1213641&view=rev
Log:
River-265 Fix for unlucky caching as requested.
River-401 Changed to utilise URI in place of URL in map's and arrays to avoid unnecessary
DNS lookups.

Modified:
    river/jtsk/trunk/src/com/sun/jini/action/GetPropertyAction.java
    river/jtsk/trunk/src/net/jini/loader/pref/PreferredClassProvider.java

Modified: river/jtsk/trunk/src/com/sun/jini/action/GetPropertyAction.java
URL: http://svn.apache.org/viewvc/river/jtsk/trunk/src/com/sun/jini/action/GetPropertyAction.java?rev=1213641&r1=1213640&r2=1213641&view=diff
==============================================================================
--- river/jtsk/trunk/src/com/sun/jini/action/GetPropertyAction.java (original)
+++ river/jtsk/trunk/src/com/sun/jini/action/GetPropertyAction.java Tue Dec 13 10:36:43 2011
@@ -55,7 +55,7 @@ import net.jini.security.Security;
  * @see		Security
  * @since 2.0
  **/
-public class GetPropertyAction implements PrivilegedAction {
+public class GetPropertyAction implements PrivilegedAction<String> {
 
     private static final Logger logger =
 	Logger.getLogger("com.sun.jini.action.GetPropertyAction");
@@ -98,7 +98,7 @@ public class GetPropertyAction implement
      * @return	the string value of the system property or the default
      * value, or <code>null</code>
      **/
-    public Object run() {
+    public String run() {
 	try {
 	    String value = System.getProperty(theProp);
 	    if (value != null) {

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=1213641&r1=1213640&r2=1213641&view=diff
==============================================================================
--- river/jtsk/trunk/src/net/jini/loader/pref/PreferredClassProvider.java (original)
+++ river/jtsk/trunk/src/net/jini/loader/pref/PreferredClassProvider.java Tue Dec 13 10:36:43
2011
@@ -29,6 +29,8 @@ import java.lang.ref.WeakReference;
 import java.lang.reflect.Modifier;
 import java.lang.reflect.Proxy;
 import java.net.MalformedURLException;
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.net.URL;
 import java.net.URLConnection;
 import java.net.URLClassLoader;
@@ -37,6 +39,7 @@ import java.rmi.server.RMIClassLoaderSpi
 import java.security.AccessController;
 import java.security.CodeSource;
 import java.security.Permission;
+import java.security.PermissionCollection;
 import java.security.Permissions;
 import java.security.PrivilegedAction;
 import java.util.Arrays;
@@ -246,8 +249,8 @@ public class PreferredClassProvider exte
      */
     private static String codebaseProperty = null;
     static {
-	String prop = (String) AccessController.doPrivileged(
-            new GetPropertyAction("java.rmi.server.codebase"));
+	String prop = AccessController.doPrivileged(
+   new GetPropertyAction("java.rmi.server.codebase"));
 	if (prop != null && prop.trim().length() > 0) {
 	    codebaseProperty = prop;
 	}
@@ -279,7 +282,7 @@ public class PreferredClassProvider exte
     private final Map<LoaderKey,LoaderEntryHolder> loaderTable = new HashMap<LoaderKey,LoaderEntryHolder>();
 
     /** reference queue for cleared class loader entries */
-    private final ReferenceQueue refQueue = new ReferenceQueue();
+    private final ReferenceQueue<ClassLoader> refQueue = new ReferenceQueue<ClassLoader>();
 
     /**
      * Creates a new <code>PreferredClassProvider</code>.
@@ -343,7 +346,8 @@ public class PreferredClassProvider exte
      * Map to hold permissions needed to check the URLs of
      * URLClassLoader objects.
      */
-    private final Map classLoaderPerms = new WeakHashMap();
+    private final Map<ClassLoader,PermissionCollection> classLoaderPerms 
+            = new WeakHashMap<ClassLoader,PermissionCollection>();
     
     /*
      * Check permissions to load from the specified loader.  The
@@ -355,22 +359,22 @@ public class PreferredClassProvider exte
      * no permissions are checked, because the caller could have used
      * an empty codebase to achieve the same effect anyway.
      */
-    private void checkLoader(ClassLoader loader, ClassLoader parent,
+    private void checkLoader(ClassLoader loader, ClassLoader parent, URI[] uris,
 			     URL[] urls)
     {
 	SecurityManager sm = System.getSecurityManager();
 
 	if ((sm != null) && (loader != null) && (loader != parent)) {
-	    assert urlsMatchLoaderAnnotation(urls, loader);
+	    assert urlsMatchLoaderAnnotation(uris, loader);
 
 	    if (loader.getClass() == PreferredClassLoader.class) {
 		((PreferredClassLoader) loader).checkPermissions();
 		
 	    } else {
-		Permissions perms;
+		PermissionCollection perms;
 		
 		synchronized (classLoaderPerms) {
-		    perms = (Permissions) classLoaderPerms.get(loader);
+		    perms = classLoaderPerms.get(loader);
 		    if (perms == null) {
 			perms = new Permissions();
 			PreferredClassLoader.addPermissionsForURLs(
@@ -378,11 +382,12 @@ public class PreferredClassProvider exte
 			classLoaderPerms.put(loader, perms);
 		    }
 		}
-
-		Enumeration en = perms.elements();
-		while (en.hasMoreElements()) {
-		    sm.checkPermission((Permission) en.nextElement());
-		}
+                synchronized (perms){
+                    Enumeration en = perms.elements();
+                    while (en.hasMoreElements()) {
+                        sm.checkPermission((Permission) en.nextElement());
+                    }
+                }
 	    }
 	}
     }
@@ -482,8 +487,13 @@ public class PreferredClassProvider exte
 		       });
 	}
 
-    	URL[] codebaseURLs = pathToURLs(codebase);	// may be null
+    	URI[] codebaseURIs = pathToURIs(codebase);	// may be null
 					// throws MalformedURLException
+        int l = codebaseURIs.length;
+        URL[] codebaseURLs = new URL[l];
+        for (int i = 0; i < l; i++ ){
+            codebaseURLs[i] = asURL(codebaseURIs[i]); // throws MalformedURLException
+        }
 
 	/*
 	 * Process array class names.
@@ -510,8 +520,8 @@ public class PreferredClassProvider exte
 	 */
 	SecurityManager sm = System.getSecurityManager();
 	if (defaultLoader != null &&
-	    (sm == null || codebaseURLs == null ||
-	     urlsMatchLoaderAnnotation(codebaseURLs, defaultLoader)))
+	    (sm == null || codebaseURIs == null ||
+	     urlsMatchLoaderAnnotation(codebaseURIs, defaultLoader)))
 	{
 	    try {
 		Class c = Class.forName(name, false, defaultLoader);
@@ -534,7 +544,9 @@ public class PreferredClassProvider exte
 	    logger.log(Level.FINEST,
 		       "(thread context class loader: {0})", contextLoader);
 	}
-	ClassLoader codebaseLoader = lookupLoader(codebaseURLs, contextLoader);
+	ClassLoader codebaseLoader;
+        codebaseLoader = lookupLoader(codebaseURIs, codebaseURLs, contextLoader);
+        
 
 	/*
 	 * Try remaining defaultLoader cases that don't require
@@ -562,7 +574,7 @@ public class PreferredClassProvider exte
 	SecurityException secEx = null;
 	if (sm != null) {
 	    try {
-		checkLoader(codebaseLoader, contextLoader, codebaseURLs);
+		checkLoader(codebaseLoader, contextLoader, codebaseURIs, codebaseURLs);
 	    } catch (SecurityException e) {
 		secEx = e;
 	    }
@@ -887,8 +899,13 @@ public class PreferredClassProvider exte
 	throws MalformedURLException
     {
 	checkInitialized();
-    	URL[] codebaseURLs = pathToURLs(codebase);	// may be null
+    	URI[] codebaseURIs = pathToURIs(codebase);	// may be null
 					// throws MalformedURLException
+        int l = codebaseURIs.length;
+        URL[] codebaseURLs = new URL[l];
+        for (int i = 0; i < l; i++ ){
+            codebaseURLs[i] = asURL(codebaseURIs[i]); // throws MalformedURLException
+        }
 
 	ClassLoader contextLoader = getRMIContextClassLoader();
 	SecurityManager sm = System.getSecurityManager();
@@ -898,8 +915,9 @@ public class PreferredClassProvider exte
 	    return contextLoader;
 	}
 
-	ClassLoader codebaseLoader = lookupLoader(codebaseURLs, contextLoader);
-	checkLoader(codebaseLoader, contextLoader, codebaseURLs);
+	ClassLoader codebaseLoader;
+        codebaseLoader = lookupLoader(codebaseURIs, codebaseURLs, contextLoader);
+	checkLoader(codebaseLoader, contextLoader, codebaseURIs, codebaseURLs);
 	return codebaseLoader;
     }
 
@@ -1059,9 +1077,13 @@ public class PreferredClassProvider exte
 		    defaultLoader
 		});
 	}
-
-    	URL[] codebaseURLs = pathToURLs(codebase);	// may be null
-					// throws MalformedURLException
+        // throws MalformedURLException containing URISyntaxException message
+    	URI[] codebaseURIs = pathToURIs(codebase);	// may be null
+        int l = codebaseURIs.length;
+        URL[] codebaseURLs = new URL[l];
+        for (int i = 0; i < l; i++ ){
+            codebaseURLs[i] = asURL(codebaseURIs[i]); // throws MalformedURLException
+        }
 
 	/*
 	 * Determine the codebase loader.
@@ -1071,7 +1093,8 @@ public class PreferredClassProvider exte
 	    logger.log(Level.FINEST,
 		       "(thread context class loader: {0})", contextLoader);
 	}
-	ClassLoader codebaseLoader = lookupLoader(codebaseURLs, contextLoader);
+	ClassLoader codebaseLoader;
+        codebaseLoader = lookupLoader(codebaseURIs, codebaseURLs, contextLoader);
 
 	/*
 	 * Check permission to access the codebase loader.
@@ -1080,7 +1103,7 @@ public class PreferredClassProvider exte
 	SecurityException secEx = null;
 	if (sm != null) {
 	    try {
-		checkLoader(codebaseLoader, contextLoader, codebaseURLs);
+		checkLoader(codebaseLoader, contextLoader, codebaseURIs, codebaseURLs);
 	    } catch (SecurityException e) {
 		secEx = e;
 	    }
@@ -1093,10 +1116,10 @@ public class PreferredClassProvider exte
 	    boolean codebaseMatchesDL = false;
 	    boolean tryDL =
 		sm == null || secEx != null ||
-		codebaseURLs == null;
+		codebaseURIs == null;
 	    if (!tryDL) {
 		codebaseMatchesDL =
-		    urlsMatchLoaderAnnotation(codebaseURLs, defaultLoader);
+		    urlsMatchLoaderAnnotation(codebaseURIs, defaultLoader);
 		tryDL = codebaseMatchesDL ||
 		    !(codebaseLoader instanceof PreferredClassLoader) ||
 		    !interfacePreferred((PreferredClassLoader) codebaseLoader,
@@ -1316,19 +1339,19 @@ public class PreferredClassProvider exte
      * for the specified class loader, or null if the annotation
      * string is null.
      **/
-    private URL[] getLoaderAnnotationURLs(ClassLoader loader)
+    private URI[] getLoaderAnnotationURIs(ClassLoader loader)
 	throws MalformedURLException
     {
-	return pathToURLs(getLoaderAnnotation(loader, false));
+	return pathToURIs(getLoaderAnnotation(loader, false));
     }
 
     /**
      * Returns true if the specified path of URLs is equal to the
      * annotation URLs of the specified loader, and false otherwise.
      **/
-    private boolean urlsMatchLoaderAnnotation(URL[] urls, ClassLoader loader) {
+    private boolean urlsMatchLoaderAnnotation(URI[] urls, ClassLoader loader) {
 	try {
-	    return Arrays.equals(urls, getLoaderAnnotationURLs(loader));
+	    return Arrays.equals(urls, getLoaderAnnotationURIs(loader));
 	} catch (MalformedURLException e) {
 	    return false;
 	}
@@ -1395,20 +1418,25 @@ public class PreferredClassProvider exte
      * @throws MalformedURLException if the string path of urls contains a
      *         mal-formed url which can not be converted into a url object.
      */
-    private static URL[] pathToURLs(String path) throws MalformedURLException {
+    private static URI[] pathToURIs(String path) throws MalformedURLException {
 	if (path == null) {
 	    return null;
 	}
 	synchronized (pathToURLsCache) {
 	    Object[] v = (Object[]) pathToURLsCache.get(path);
 	    if (v != null) {
-		return ((URL[])v[0]);
+		return ((URI[])v[0]);
 	    }
 	}
 	StringTokenizer st = new StringTokenizer(path);	// divide by spaces
-	URL[] urls = new URL[st.countTokens()];
+	URI[] urls = new URI[st.countTokens()];
 	for (int i = 0; st.hasMoreTokens(); i++) {
-	    urls[i] = new URL(st.nextToken());
+            try {
+                urls[i] = new URI(st.nextToken()).normalize();
+            } catch (URISyntaxException ex) {
+                throw new MalformedURLException("URL's must be RFC 2396 Compliant: " 
+                        + ex.getMessage());
+            }
 	}
 	synchronized (pathToURLsCache) {
 	    pathToURLsCache.put(path,
@@ -1416,8 +1444,21 @@ public class PreferredClassProvider exte
 	}
 	return urls;
     }
+    
+    /* Converts a URI to an URL.
+     */
+    private URL asURL(URI uri) throws MalformedURLException{
+        if (uri == null) throw new NullPointerException("URI cannot be null");
+        try {
+            return uri.toURL();
+        } catch (IllegalArgumentException ex){
+            throw new MalformedURLException(ex.getMessage());
+        }
+    }
 
-    /** map from weak(key=string) to [URL[], soft(key)] */
+    /** 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);
 
     /**
@@ -1429,12 +1470,13 @@ public class PreferredClassProvider exte
 	 * The current implementation simply uses the current thread's
 	 * context class loader.
 	 */
-	return (ClassLoader) AccessController.doPrivileged(
-	    new PrivilegedAction() {
-		public Object run() {
-		    return Thread.currentThread().getContextClassLoader();
-		}
-	    });
+	return AccessController.doPrivileged(
+            new PrivilegedAction<ClassLoader>() {
+              public ClassLoader run() {
+                  return Thread.currentThread().getContextClassLoader();
+              }
+            }
+        );
     }
 
     /**
@@ -1482,25 +1524,26 @@ public class PreferredClassProvider exte
      * loader for a loader that has an export path which matches the
      * parameter path.
      */
-    private ClassLoader findOriginLoader(final URL[] pathURLs,
+    private ClassLoader findOriginLoader(final URI[] pathURLs,
 					 final ClassLoader parent)
     {
-	return (ClassLoader) AccessController.doPrivileged(
-	    new PrivilegedAction() {
-		public Object run() {
-		    return findOriginLoader0(pathURLs, parent);
-		}
-	    });
+	return AccessController.doPrivileged(
+            new PrivilegedAction<ClassLoader>() {
+              public ClassLoader run() {
+                  return findOriginLoader0(pathURLs, parent);
+              }
+            }
+        );
     }
 
-    private ClassLoader findOriginLoader0(URL[] pathURLs, ClassLoader parent) {
+    private ClassLoader findOriginLoader0(URI[] pathURLs, ClassLoader parent) {
 	for (ClassLoader ancestor = parent;
 	     ancestor != null;
 	     ancestor = ancestor.getParent())
 	{
-	    URL[] ancestorURLs;
+	    URI[] ancestorURLs;
 	    try {
-		ancestorURLs = getLoaderAnnotationURLs(ancestor);
+		ancestorURLs = getLoaderAnnotationURIs(ancestor);
 	    } catch (MalformedURLException e) {
 		// this ancestor's annotation must not match pathURLs
 		continue;
@@ -1531,15 +1574,15 @@ public class PreferredClassProvider exte
      * and the given parent class loader.  A new class loader instance
      * will be created and returned if no match is found.
      */
-    private ClassLoader lookupLoader(final URL[] urls,
-				     final ClassLoader parent)
+    private ClassLoader lookupLoader(final URI[] uris, URL[] urls,
+				     final ClassLoader parent) throws MalformedURLException
     {
 	/*
 	 * If the requested codebase is null, then PreferredClassProvider
 	 * assumes that the class is expected to be a "platform" class
 	 * with respect to the parent class loader.
 	 */
-	if (urls == null) {
+	if (uris == null) {
 	    return parent;
 	}
 
@@ -1574,7 +1617,7 @@ public class PreferredClassProvider exte
 	    }
 	}
 
-	LoaderKey key = new LoaderKey(urls, parent);
+	LoaderKey key = new LoaderKey(uris, parent);
 	LoaderEntryHolder holder;
 	synchronized (loaderTable) {
 	    if (!toRemove.isEmpty()) {
@@ -1609,7 +1652,7 @@ public class PreferredClassProvider exte
 	    LoaderEntry entry = holder.entry;
 	    ClassLoader loader;
 	    if (entry == null ||
-		(loader = (ClassLoader) entry.get()) == null)
+		(loader = entry.get()) == null)
 	    {
 		/*
 		 * If entry was in table but it's weak reference was cleared,
@@ -1637,18 +1680,22 @@ public class PreferredClassProvider exte
 		 * context restricted to the permissions necessary to
 		 * load classes from its codebase URL path.
 		 */
-		loader = findOriginLoader(urls, parent);
+		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);
+                    holder.entry = entry;
 		}
 
-		/*
-		 * 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);
-		holder.entry = entry;
 	    }
 	    return loader;
 	}
@@ -1691,9 +1738,9 @@ public class PreferredClassProvider exte
 					    final boolean requireDlPerm)
     {
 	checkInitialized();
-	return (ClassLoader)
-	    AccessController.doPrivileged(new PrivilegedAction() {
-		public Object run() {
+	return
+	    AccessController.doPrivileged(new PrivilegedAction<ClassLoader>() {
+		public ClassLoader run() {
 		    return new PreferredClassLoader(urls, parent, null,
 						    requireDlPerm);
 		}
@@ -1701,24 +1748,23 @@ public class PreferredClassProvider exte
     }
 
     /**
-     * Loader table key: a codebase URL path and a weak reference to
+     * Loader table key: a codebase URI path and a weak reference to
      * 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.
      **/
-    private class LoaderKey extends WeakReference {
-	private final URL[] urls;
+    private class LoaderKey extends WeakReference<ClassLoader> {
+	private final URI[] uris;
 	private final boolean nullParent;
 	private final int hashValue;
 
-	public LoaderKey(URL[] urls, ClassLoader parent) {
+	public LoaderKey(URI[] urls, ClassLoader parent) {
 	    super(parent, refQueue);
-	    this.urls = urls;
 	    nullParent = (parent == null);
-
+            uris = urls;
 	    int h = nullParent ? 0 : parent.hashCode();
 	    for (int i = 0; i < urls.length; i++) {
-		h ^= urls[i].hashCode();
+		h ^= uris[i].hashCode();
 	    }
 	    hashValue = h;
 	}
@@ -1736,9 +1782,9 @@ public class PreferredClassProvider exte
 	    LoaderKey other = (LoaderKey) obj;
 	    ClassLoader parent;
 	    return (nullParent ? other.nullParent
-			       : ((parent = (ClassLoader) get()) != null &&
+			       : ((parent = get()) != null &&
 				  parent == other.get()))
-		&& Arrays.equals(urls, other.urls);
+		&& Arrays.equals(uris, other.uris);
 	}
     }
 
@@ -1747,13 +1793,21 @@ public class PreferredClassProvider exte
      * weak reference is registered with "refQueue" so that the entry
      * can be removed after the loader has become unreachable.
      **/
-    private class LoaderEntry extends WeakReference {
+    private 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;
 
@@ -1767,9 +1821,9 @@ public class PreferredClassProvider exte
     }
 
     private static ClassLoader getClassLoader(final Class c) {
-	return (ClassLoader) AccessController.doPrivileged(
-	    new PrivilegedAction() {
-		public Object run() { return c.getClassLoader(); }
+	return AccessController.doPrivileged(
+	    new PrivilegedAction<ClassLoader>() {
+		public ClassLoader run() { return c.getClassLoader(); }
 	    });
     }
 }



Mime
View raw message