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
