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

[Daniel Fuchs]

Hi Peter,

On 28/04/15 16:07, Peter Levart wrote:

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

And I believed I did pay attention ;-)

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

Hey - very good catch! Argh.
I believe you're right. I should have used two nested
try finally block.

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

hmmm... I'm not sure it really simplifies things. Though I have to admit
that an initial state value 0 is appealing.
I was hopping to avoid a multi state int value - I still have
a preference for the 2 booleans...

>
> Here's my attempt at simplifying this:
>
> http://cr.openjdk.java.net/~plevart/misc/LogManager.synchronization/webrev.01/

LogManager can be subclassed, and subclasses may override reset() for
different purposes.
So I'm afraid the Cleaner thread still needs to call te public
reset() method. The same unfortunately applies to
readConfiguration().

best regards,

-- daniel

> So what do you think?
>
> Regards, Peter
>
>
>
>>
>> best regards,
>>
>> -- daniel
>>>
>>> Regards, Peter
>>>
>>
>