Hi Mandy,
Thanks for the feedback!
On 9/6/13 4:28 AM, Mandy Chung wrote:
On 9/5/2013 11:51 AM, Daniel Fuchs wrote:
<http://cr.openjdk.java.net/~dfuchs/webrev_8023168/webrev.02/>
Logger.java - I like this and much cleaner fix.
251 // because global.manager will become not null somewhen
during
252 // the initialization of LogManager.
s/somewhen/somewhere/
OK
The comment for the setLogManager method needs updated since it's
called when a logger is registered to a log manager.
347 // It is called from the LogManager.<clinit> to complete
348 // initialization of the global Logger.
Ah. Right.
The checkPermission() method has this code to deal with null manager:
if (manager == null) {
// Complete initialization of the global Logger.
manager = LogManager.getLogManager();
}
I think it's still possible that checkPermission is called with
null manager, right? e.g. calling Logger.global.setFilter
Should this simply call LogManager.getLogManager() unconditionally as
in getGlobal()?
I don't think this would be required.
Maybe we could make checkPermission() static in LogManager?
But we might still need to call LogManager.getLogManager() to avoid
regression
in code using Logger.global directly...
LogManager.java - the initialization is really tricky and the discussion
on the ensureLogManagerInitialized is very useful.
159 private volatile Logger rootLogger; // non final field - make
it volatile to
160 // make sure that other threads will see the new
value once
161 // ensureLogManagerInitialized() has finished executing.
suggest to move the comments above the declaration
OK
Maybe renaming "initializedCalled" to "recursiveInit" to make the 2
variables easy to tell. space missing around "=" in line 270, 271
310 // Read configuration. This was
previously triggered
311 // by the new RootLogger() constructor -
but no longer.
Previously readPrimordialConfiguration() wascalled by
LogManager.getLogManager
that was triggered when instantiating the RootLogger - to be precise.
It seems that leaving out the history might cause less confusion
since it no longer calls it.
OK
760 final LogManager owner = getOwner();
761 logger.setLogManager(owner);
Should this have an assert to ensure logger.manager == null or == owner?
We don't expect a Logger to change its owner, do we? The behavior of
multiple
LogManager instances is not specified anyway.
I am concerned it could introduce regressions in applications that
use multiple instances of LogManager or subclasses of Logger.
I agree this is not perfect. Unfortunately I don't see any ideal solution.
Reading the RootLogger - looks like it's potential for cleanup.
Logger.global uses a private constructor that doesn't call
LogManager.getLogManager to break the cyclic dependency. Perhaps
RootLogger should do the same? It's fine to leave it for future
if you prefer since you have well tested the bits you have.
It does. I mean it no longer uses the two args constructor that calls
LogManager.getLogManager().
Mandy
