Review Request JDK-8170772: ResourceBundle improper caching causes tools/javadoc tests intermittently
Naoto Sato
naoto.sato at oracle.com
Mon Jan 9 15:58:18 UTC 2017
On 1/6/17 10:57 PM, Peter Levart wrote:
> Hi Mandy, Naoto,
>
> I think Mandy's original proposal has been pushed, but the
> simplification of CacheKey is still open:
>
> https://bugs.openjdk.java.net/browse/JDK-8171139
>
> as well as the re-examination of clearCahe methods:
>
> https://bugs.openjdk.java.net/browse/JDK-8171140
>
> Were you thinking of those two issues Naoto?
Yes. That was my intent.
Naoto
>
>
> Regards, Peter
>
> On 01/07/2017 05:41 AM, Mandy Chung wrote:
>> 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