On 9/5/13 8:07 AM, Peter Levart wrote:
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;
}
}
}
Ah! Thanks for that comment Peter - that's exactly what I had in mind.
... 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.
Hmmm... By that I assume you mean the operations that access
LogManager.rootLogger
from LoggerContext. AFAIR all these operations are synchronized on
LoggerContext,
and will trigger a call to ensureLogManagerInitialized().
ensureLogManagerInitialized() is called for every add/get logger operation.
If ensureLogManagerInitialized() is executing other threads will be
blocked.
When it finishes - threads that were waiting on the monitor will find the
initializationCalled flag set to true and will simply return.
Threads that call ensureLogManagerInitialized() after that will find the
flag
is true before entering the synchronized block and will return directly.
I can make rootLogger volatile. I'm not 100% sure it is required - but I
think
it is more correct, and as such it will make the code more maintainable.
Good point. It's probably something I would have overlooked.
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.
Yes. I know this is broken - and I believe I have seen a defect that
points that out.
I'd prefer not to change that in this changeset though.
best regards
-- daniel
Regards, Peter
