RFR: 8002070 Remove the stack search for a resource bundle for Logger to use

[Mandy Chung]

On 3/5/2013 9:16 AM, Jim Gish wrote:
>
> On 03/01/2013 05:48 PM, Mandy Chung wrote:
>> On 3/1/2013 1:25 PM, Jim Gish wrote:
>>> Please review 
>>> http://cr.openjdk.java.net/~jgish/Bug8002070-RemoveResourceBundleStackSearch/ 
>>>
>>> This removes the stack search from Logger.findResourceBundle()
>>>
>>
>> It's good to see this stack walk search of resource bundle going away.
>>
>> In Logger.java, the existing implementation returns the previous 
>> cached resource bundle if it fails to find one matching the current 
>> locale but the name matches:
>>
>> 1608         if (name.equals(catalogName)) {
>> 1609             // Return the previous cached value for that name.
>> 1610             // This may be null.
>> 1611             return catalog;
>> 1612         }
>>
>>
>> Your fix removes these lines which I think is fine.  The 
>> Logger.getResourceBundle method specifies to return the resource 
>> bundle for this logger for the current default locale.  I think it'd 
>> be rare to change the default locale during the execution of an 
>> application.
> The old behavior upon reaching this point in the code was as follows:
> To get here, the sought after bundle was not found and (from the 
> checks on line 1559,1560), either
> (1) the previously cached catalog was null, meaning nothing was yet 
> cached and so null was returned, or

correct.

> (2) the current locale had changed and no bundle for that locale was 
> found, in which case the cached bundle (for the old/wrong locale) was 
> returned, or

do you mean it returns the cached bundle only if the name matches?  or 
am I missing the code that you read?

> (3) the name was the same but the location of the objects used to 
> compare the cached name with that passed in was different, 

Can you elaborate?  Are you referring the "if 
(name.equals(catalogName))" statement?

> so the previously cached bundle was returned (this is frankly an odd 
> case, because it also means that re-searching for a previously found 
> bundle is now failing, which only seems possible if the bundle is/was 
> (a) not available via the system classloader, (b) the context 
> classloader, or (c), the classloaders up the call chain.
>
> The new behavior /does /still allow for a change in the default 
> locale.  For example, if you first call findResourceBundleName("foo") 
> and the default locale is US, it will search for the foo_US bundle and 
> set catalogLocale=US, and catalogName=foo and return the foo_US 
> bundle.  If you then search for "foo" and have changed the default 
> locale to DE (if that is indeed a valid locale), it will then search 
> for foo_DE and if found set catalogLocale=DE and cache and return the 
> foo_DE bundle.   The /only /change in behavior that I see here, is 
> that if you change the locale and the bundle is not found, null will 
> be returned instead of the previously cached bundle (if one exists), 
> for a /different/ locale.
>

Right.   The behavioral difference that I point out was when foo_DE is 
cached, later the current locale is set to JP where foo_JP and foo don't 
exist, the old behavior returns foo_DE which was a bug too me whereas 
the new behavior returns null which matches the spec of 
Logger.getResourceBundle().

> So, the old behavior in effect had a notion of a default catalog, but 
> only if you happened to find a bundle on a previous lookup and cache 
> it.  However, you wouldn't be guaranteed of acquiring this same bundle 
> on a subsequent search if a search for a different name had intervened.
>
> Thus, the new behavior is that if you search for a name/locale and 
> it's not found, you will get null (every time). This seems better to 
> me because it's consistent, explainable and simple.
>

This matches what Logger.getResourceBundle() is specified and the 
behavior is correct.  The difference in behavior is a minimal 
compatibility risk.

Comments on the updated webrev:

> http://cr.openjdk.java.net/~jgish/Bug8002070-RemoveResourceBundleStackSearch/ 


L122-125: you may want to replace <code>....</code> with {@code .... }
L129-130: you can use @linkplain instead:

     {@linkplain ClassLoader#getSystemClassLoader()system ClassLoader}

L141-142: "the bundle associated with the Logger object itself cannot be 
changed" - is this statement correct?  The cached catalog object is not 
accessed directly; instead when it finds the resource bundle every time 
it logs a message.

In addition the new paragraph L132-142 doesn't seem to be necessary 
since the spec is pretty clear on using the resource bundle of the 
current locale.

L1575: the @link here is not necessary in a comment

LoggerResourceBundleRace.java: I think what you really want is to add a 
new test that sets a context class loader to a class loader that finds 
the resource bundle for a logger that a system class loader can't 
find.   I suggest to leave this test as it is and then add a new test to 
exercise the context class loader search of a resource bundle as a 
separate RFE that will improve the test coverage.

Mandy

> I'd appreciate you verifying this.
>
> Thanks,
>
> Jim
>>
>> I suggest to document this behavioral change as well in the bug 
>> report and CCC.
>>
>> Thanks
>> Mandy
>
> -- 
> Jim Gish | Consulting Member of Technical Staff | +1.781.442.0304
> Oracle Java Platform Group | Core Libraries Team
> 35 Network Drive
> Burlington, MA 01803
> jim.gish at oracle.com