8029281: Synchronization issues in Logger and LogManager

[Mandy Chung]

On 11/29/2013 3:41 AM, Daniel Fuchs wrote:
> Hi,
>
> Here is a new revision that includes Peter's findings.
> Thanks again for that Peter!
>
> <http://cr.openjdk.java.net/~dfuchs/webrev_8029281/webrev.01/>
>

LogManager.java
    L156: make props volatile is good.

    I think the reason why LoggerWeakRef.node and parentRef don't need 
to be volatile because of the synchronized block to check and set 
disposed flag that ensures that only one thread will be doing the node 
and parentRef cleanup.  I agree that the two lines should be done with 
synchronized on the LoggerContext.

      node.context.removeLogger(name);
      node.loggerRef = null;  // clear LogNode's weak ref to us

I don't see anything obvious wrong with this patch.   Have you 
considered having the dispose() method simply synchronizes on 
node.context (probably need to get the LoggerContext when instantiating 
LoggerWeakRef and break dispose method into two - one for the node and 
the other for the parentRef)?    I'm not proposing to do this in JDK 8 
since we are ramping down and get a low risk fix in.   It may be 
something to revisit for jdk 9 to a cleaner version.

Logger.java
    The assert this.loggerBundle == NO_RESOURCE_BUNDLE; in the Logger 
constructors doesn't seem to be needed.

Having an immutable LoggerBundle class is good to ensure the 
resourceBundleName and resourceBundle are consistent.

    L1930: is lb.userBundle and lb.resourceBundleName null when getting 
to this line?  Might be better to rename setupResourceInfo to 
setResourceBundleName (that creates a LoggerBundle with a null RB). On 
the other hand,  setResourceBundle will instantiate a LoggerBundle with 
non-null rbname and RB.  It wasn't obvious to me what does what.

2137             final String rbName = getResourceBundleName();
2138             if (!SYSTEM_LOGGER_RB_NAME.equals(rbName)
2139                     && !SYSTEM_LOGGER_RB_NAME.equals(b.getBaseBundleName())) {
2140                 return LoggerBundle.get(rbName, b);
2141             } else {
2142                 return SYSTEM_BUNDLE;
2143             }

To get to the above line, either lb.userBundle is null or 
getResourceBundle() isoverridden.  L2126 already checks that this logger 
does not associate with the system rbname.  It seems to me that the 
special case here is when the Logger subclass overrides 
getResourceBundleName() to returnSYSTEM_LOGGER_RB_NAME or override 
getResourceBundle to return the "sun.util.logging.resources.logging" RB. 
I tink that we might just simplify the above and just return 
LoggerBundle.get(rbName, b) and no need to worry about those overridden 
case which does no use to such a Logger subclass with unmatched rbname 
and RB.  Same comment applied to L2157-2165.

thanks
Mandy