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