Hi Mandy,
I have updated the webrev taking your comments into account.
<http://cr.openjdk.java.net/~dfuchs/webrev_8029281/webrev.02/>
Changes are small and outlined below:
On 12/3/13 1:49 AM, Mandy Chung wrote:
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.
I believe that nulling the instance variables in LoggerWeakRef
is probably no longer needed - since we now should be calling
dispose only once - but I didn't want to change the code too
much. I agree it would be better to revisit that in 9.
Logger.java
The assert this.loggerBundle == NO_RESOURCE_BUNDLE; in the Logger
constructors doesn't seem to be needed.
OK - removed.
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.
Right. Let's keep the old name for now. We can revisit the naming in 9.
You right that when we reach the last line lb.userBundle must be null,
otherwise we would have either returned or thrown an exception, since
lb.userBundle != null => lb.resourceBundleName != null.
Therefore I added an assert lb.userBundle == null and
replaced the call to
LoggerBundle.get(name, lb.userBundle)
by
LoggerBundle.get(name, null)
Hopefully it will make what happens more obvious.
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.
Ah. I'm glad to hear that. This is a great simplification.
done.
-- daniel
thanks
Mandy
