RFR: 8023168 - Cleanup LogManager class initialization and LogManager/LoggerContext relationship

[Daniel Fuchs]

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
>>
>