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

[Peter Levart]

On 09/05/2013 10:08 AM, Daniel Fuchs wrote:
>> 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,

The fact that they are synchronized on LoggerContext does not prevent 
publication of rootLogger to them without write-read edge, since writing 
to rootLogger field is performed without synchronization on the same 
LoggerContext (it is performed with synchronization on the LogManager 
instance)...

> 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.

Ok.

> Threads that call ensureLogManagerInitialized() after that will find 
> the flag
> is true before entering the synchronized block and will return directly.

...and such threads can see rootLogger field being either still null or 
not null but the Logger instance that is obtained via such data race can 
be half-initialized.

> 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.


I think volatile rootLogger field is required for correct publication of 
RootLogger instances if double-checked locking is used...


Regards, Peter