RFR: 8023168 - Cleanup LogManager class initialization and LogManager/LoggerContext relationship
Daniel Fuchs
daniel.fuchs at oracle.com
Thu Sep 5 18:16:08 UTC 2013
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