logging fixes for last pre-ZBB JDK7-TL snapshot (6977677, 7016208, 7041595)

[Daniel D. Daugherty]

On 5/16/2011 9:13 PM, Mandy Chung wrote:
>  On 5/16/11 7:00 PM, David Holmes wrote:
>> Dan,
>>
>> I'm sorry this didn't come through in time ...
>>
>> Mandy Chung said the following on 05/17/11 05:22:
>>> Logger.java
>>>    Looks good.   The removal of the synchronization required by 
>>> Logger.getLogger fixes the deadlock issue.
>>
>> But this method now has a race condition:
>>
>>  372     // Synchronization is not required here. All synchronization 
>> for
>>  373     // adding a new Logger object is handled by 
>> LogManager.addLogger().
>>  374     public static Logger getLogger(String name, String 
>> resourceBundleName) {
>>  375         LogManager manager = LogManager.getLogManager();
>>  376         Logger result = manager.demandLogger(name);
>>  377         if (result.resourceBundleName == null) {
>>  378             // Note: we may get a MissingResourceException here.
>>  379             result.setupResourceInfo(resourceBundleName);
>>  380         } else if 
>> (!result.resourceBundleName.equals(resourceBundleName)) {
>>  381             throw new 
>> IllegalArgumentException(result.resourceBundleName +
>>  382                                 " != " + resourceBundleName);
>>  383         }
>>  384         return result;
>>  385     }
>>
>> Two threads calling this method with the same name but different 
>> resource bundle names can both see null at line 377 and install their 
>> different resource bundles. That section needs to be atomic.
>>
>
> My bad - missed this race condition.

We all missed it except for David.
Trust me, I'm kicking myself enough for everyone...


> The setupResourceInfo method synchronizes on this Logger instance.  
> The resourceBundleName should only be set up to once or same resource 
> bundle name.  I think it should check if this.resourceBundleName is 
> null or same as the given resourceBundleName; if not, throw IAE.   The 
> if-then-else in L377-383 above is no longer needed and can simply call 
> result.setupResourceInfo(resourceBundleName).

Basically move the sanity checking that it currently in
getLogger(String, String) down into setupResourceInfo() where
it's protected by the lock. Make sure that I don't change any
of the implied semantics of getLogger(String, String) or
Logger(String, String)...

Thanks for the analysis...

Dan