8171139: Simplify ResourceBundle.CacheKey and ClassLoader may not be needed
authornaoto
Tue, 17 Jan 2017 11:34:47 -0800
changeset 43182 66e6655abfde
parent 43109 fe275140c3f1
child 43183 b50e0f90d284
8171139: Simplify ResourceBundle.CacheKey and ClassLoader may not be needed Reviewed-by: dfuchs Contributed-by: peter.levart@gmail.com
jdk/src/java.base/share/classes/java/util/ResourceBundle.java
jdk/test/java/util/ResourceBundle/ResourceBundleTest.java
--- a/jdk/src/java.base/share/classes/java/util/ResourceBundle.java	Mon Jan 16 12:15:44 2017 -0800
+++ b/jdk/src/java.base/share/classes/java/util/ResourceBundle.java	Tue Jan 17 11:34:47 2017 -0800
@@ -43,6 +43,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.UncheckedIOException;
+import java.lang.ref.Reference;
 import java.lang.ref.ReferenceQueue;
 import java.lang.ref.SoftReference;
 import java.lang.ref.WeakReference;
@@ -365,7 +366,7 @@
                 @Override
                 public ResourceBundle getBundle(String baseName, Locale locale, Module module) {
                     // use the given module as the caller to bypass the access check
-                    return getBundleImpl(module, module, getLoader(module),
+                    return getBundleImpl(module, module,
                                          baseName, locale, Control.INSTANCE);
                 }
 
@@ -537,63 +538,19 @@
         return locale;
     }
 
-    /*
-     * Automatic determination of the ClassLoader to be used to load
-     * resources on behalf of the client.
-     */
-    private static ClassLoader getLoader(Class<?> caller) {
-        ClassLoader cl = caller == null ? null : caller.getClassLoader();
-        if (cl == null) {
-            // When the caller's loader is the boot class loader, cl is null
-            // here. In that case, ClassLoader.getSystemClassLoader() may
-            // return the same class loader that the application is
-            // using. We therefore use a wrapper ClassLoader to create a
-            // separate scope for bundles loaded on behalf of the Java
-            // runtime so that these bundles cannot be returned from the
-            // cache to the application (5048280).
-            cl = RBClassLoader.INSTANCE;
-        }
-        return cl;
-    }
-
     private static ClassLoader getLoader(Module module) {
         PrivilegedAction<ClassLoader> pa = module::getClassLoader;
         return AccessController.doPrivileged(pa);
     }
 
     /**
-     * A wrapper of ClassLoader.getSystemClassLoader().
+     * @param module a non-null-screened module form the {@link CacheKey#getModule()}.
+     * @return the ClassLoader to use in {@link Control#needsReload}
+     *         and {@link Control#newBundle}
      */
-    private static class RBClassLoader extends ClassLoader {
-        private static final RBClassLoader INSTANCE = AccessController.doPrivileged(
-                    new PrivilegedAction<RBClassLoader>() {
-                        public RBClassLoader run() {
-                            return new RBClassLoader();
-                        }
-                    });
-        private RBClassLoader() {
-        }
-        public Class<?> loadClass(String name) throws ClassNotFoundException {
-            ClassLoader loader = ClassLoader.getSystemClassLoader();
-            if (loader != null) {
-                return loader.loadClass(name);
-            }
-            return Class.forName(name);
-        }
-        public URL getResource(String name) {
-            ClassLoader loader = ClassLoader.getSystemClassLoader();
-            if (loader != null) {
-                return loader.getResource(name);
-            }
-            return ClassLoader.getSystemResource(name);
-        }
-        public InputStream getResourceAsStream(String name) {
-            ClassLoader loader = ClassLoader.getSystemClassLoader();
-            if (loader != null) {
-                return loader.getResourceAsStream(name);
-            }
-            return ClassLoader.getSystemResourceAsStream(name);
-        }
+    private static ClassLoader getLoaderForControl(Module module) {
+        ClassLoader loader = getLoader(module);
+        return loader == null ? ClassLoader.getSystemClassLoader() : loader;
     }
 
     /**
@@ -610,23 +567,23 @@
 
     /**
      * Key used for cached resource bundles.  The key checks the base
-     * name, the locale, the class loader, and the caller module
+     * name, the locale, the bundle module, and the caller module
      * to determine if the resource is a match to the requested one.
-     * The loader may be null, but the base name, the locale and
-     * module must have a non-null value.
+     * The base name, the locale and both modules must have a non-null value.
      */
-    private static class CacheKey implements Cloneable {
+    private static final class CacheKey {
         // These four are the actual keys for lookup in Map.
-        private String name;
-        private Locale locale;
-        private KeyElementReference<ClassLoader> loaderRef;
-        private KeyElementReference<Module> moduleRef;
-        private KeyElementReference<Module> callerRef;
-
+        private final String name;
+        private volatile Locale locale;
+        private final KeyElementReference<Module> moduleRef;
+        private final KeyElementReference<Module> callerRef;
+        // this is the part of hashCode that pertains to module and callerModule
+        // which can be GCed..
+        private final int modulesHash;
 
         // bundle format which is necessary for calling
         // Control.needsReload().
-        private String format;
+        private volatile String format;
 
         // These time values are in CacheKey so that NONEXISTENT_BUNDLE
         // doesn't need to be cloned for caching.
@@ -639,63 +596,55 @@
         private volatile long expirationTime;
 
         // Placeholder for an error report by a Throwable
-        private Throwable cause;
-
-        // Hash code value cache to avoid recalculating the hash code
-        // of this instance.
-        private int hashCodeCache;
+        private volatile Throwable cause;
 
         // ResourceBundleProviders for loading ResourceBundles
-        private ServiceLoader<ResourceBundleProvider> providers;
-        private boolean providersChecked;
+        private volatile ServiceLoader<ResourceBundleProvider> providers;
+        private volatile boolean providersChecked;
 
         // Boolean.TRUE if the factory method caller provides a ResourceBundleProvier.
-        private Boolean callerHasProvider;
+        private volatile Boolean callerHasProvider;
 
-        CacheKey(String baseName, Locale locale, ClassLoader loader, Module module, Module caller) {
+        CacheKey(String baseName, Locale locale, Module module, Module caller) {
             Objects.requireNonNull(module);
+            Objects.requireNonNull(caller);
 
             this.name = baseName;
             this.locale = locale;
-            if (loader == null) {
-                this.loaderRef = null;
-            } else {
-                this.loaderRef = new KeyElementReference<>(loader, referenceQueue, this);
-            }
             this.moduleRef = new KeyElementReference<>(module, referenceQueue, this);
             this.callerRef = new KeyElementReference<>(caller, referenceQueue, this);
+            this.modulesHash = module.hashCode() ^ caller.hashCode();
+        }
 
-            calculateHashCode();
+        CacheKey(CacheKey src) {
+            // Create References to src's modules
+            this.moduleRef = new KeyElementReference<>(
+                Objects.requireNonNull(src.getModule()), referenceQueue, this);
+            this.callerRef = new KeyElementReference<>(
+                Objects.requireNonNull(src.getCallerModule()), referenceQueue, this);
+            // Copy fields from src. ResourceBundleProviders related fields
+            // and "cause" should not be copied.
+            this.name = src.name;
+            this.locale = src.locale;
+            this.modulesHash = src.modulesHash;
+            this.format = src.format;
+            this.loadTime = src.loadTime;
+            this.expirationTime = src.expirationTime;
         }
 
         String getName() {
             return name;
         }
 
-        CacheKey setName(String baseName) {
-            if (!this.name.equals(baseName)) {
-                this.name = baseName;
-                calculateHashCode();
-            }
-            return this;
-        }
-
         Locale getLocale() {
             return locale;
         }
 
         CacheKey setLocale(Locale locale) {
-            if (!this.locale.equals(locale)) {
-                this.locale = locale;
-                calculateHashCode();
-            }
+            this.locale = locale;
             return this;
         }
 
-        ClassLoader getLoader() {
-            return (loaderRef != null) ? loaderRef.get() : null;
-        }
-
         Module getModule() {
             return moduleRef.get();
         }
@@ -728,7 +677,7 @@
             try {
                 final CacheKey otherEntry = (CacheKey)other;
                 //quick check to see if they are not equal
-                if (hashCodeCache != otherEntry.hashCodeCache) {
+                if (modulesHash != otherEntry.modulesHash) {
                     return false;
                 }
                 //are the names the same?
@@ -739,24 +688,11 @@
                 if (!locale.equals(otherEntry.locale)) {
                     return false;
                 }
-                //are refs (both non-null) or (both null)?
-                if (loaderRef == null) {
-                    return otherEntry.loaderRef == null;
-                }
-                ClassLoader loader = getLoader();
+                // are modules and callerModules the same and non-null?
                 Module module = getModule();
                 Module caller = getCallerModule();
-
-                return (otherEntry.loaderRef != null)
-                        // with a null reference we can no longer find
-                        // out which class loader or module was referenced; so
-                        // treat it as unequal
-                        && (loader != null)
-                        && (loader == otherEntry.getLoader())
-                        && (module != null)
-                        && (module.equals(otherEntry.getModule()))
-                        && (caller != null)
-                        && (caller.equals(otherEntry.getCallerModule()));
+                return ((module != null) && (module.equals(otherEntry.getModule())) &&
+                        (caller != null) && (caller.equals(otherEntry.getCallerModule())));
             } catch (NullPointerException | ClassCastException e) {
             }
             return false;
@@ -764,51 +700,7 @@
 
         @Override
         public int hashCode() {
-            return hashCodeCache;
-        }
-
-        private void calculateHashCode() {
-            hashCodeCache = name.hashCode() << 3;
-            hashCodeCache ^= locale.hashCode();
-            ClassLoader loader = getLoader();
-            if (loader != null) {
-                hashCodeCache ^= loader.hashCode();
-            }
-            Module module = getModule();
-            if (module != null) {
-                hashCodeCache ^= module.hashCode();
-            }
-            Module caller = getCallerModule();
-            if (caller != null) {
-                hashCodeCache ^= caller.hashCode();
-            }
-        }
-
-        @Override
-        public Object clone() {
-            try {
-                CacheKey clone = (CacheKey) super.clone();
-                if (loaderRef != null) {
-                    clone.loaderRef = new KeyElementReference<>(getLoader(),
-                                                                referenceQueue, clone);
-                }
-                clone.moduleRef = new KeyElementReference<>(getModule(),
-                                                            referenceQueue, clone);
-                clone.callerRef = new KeyElementReference<>(getCallerModule(),
-                                                            referenceQueue, clone);
-
-                // Clear the reference to ResourceBundleProviders and the flag
-                clone.providers = null;
-                clone.providersChecked = false;
-                // Clear the reference to a Throwable
-                clone.cause = null;
-                // Clear callerHasProvider
-                clone.callerHasProvider = null;
-                return clone;
-            } catch (CloneNotSupportedException e) {
-                //this should never happen
-                throw new InternalError(e);
-            }
+            return (name.hashCode() << 3) ^ locale.hashCode() ^ modulesHash;
         }
 
         String getFormat() {
@@ -845,8 +737,12 @@
                     l = "\"\"";
                 }
             }
-            return "CacheKey[" + name + ", lc=" + l + ", ldr=" + getLoader()
-                + "(format=" + format + ")]";
+            return "CacheKey[" + name +
+                   ", locale=" + l +
+                   ", module=" + getModule() +
+                   ", callerModule=" + getCallerModule() +
+                   ", format=" + format +
+                   "]";
         }
     }
 
@@ -1568,7 +1464,7 @@
                                                 Locale locale,
                                                 Class<?> caller,
                                                 Control control) {
-        return getBundleImpl(baseName, locale, caller, getLoader(caller), control);
+        return getBundleImpl(baseName, locale, caller, caller.getClassLoader(), control);
     }
 
     /**
@@ -1587,26 +1483,25 @@
                                                 Class<?> caller,
                                                 ClassLoader loader,
                                                 Control control) {
-        if (caller != null && caller.getModule().isNamed()) {
-            Module module = caller.getModule();
-            ClassLoader ml = getLoader(module);
-            // get resource bundles for a named module only
-            // if loader is the module's class loader
-            if (loader == ml || (ml == null && loader == RBClassLoader.INSTANCE)) {
-                return getBundleImpl(module, module, loader, baseName, locale, control);
-            }
-        }
-        // find resource bundles from unnamed module
-        Module unnamedModule = loader != null
-            ? loader.getUnnamedModule()
-            : ClassLoader.getSystemClassLoader().getUnnamedModule();
-
         if (caller == null) {
             throw new InternalError("null caller");
         }
+        Module callerModule = caller.getModule();
 
-        Module callerModule = caller.getModule();
-        return getBundleImpl(callerModule, unnamedModule, loader, baseName, locale, control);
+        // get resource bundles for a named module only if loader is the module's class loader
+        if (callerModule.isNamed() && loader == getLoader(callerModule)) {
+            return getBundleImpl(callerModule, callerModule, baseName, locale, control);
+        }
+
+        // find resource bundles from unnamed module of given class loader
+        // Java agent can add to the bootclasspath e.g. via
+        // java.lang.instrument.Instrumentation and load classes in unnamed module.
+        // It may call RB::getBundle that will end up here with loader == null.
+        Module unnamedModule = loader != null
+            ? loader.getUnnamedModule()
+            : BootLoader.getUnnamedModule();
+
+        return getBundleImpl(callerModule, unnamedModule, baseName, locale, control);
     }
 
     private static ResourceBundle getBundleFromModule(Class<?> caller,
@@ -1622,12 +1517,11 @@
                 sm.checkPermission(GET_CLASSLOADER_PERMISSION);
             }
         }
-        return getBundleImpl(callerModule, module, getLoader(module), baseName, locale, control);
+        return getBundleImpl(callerModule, module, baseName, locale, control);
     }
 
     private static ResourceBundle getBundleImpl(Module callerModule,
                                                 Module module,
-                                                ClassLoader loader,
                                                 String baseName,
                                                 Locale locale,
                                                 Control control) {
@@ -1636,10 +1530,10 @@
         }
 
         // We create a CacheKey here for use by this call. The base name
-        // loader, and module will never change during the bundle loading
+        // and modules will never change during the bundle loading
         // process. We have to make sure that the locale is set before
         // using it as a cache key.
-        CacheKey cacheKey = new CacheKey(baseName, locale, loader, module, callerModule);
+        CacheKey cacheKey = new CacheKey(baseName, locale, module, callerModule);
         ResourceBundle bundle = null;
 
         // Quick lookup of the cache.
@@ -1708,6 +1602,11 @@
             bundle = baseBundle;
         }
 
+        // keep callerModule and module reachable for as long as we are operating
+        // with WeakReference(s) to them (in CacheKey)...
+        Reference.reachabilityFence(callerModule);
+        Reference.reachabilityFence(module);
+
         return bundle;
     }
 
@@ -1745,7 +1644,7 @@
         }
 
         // Before we do the real loading work, see whether we need to
-        // do some housekeeping: If references to class loaders or
+        // do some housekeeping: If references to modules or
         // resource bundles have been nulled out, remove all related
         // information from the cache.
         Object ref;
@@ -1781,31 +1680,24 @@
         }
 
         if (bundle != NONEXISTENT_BUNDLE) {
-            CacheKey constKey = (CacheKey) cacheKey.clone();
             trace("findBundle: %d %s %s formats: %s%n", index, candidateLocales, cacheKey, formats);
-            try {
-                if (module.isNamed()) {
-                    bundle = loadBundle(cacheKey, formats, control, module, callerModule);
-                } else {
-                    bundle = loadBundle(cacheKey, formats, control, expiredBundle);
+            if (module.isNamed()) {
+                bundle = loadBundle(cacheKey, formats, control, module, callerModule);
+            } else {
+                bundle = loadBundle(cacheKey, formats, control, expiredBundle);
+            }
+            if (bundle != null) {
+                if (bundle.parent == null) {
+                    bundle.setParent(parent);
                 }
-                if (bundle != null) {
-                    if (bundle.parent == null) {
-                        bundle.setParent(parent);
-                    }
-                    bundle.locale = targetLocale;
-                    bundle = putBundleInCache(cacheKey, bundle, control);
-                    return bundle;
-                }
+                bundle.locale = targetLocale;
+                bundle = putBundleInCache(cacheKey, bundle, control);
+                return bundle;
+            }
 
-                // Put NONEXISTENT_BUNDLE in the cache as a mark that there's no bundle
-                // instance for the locale.
-                putBundleInCache(cacheKey, NONEXISTENT_BUNDLE, control);
-            } finally {
-                if (constKey.getCause() instanceof InterruptedException) {
-                    Thread.currentThread().interrupt();
-                }
-            }
+            // Put NONEXISTENT_BUNDLE in the cache as a mark that there's no bundle
+            // instance for the locale.
+            putBundleInCache(cacheKey, NONEXISTENT_BUNDLE, control);
         }
         return parent;
     }
@@ -1991,12 +1883,20 @@
         // specified by the getFormats() value.
         Locale targetLocale = cacheKey.getLocale();
 
+        Module module = cacheKey.getModule();
+        if (module == null) {
+            // should not happen
+            throw new InternalError(
+                "Module for cache key: " + cacheKey + " has been GCed.");
+        }
+        ClassLoader loader = getLoaderForControl(module);
+
         ResourceBundle bundle = null;
         for (String format : formats) {
             try {
                 // ResourceBundle.Control.newBundle may be overridden
                 bundle = control.newBundle(cacheKey.getName(), targetLocale, format,
-                                           cacheKey.getLoader(), reload);
+                                           loader, reload);
             } catch (LinkageError | Exception error) {
                 // We need to handle the LinkageError case due to
                 // inconsistent case-sensitivity in ClassLoader.
@@ -2138,12 +2038,15 @@
                         if (!bundle.expired && expirationTime >= 0 &&
                             expirationTime <= System.currentTimeMillis()) {
                             try {
-                                bundle.expired = control.needsReload(key.getName(),
-                                                                     key.getLocale(),
-                                                                     key.getFormat(),
-                                                                     key.getLoader(),
-                                                                     bundle,
-                                                                     key.loadTime);
+                                Module module = cacheKey.getModule();
+                                bundle.expired =
+                                    module == null || // already GCed
+                                    control.needsReload(key.getName(),
+                                                        key.getLocale(),
+                                                        key.getFormat(),
+                                                        getLoaderForControl(module),
+                                                        bundle,
+                                                        key.loadTime);
                             } catch (Exception e) {
                                 cacheKey.setCause(e);
                             }
@@ -2185,7 +2088,7 @@
                                                    Control control) {
         setExpirationTime(cacheKey, control);
         if (cacheKey.expirationTime != Control.TTL_DONT_CACHE) {
-            CacheKey key = (CacheKey) cacheKey.clone();
+            CacheKey key = new CacheKey(cacheKey);
             BundleReference bundleRef = new BundleReference(bundle, referenceQueue, key);
             bundle.cacheKey = key;
 
@@ -2239,7 +2142,7 @@
     @CallerSensitive
     public static final void clearCache() {
         Class<?> caller = Reflection.getCallerClass();
-        clearCache(getLoader(caller), caller.getModule());
+        clearCacheImpl(caller.getModule(), caller.getClassLoader());
     }
 
     /**
@@ -2254,7 +2157,8 @@
     @CallerSensitive
     public static final void clearCache(ClassLoader loader) {
         Objects.requireNonNull(loader);
-        clearCache(loader, Reflection.getCallerClass().getModule());
+        Class<?> caller = Reflection.getCallerClass();
+        clearCacheImpl(caller.getModule(), loader);
     }
 
     /**
@@ -2272,14 +2176,19 @@
      * @see ResourceBundle.Control#getTimeToLive(String,Locale)
      */
     public static final void clearCache(Module module) {
-        clearCache(module.getClassLoader(), module);
+        Objects.requireNonNull(module);
+        clearCacheImpl(module, module.getClassLoader());
     }
 
-    private static void clearCache(ClassLoader loader, Module module) {
-        Set<CacheKey> set = cacheList.keySet();
-        set.stream()
-           .filter((key) -> (key.getLoader() == loader && key.getModule() == module))
-           .forEach(set::remove);
+    private static void clearCacheImpl(Module callerModule, ClassLoader loader) {
+        cacheList.keySet().removeIf(
+            key -> {
+                Module m;
+                return key.getCallerModule() == callerModule &&
+                       (m = key.getModule()) != null &&
+                       getLoader(m) == loader;
+            }
+        );
     }
 
     /**
--- a/jdk/test/java/util/ResourceBundle/ResourceBundleTest.java	Mon Jan 16 12:15:44 2017 -0800
+++ b/jdk/test/java/util/ResourceBundle/ResourceBundleTest.java	Tue Jan 17 11:34:47 2017 -0800
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2007, 2013, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2007, 2017, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -22,7 +22,7 @@
  */
 /*
     @test
-    @bug 4049325 4073127 4083270 4106034 4108126 8027930
+    @bug 4049325 4073127 4083270 4106034 4108126 8027930 8171139
     @summary test Resource Bundle
     @build TestResource TestResource_de TestResource_fr TestResource_fr_CH
     @build TestResource_it FakeTestResource
@@ -65,6 +65,7 @@
 import java.util.*;
 import java.util.ResourceBundle.Control;
 import java.io.*;
+import java.net.URL;
 
 public class ResourceBundleTest extends RBTestFmwk {
     public static void main(String[] args) throws Exception {
@@ -330,6 +331,44 @@
         }
     }
 
+    /*
+     * @bug 8171139
+     * @summary Make sure clearCache() clears cached ResourceBundle instances
+     */
+    public void TestClearCache() {
+        final String className = "TestResource";
+        Locale loc = Locale.getDefault();
+
+        // testing no-arg clearCache()
+        ResourceBundle rb1 = ResourceBundle.getBundle(className, loc);
+        ResourceBundle.clearCache();
+        ResourceBundle rb2 = ResourceBundle.getBundle(className, loc);
+        if (rb1 == rb2) {
+            errln("clearCache(no-arg) did not clear cache");
+        }
+
+        // clearCache() with a custom classloader
+        ClassLoader cl1 = new DummyClassLoader();
+        rb1 = ResourceBundle.getBundle(className, loc, cl1);
+        if (rb1 == rb2) {
+            errln("Same bundle was returned for different class loaders");
+        }
+        ResourceBundle.clearCache(cl1);
+        rb2= ResourceBundle.getBundle(className, loc, cl1);
+        if (rb1 == rb2) {
+            errln("clearCache(classLoader) did not clear cache");
+        }
+        ClassLoader cl2 = new DummyClassLoader();
+        rb1 = ResourceBundle.getBundle(className, loc, cl2);
+        if (rb1 == rb2) {
+            errln("Same bundle was returned for different class loaders");
+        }
+        ResourceBundle.clearCache(cl1);
+        rb2 = ResourceBundle.getBundle(className, loc, cl2);
+        if (rb1 != rb2) {
+            errln("clearCache(classLoader) incorrectly cleared cache");
+        }
+    }
 
     private void makePropertiesFile() {
         try {
@@ -393,4 +432,22 @@
             errln("Wrong number of elements in key list: expected " + expectedKeys.length +
                 " got " + elementCount);
     }
+
+    private static class DummyClassLoader extends ClassLoader {
+        public DummyClassLoader() {
+            super(DummyClassLoader.class.getClassLoader());
+        }
+
+        public Class<?> loadClass(String name) throws ClassNotFoundException {
+            return DummyClassLoader.class.getClassLoader().loadClass(name);
+        }
+
+        public URL getResource(String name) {
+            return DummyClassLoader.class.getClassLoader().getResource(name);
+        }
+
+        public InputStream getResourceAsStream(String name) {
+            return DummyClassLoader.class.getClassLoader().getResourceAsStream(name);
+        }
+    }
 }