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
