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

[Daniel Fuchs]

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
>