Review Request JDK-8170772: ResourceBundle improper caching causes tools/javadoc tests intermittently
Peter Levart
peter.levart at gmail.com
Sat Jan 7 06:57:16 UTC 2017
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?
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