Review Request JDK-8170772: ResourceBundle improper caching causes tools/javadoc tests intermittently
Daniel Fuchs
daniel.fuchs at oracle.com
Tue Dec 13 10:14:00 UTC 2016
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
>
More information about the core-libs-dev
mailing list