RFR: 8023168 - Cleanup LogManager class initialization and LogManager/LoggerContext relationship

[Peter Levart]

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