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

[Peter Levart]

Hi Daniel, Mandy,

What about the following:

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

You see, no boolean flags needed. The globalHandlersState is always 
changed atomically within a locked region, so a graph of transitions can 
be drawn:

http://cr.openjdk.java.net/~plevart/misc/LogManager.synchronization/LogManager.globalHandlersState.png

STATE_INITIALIZING and STATE_READING_CONFIG are intra-locked-region 
states used to prevent infinite recursion in initializeGlobalHandlers() 
and communicating state from within locked-region of readConfiguration() 
to nested reset(), respectively, but never from one locked region to 
another.

Regards, Peter

On 05/01/2015 02:03 AM, Peter Levart wrote:
>
>
> On 04/30/2015 04:42 PM, Daniel Fuchs wrote:
>> On 28/04/15 17:46, Peter Levart wrote:
>>>
>>>
>>> On 04/28/2015 04:57 PM, Daniel Fuchs wrote:
>>>>> 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
>>>
>>> Um, yes of course. This can be fixed:
>>>
>>> http://cr.openjdk.java.net/~plevart/misc/LogManager.synchronization/webrev.02/ 
>>>
>>
>> Hi Peter,
>>
>> Sorry for the late reply.
>>
>> My gut feeling is that I dislike multi-state ints.
>> But I guess that's a matter of taste - so I could probably
>> overcome it ;-)
>
> Isn't that a simplification (of reasoning)? In particular if 
> individual boolean flags are read and/or written without holding any lock.
>
>>
>> What makes me less happy is that I had managed to
>> remove the explicit synchronized() { } block in the
>> Cleaner thread - and I now see it's back.
>>
>> Could we maybe keep the imminentDeath boolean and remove
>> the explicit locking in the Cleaner thread?
>>
>> I mean... imminentDeath should be checked from within
>> the lock - before doing anything. But it does not need
>> to be set from within a locked section.
>>
>> best regards,
>>
>> -- daniel
>>
>
>
> Hi Daniel, Mandy,
>
> Explicit locking in Cleaner is something that is performed in reset() 
> anyway, so getting rid of it is not actually getting rid of it, as 
> Cleaner is calling reset() anyway. The lock is reentrant.
>
> But If you want get rid of it so that reset() is not called under lock 
> held because reset() might be overridden, then imminentDeath must return.
>
> @Mandy:
>
> In webrev.01 we had a private reset(int newState), called from 
> reset(), Cleaner and readConfiguration(), but Daniel then spotted that 
> reset() is an overridable method, so it has to be called from Cleaner 
> and readConfiguration() too, unfortunately.
>
> The STATE_XXX names are best depicted if looking at code in 
> initializeGlobalHandlers() method.
>
> Let me prepare a changed webrew...
>
> Regards, Peter
>