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

[Jim Gish]

On 03/06/2013 12:58 PM, Mandy Chung wrote:
>
>
> 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?
Yes.
>
>> (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?
It used to be:  "if (name == catalogName)" and now is "if 
(name.equals(catalogName))"  Therefore if a different object was passed 
in for name than catalogName, that test would fail.  However, even  if 
name.equals(catalogName) was initially true in the old code it could 
proceed to do the search (possibly unnecessarily in the case where it 
would simply return the cached bundle anyway), but even if the search 
failed it would discover the String value of the name was equivalent to 
the String value of catalogName and then and only then return the cached 
catalog.  Now, we simply check for matching String values at the 
beginning and return the cached catalog if the Locale hasn't changed.
>
>> 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 .... }
I think we'll save that for a later day - and then update all the 
occurrences of <code> to {@code}
> L129-130: you can use @linkplain instead:
>      {@linkplain ClassLoader#getSystemClassLoader()system ClassLoader}
All other existing links are using @link.  Why should we use @linkplain 
here?
> 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.
I meant to say "cannot be /directly/ changed"  I'll fix it. Thanks.
>
> 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.
I'll take another look at that.
>
> L1575: the @link here is not necessary in a comment
OK -- just trying to be complete and consistent with the previous 
references.
>
> 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.
Leaving the existing test as is not an option unless we change the test 
to run in othervm as I had on my first webrev.  The bundles are not 
found otherwise.  Hence the change to set the context classloader.

>
> 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

-- 
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