RFR: 8023168 - Cleanup LogManager class initialization and LogManager/LoggerContext relationship
Daniel Fuchs
daniel.fuchs at oracle.com
Thu Sep 5 18:51:10 UTC 2013
New webrev.
Has Martin's comments + simplified Logger.getGlobal().
Rest left unchanged. Tests are running...
<http://cr.openjdk.java.net/~dfuchs/webrev_8023168/webrev.02/>
cheers,
-- daniel
On 9/5/13 8:16 PM, Daniel Fuchs wrote:
> Hi Peter,
>
> On 9/5/13 7:15 PM, Peter Levart wrote:
>> 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?
>
> Hmmmm... I think that's needed. I could try to take it off
> and see what breaks ;-)
> Will double check. An installed subclass of LogManager
> could potentially call methods on the default log manager
> without calling LogManager.getLogManager().
>
> public MyLogManager extends LogManager {
>
> static volatile MyLogManager INSTANCE;
> static synchronized boolean singletonCheck() {
> if (INSTANCE != null) {
> throw new UnsupportedOperationException();
> }
> return INSTANCE == null;
> }
> public MyLogManager() {
> this(checkSingleton());
> }
> private MyLogManager(boolean first) {
> synchronized(MyLogManager.class) {
> if (INSTANCE == null) INSTANCE = this;
> }
> }
>
> public static getInstance() {
> return INSTANCE;
> }
> }
>
> Presumably if we have -Djava.util.logging.manager=MyLogManager
> then main() could do:
>
> main() {
> MyLogManager.getInstance().getLogger("");
> }
>
>
>> - 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:
>
> hmmmm... you're almost convincing me...
>
>>
>> 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.
>
> So the only possible path would be that of the subclass as outlined
> above. hmmmm... does it deserve the additional complexity?
>
> On the other hand - LogManager can be subclassed - so a subclass could
> override any method in LogManager - for instance addLogger - and
> could legitimately want to call LogManager.getLogManager() within
> the overridden method.
> This would cause a recursion.
>
> So I think we need the recursion break - even if we manage to find a
> way to eliminate direct recursive calls from the standard classes.
>
> I believe I'd prefer to play it safe and keep the guards as well as
> the call to ensureLogManagerInitialized() from
> requiresDefaultLoggers().
>
> However - I will need to reexamine the case of getGlobal().
> As you can imagine I didn't write this in one step.
> It was a very iterative process.
> The changes I put in getGlobal() were required to fix an intermittent
> test failure in TestGetGlobalConcurrent - which happened when
> several threads called Logger.getGlobal() concurrently.
>
> What happened was that Thread #1 would see global.manager null
> and would call LogManager.getLogManager(). LogManager.getLogManager(),
> at some point will set global.manager - but this happens before
> LogManager.getLogManager() is finished (in fact the assignement
> global.manager = LogManager.getLogManager();
> is redundant. It's here only for aesthetical reasons.
>
> So at this point Thread #2 could come - see that global.manager is not
> null, and return global. But global was not fully initialized yet.
>
> Actually - I wrote getGlobal() in this way because at the time I felt
> it made more sense - the reader would feel he understood what is
> happening.
> Now I realize it was a mistake because it causes the reader to make
> assumptions that are false.
>
> One way to simplify getGlobal() would be to implement it as follows:
>
> public static final Logger getGlobal() {
> LogManager.getLogManager() // has the side effect
> // of finishing to initialize global - or
> // wait until it is fully initialized if
> // it's beeing initialized by another thread.
> // Note that it is absolutely mandatory to call
> // LogManager.getLogManager() unconditionally
> // here in order to avoid race conditions and
> // obtain a fully initialized global logger.
>
> // here global will be fully initialized.
> return global;
> }
>
> That's what I should have done. I will do it.
>
> -- daniel
>
>>
>> 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
>>>>
>>>
>>
>
More information about the core-libs-dev
mailing list