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
