Review Request JDK-8170772: ResourceBundle improper caching causes tools/javadoc tests intermittently
Mandy Chung
mandy.chung at oracle.com
Sat Jan 7 04:41:18 UTC 2017
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 <naoto.sato at oracle.com> 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
>>>
>>
More information about the core-libs-dev
mailing list