Thanks for looking through this, Naoto. This has been pushed and resolved in jdk-9+149.
Mandy > On Jan 6, 2017, at 3:46 PM, Naoto Sato <[email protected]> wrote: > > Hi Peter, Daniel, Mandy, > > Sorry for the late reply, and thanks for the patch. I went through the patch > and I did not find any problem with it. Would you want to proceed with this? > > Naoto > > On 12/13/16 2:14 AM, Daniel Fuchs wrote: >> Hi Peter, >> >> This is a bold proposal, I would be frightened to touch at >> this code :-) >> >> Good observations about the simplifications induced by taking >> the caller's module as part of the cache key (in particular >> getting rid of RBClassLoader.INSTANCE). >> >> I have imported your patch (had to fight a bit because it >> includes changes that had already been pushed) and sent it >> through our internal testing system - and haven't seen any >> new failures that seemed linked to resource bundles. >> >> A few observations concerning CacheKey - if I'm not mistaken >> most of the key variables could be made final (in particular >> callerRef and moduleRef) - and since they are required to be >> non null - then getModule() and getCallerModule() could be >> simplified. Not sure whether making those final might require >> to add a copy constructor to support clone() though? >> It seems CacheKey::setName is never called - but it's probably >> safer to keep it (maybe it's called by tests). >> >> I am not an expert of ResourceBundle - though I had to dive >> into it a few times due it's use in logging. Hopefully others >> will jump on this. >> >> best regards, >> >> -- daniel >> >> On 12/12/16 15:10, Peter Levart wrote: >>> Hi Mandy (once again for the list), >>> >>> On 12/09/2016 05:49 PM, Mandy Chung wrote: >>>> Naoto, > > Can you review this ResourceBundle caching fix? The >>>> caller module >>>> may be different than the specified module to > >>> ResourceBundle.getBundle(String, Module) method and it should also > >>> part of the cache key. > > >>> http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8170772/webrev.00/ > > >>> The new test shows the issue there and the first loading of the > >>> resource bundle of a specific module (success or fail) will be put in > >>> the cache and used by subsequent calls. > > Thanks Mandy >>> >>> >>> I think that now callerModule is part of CacheKey, caching logic could >>> be simplified. 1st the getBundleImpl method in line 1629: >>> >>> private static ResourceBundle getBundleImpl(String baseName, >>> Locale locale, >>> 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(); >>> return getBundleImpl(callerModule, unnamedModule, loader, >>> baseName, locale, control); >>> } >>> >>> >>> ... could be cleaned up a bit without changing its semantics: >>> >>> private static ResourceBundle getBundleImpl(String baseName, >>> Locale locale, >>> Class<?> caller, >>> ClassLoader loader, >>> Control control) { >>> if (caller == null) { >>> throw new InternalError("null caller"); >>> } >>> Module callerModule = caller.getModule(); >>> >>> if (callerModule.isNamed()) { >>> ClassLoader ml = getLoader(callerModule); >>> // 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(callerModule, callerModule, loader, >>> baseName, locale, control); >>> } >>> } >>> >>> // find resource bundles from unnamed module >>> Module unnamedModule = loader != null >>> ? loader.getUnnamedModule() >>> : ClassLoader.getSystemClassLoader().getUnnamedModule(); >>> >>> return getBundleImpl(callerModule, unnamedModule, loader, >>> baseName, locale, control); >>> } >>> >>> >>> Next, I checked all callers of this method (there are 3 of them in >>> lines: 1367, 1589, 1615) and all of them guard against passing a null >>> 'loader' to this method: >>> >>> @CallerSensitive >>> public static ResourceBundle getBundle(String baseName, Locale >>> locale, >>> ClassLoader loader) >>> { >>> if (loader == null) { >>> throw new NullPointerException(); >>> } >>> Class<?> caller = Reflection.getCallerClass(); >>> return getBundleImpl(baseName, locale, caller, loader, >>> getDefaultControl(caller, baseName)); >>> } >>> >>> ... >>> >>> @CallerSensitive >>> public static ResourceBundle getBundle(String baseName, Locale >>> targetLocale, >>> ClassLoader loader, Control >>> control) { >>> if (loader == null || control == null) { >>> throw new NullPointerException(); >>> } >>> Class<?> caller = Reflection.getCallerClass(); >>> checkNamedModule(caller); >>> return getBundleImpl(baseName, targetLocale, caller, loader, >>> control); >>> } >>> >>> ... >>> >>> private static ResourceBundle getBundleImpl(String baseName, >>> Locale locale, >>> Class<?> caller, >>> Control control) { >>> return getBundleImpl(baseName, locale, caller, >>> getLoader(caller), control); >>> } >>> >>> 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; >>> } >>> >>> >>> Therefore the above method can be simplified further: >>> >>> private static ResourceBundle getBundleImpl(String baseName, >>> Locale locale, >>> Class<?> caller, >>> ClassLoader loader, >>> Control control) { >>> if (caller == null) { >>> throw new InternalError("null caller"); >>> } >>> if (loader == null) { >>> throw new InternalError("null loader"); >>> } >>> Module callerModule = caller.getModule(); >>> >>> if (callerModule.isNamed()) { >>> ClassLoader ml = getLoader(callerModule); >>> // 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(callerModule, callerModule, loader, >>> baseName, locale, control); >>> } >>> } >>> >>> // find resource bundles from unnamed module >>> Module unnamedModule = loader.getUnnamedModule(); >>> return getBundleImpl(callerModule, unnamedModule, loader, >>> baseName, locale, control); >>> } >>> >>> >>> >>> ...here we can see that (callerModule, module, loader) triple passed to >>> downstream getBundleImpl is either (callerModule, callerModule, >>> callerModule's class loader) - with a RBClassLoader.INSTANCE substitute >>> for bootstrap class loader, when the callerModule is a named module and >>> requested loader is the callerModule's loader, or (callerModule, >>> loader's unnamed module, loader) when loader is not callerModule's >>> loader or callerModule is unnamed. >>> >>> other two callers of the downstream getBundleImpl are the >>> JavaUtilResourceBundleAccess method: >>> >>> 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), >>> baseName, locale, >>> Control.INSTANCE); >>> } >>> >>> ...which is used in logging - the module passed here can be either named >>> or unnamed; >>> >>> ... and a method invoked from new JDK 9 public getBundle() methods >>> taking explicit Module argument: >>> >>> private static ResourceBundle getBundleFromModule(Class<?> caller, >>> Module module, >>> String baseName, >>> Locale locale, >>> Control control) { >>> Objects.requireNonNull(module); >>> Module callerModule = caller.getModule(); >>> if (callerModule != module) { >>> SecurityManager sm = System.getSecurityManager(); >>> if (sm != null) { >>> sm.checkPermission(GET_CLASSLOADER_PERMISSION); >>> } >>> } >>> return getBundleImpl(callerModule, module, getLoader(module), >>> baseName, locale, control); >>> } >>> >>> In all of these cases, the loader passed to downstream getBundleImpl is >>> the module's (2nd argument 'module') class loader (or a special >>> substitute for bootstrap loader). >>> >>> Considering all this, I think class loader is not needed any more as the >>> CacheKey component. The distinction between scopes of system class >>> loader (when the caller is not a bootstrap class) and the >>> RBClassLoader.INSTANCE (when the caller is the bootstrap class) is also >>> not needed any more since the callerModule is now part of CacheKey. >>> >>> I modified your patch (just ResourceBundle.java) to include all these >>> simplifications and some cleanup: >>> >>> http://cr.openjdk.java.net/~plevart/jdk9-dev/8170772_ResourceBundle.caching/webrev.01/ >>> >>> >>> >>> >>> This modification also contains a re-interpretation of clearCache() >>> methods. Both existing clearCahe() methods together with the additional >>> @since 9 method contain the following wording: >>> >>> "Removes all resource bundles from the cache that have been loaded by >>> the caller's / given module..." >>> >>> What does it meant for a bundle to be loaded *by* some module? I think >>> the right interpretation is that this is the caller module (the one that >>> invokes ResourceBundle.getBundle() method). The module that calls >>> ResourceBundle.getBundle() is usually also the module that is >>> responsible for clearing the cache of entries that were cached by its >>> loading requests, isn't it? >>> >>> So, what do you think? >>> >>> Regards, Peter >>> >>
