Hi,
Here is the new patch. It uses an additional volatile flag
to avoid synchronizing once the log manager is fully initialized.
I have added some comments to spell out the purpose of the flags,
as well as some assertion that should make it clear what is expected,
what is possible, and what is not.
I have also implemented the other remarks from David, Peter, and Mandy.
<http://cr.openjdk.java.net/~dfuchs/webrev_8023168/webrev.01/>
Testing is still under way - but the results so far are looking good.
I also ran the JCK for api/java_util - nothing jumped at my face.
I have modified my NetBeans configuration to run NetBeans 7.3 over
my private build - and I'm not seeing any troubles either so far.
(I had done that with the previous patch too)
best regards,
-- daniel
On 9/5/13 2: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.
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)
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
