RFR: 8077846: improve locking strategy for readConfiguration(), reset(), and initializeGlobalHandlers()

[Peter Levart]

On 04/28/2015 10:13 AM, Daniel Fuchs wrote:
> On 28/04/15 09:59, Peter Levart wrote:
>> On 04/27/2015 10:05 PM, David Holmes wrote:
>>>>>>> The patch proposes to use a Reantrant lock to deal with
>>>>>>> configurations changes in reset() and readConfiguration(),
>>>>>>> and avoids lock contention in initializeGlobalHandlers()
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~dfuchs/webrev_8077846/webrev.00/
>>>>>>
>>>>>> How early in VM startup can logging be initialized and used? I just
>>>>>> wonder whether the use of ReentrantLock changes that at all?
>>>>>
>>>>> The initialization will not happen before the first class calls
>>>>> Logger.getLogger() or LogManager.getLogManager().
>>>>> In particular, it does not happen when the LogManager.class is loaded
>>>>> (that is: it no longer happens as  part of the class static
>>>>> initialization).
>>>>>
>>>>> I see Alan replied to that question as well...
>>>>
>>>> I'm just wondering what anyone might be running with a customized core
>>>> class that uses logging, and which may now fail due to the different
>>>> requirements.
>>>
>>> But then I just recalled that we used to use ReentrantLock via
>>> ConcurrentHashMap in Class, so introducing it early is not likely to
>>> cause any issue.
>>>
>>> Cheers,
>>> David
>>
>> Hi David, Daniel,
>>
>> CHM in JDK8 is not using ReentrantLock. But in JDK7, it is used, yes.
>>
>> But anyway, is there a special reason to use ReentrantLock here in
>> LogManager instead of Java built-in monitor lock? It is reentrant too.
>
> Hi Peter,
>
> That's one of the possibility I was envisaging - but I didn't feel very
> comfortable with synchronizing the whole method bodies.

Hi Daniel,

There is a synchronized (lock) { ... } syntax too.

>
> Using a reentrant lock also offers more possibility for evolutions
> like try locking or timeout locking - which you can't do with a
> monitor (unless you reinvent ReentrantLock)...

Ok, fair enough.

Now that I studied the synchronization in more detail, I must say, huh, 
it's very tricky. The state is composed of 3 boolean flags (that's 
theoretically 8 states) that are sometimes modified in pairs under lock 
and sometimes individually under lock and sometimes (shutdown hook) not 
under lock. So one has to pay attention to all possible interleavings.

Anyway, I think I found a bug (not synchronization related, but 
re-entrancy related). In initializeGlobalHandlers() In line 1629 you 
check if initializeGlobalHandlersCalled is true and return (a recursive 
call), but you do this inside the try block and therefore finally block 
is executed which sets initializeGlobalHandlersDone to true. This is 
prematurely, as loadLoggerHandlers might not be finished yet.

I think all possible states can be compressed into just 4 and this can 
be an int so that it is always updated atomically (easier to reason 
about). This way you can also get initial state to be default value (0).

Here's my attempt at simplifying this:

http://cr.openjdk.java.net/~plevart/misc/LogManager.synchronization/webrev.01/

So what do you think?

Regards, Peter



>
> best regards,
>
> -- daniel
>>
>> Regards, Peter
>>
>