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
