Review Request JDK-8170772: ResourceBundle improper caching causes tools/javadoc tests intermittently
Naoto Sato
naoto.sato at oracle.com
Fri Jan 6 23:46:07 UTC 2017
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
>>
>
More information about the core-libs-dev
mailing list