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
