On 09/04/2013 11:36 PM, David M. Lloyd wrote:
On 09/04/2013 02:56 PM, Daniel Fuchs wrote:
Hi,
Please find below a changeset that will fix
8023168: Cleanup LogManager class initialization and
LogManager/LoggerContext relationship.
<http://cr.openjdk.java.net/~dfuchs/webrev_8023168/webrev.00/>
LogManager class initialization is fragile: because Logger
constructor calls LogManager.getLogManager, and because
LogManager calls new Logger during LogManager class initialization,
and because the global logger is instantiated during Logger class
initialization, loading Logger.class or LogManager.class can lead to
challenging hard to diagnose issues.
As far as calling initialization ("ensureLogManagerInitialized()"),
it's a shame that checking for a one-time action has to run through a
synchronization block every time. Maybe a "lazy holder" class would
be more appropriate here, especially given that the point seems to be
to produce the RootLogger() instance which doubles as the indicator
that initialization was done.
Or, without using yet another class, double-checked locking, like:
private volatile boolean initializedCalled;
final void ensureLogManagerInitialized() {
if (initializedCalled) {
return;
}
synchronized (this) {
final LogManager owner = this;
// re-check under lock
if (initializedCalled || owner != manager) {
// we don't want to do this twice, and we don't want to do
// this on private manager instances.
return;
}
try {
AccessController.doPrivileged(....);
} finally {
initializedCalled = true;
}
}
}
... the code in PrivilegedAction that does the initialization, that is,
the following statements:
owner.readPrimordialConfiguration();
* owner.rootLogger = owner.new RootLogger();*
owner.addLogger(owner.rootLogger);
final Logger global = Logger.global;
owner.addLogger(global);
...almost all of them (except assignment to rootLogger) by themselves
ensure that the state mutations they make are published to other
threads, so if also *rootLogger* field was made volatile, such
double-checked locking would presumably not be broken.
One observation on method readPrimordialConfiguration() - this method
performs similar double-checked locking initialization, but I think it
sets the *readPrimordialConfiguration* flag to soon. I think it should
do it in a final block of a try statement, like presented initialization
above.
Regards, Peter
