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

[Daniel Fuchs]

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
>