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

[Peter Levart]

On 09/05/2013 02:43 PM, Daniel Fuchs wrote:
> On 9/5/13 1:51 PM, Peter Levart wrote:
>>> 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 don't think so, because the flag isn't set to true until
> the initialization is finished.
>
> BTW - there was a reason for having both an initializationCalled
> *and* a synchronized at method level. The purpose of
> initializationCalled flag was to prevent infinite recursion in the
> same thread - because addLogger within ensureLogManagerInitialized
> will trigger a new call to ensureLogManagerInitialized that the
> synchronized won't block - since it's in the same thread.

Oh, I see. The ensureLogManagerInitialized() is also called from 
requiresDefaultLoggers() which is called from addLocalLogger(Logger) 
which is called from addLogger(Logger) which is also called from 
ensureLogManagerInitialized().

I suspect that calling ensureLogManagerInitialized() from 
requiresDefaultLoggers() is not needed (any more). Why?

- the ensureLogManagerInitialized() is only meant for the global 
LogManager (LogManager.manager). It has no effect on private LogManagers.
- almost all accesses to the global LogManager are done via 
LogManager.getLogManager() which already calls 
ensureLogManagerInitialized() before returning the LogManager.manager 
instance.
- the only other places where private LogManager.manager field is 
accessed directly are:
     - in the ensureLogManagerInitialized() method itself to skip 
initialization for private LogManagers
     - in the LoggerContext.requiresDefaultLoggers() to check if the 
owner is the global LogManager (an reference equality check)
     - in the Cleaner.run() to prevent premature GC of global LogManager

All those direct LogManager.manager field usages do not call any methods 
on the so obtained global LogManager, so I conclude that any code that 
calls any instance method on global LogManager must have obtained it 
from the LogManager.getLogManager() method and this method already 
ensures that it is initialized.

If you remove a call to ensureLogManagerInitialized() from 
LoggerContext.requiresDefaultLoggers() then there is one less 
possibility for recursion. The other is in Logger.getGlobal(). Is this 
needed? I think not:

     public static final Logger getGlobal() {

         if (global != null && global.manager == null) {
             global.manager = LogManager.getLogManager();
         }

         if (global.manager !=  null) {
             global.manager.ensureLogManagerInitialized();
         }

         return global;
     }


Static initialization of Logger class now has no dependency on 
LogManager, so there should be no recursion in static initialization. 
The 1st call to Logger.getGlobal() should find global != null and 
global.manager == null and so LogManager.getLogManager() should be 
called but that call does not recurse because LogManager obtains the 
global Logger directly from the deprecated Logger.global field, so the 
global.manager should be assigned with the initialized global 
LogManager. No need to call global.manager.ensureLogManagerInitialized() 
later.

The method could simply be:

     public static final Logger getGlobal() {

         if (global.manager == null) {
             global.manager = LogManager.getLogManager();
         }

         return global;
     }


Now that both recursive calls to ensureLogManagerInitialized() are 
removed, there's no need to check for recursive invocation in the 
ensureLogManagerInitialized() and initializationCalled flag can be removed.


>
> So I need in fact two flags: one initializationDone - which will
> be volatile - the other initializationCalled - which doesn't need
> to be since it will be always written/read from within the
> synchronized block.
>
> I toyed with the idea of using a single volatile three valued byte
> instead:
>
> initialization: 0 => uninitialized, 1 => initializing, 2 => initialized
>
> I opted for the two booleans - but the three valued byte might be more
> efficient:
>
> 1)
>
> volatile byte initialization = 0;
>
> ensureLogManagerInitialized() {
>   if (initialization == 2) return; // already done
>   synchronized(this) {
>     if (initialization > 0) return;
>     initialization = 1; // avoids recursion
>     try {
>       // do stuff which might recurse on
>       // ensureLogManagerInitialized()
>     } finally {
>       initialization = 2; // mark as done
>     }
>   }
> }
>
> vs 2)
>
> volatile boolean initializationDone = false;
> boolean initializationCalled = false;
>
> ensureLogManagerInitialized() {
>   if (initializationDone) return; // already done
>   synchronized(this) {
>     if (initializedCalled || initializationDone) return;
>     initializedCalled = true; // avoids recursion
>     try {
>       // do stuff which might recurse on
>       // ensureLogManagerInitialized()
>     } finally {
>         initializationDone = true; // mark as done.
>     }
>   }
> }
>
> Don't know if you have a preference for one or the other.
> My current impl (still under test) is 2)

After re-checking the JMM cookbook 
(http://gee.cs.oswego.edu/dl/jmm/cookbook.html), I see that you were 
right in the following:

volatile boolean initializationDone;
Logger rootLogger;

Thread1:

rootLogger = new RootLogger();
initializationDone = true;

Thread2:

if (initializationDone) {
    ...
    Logger logger = rootLogger;
    ...use logger...
}

A normal write followed by volatile write can not be reordered.
A volatile read followed by normal read can not be reordered either.

So if every read access of rootLogger field is performed via some call 
to LogManager instance method and an instance of global LogManager can 
only be obtained via LogManager.getLogManager() which calls 
ensureLogManagerInitialized(), the synchronization should be OK.


Regards, Peter

> best regards,
>
> -- daniel
>
>>
>>> 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
>>
>