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

[Peter Levart]

On 05/04/2015 03:46 PM, Daniel Fuchs wrote:
> On 04/05/15 15:26, Peter Levart wrote:
>> 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.
>
> That looks better.
>
> I would consider removing these lines:
>
> 1333             if (globalHandlersState == STATE_SHUTDOWN) {
> 1334                 // already in terminal state
> 1335                 return;
> 1336             }
>
> in favor of:
>
> 1354             } else if (globalHandlersState != STATE_SHUTDOWN) {
> 1355                 // ...user calling reset()...
>
> and let the reset reset twice if it's called from different contexts.
> There may be permissions that could prevent a user-called reset() to
> close all handlers, and we don't want that to prevent the Cleaner-called
> reset to come afterwards and perform its job.

Then we don't need STATE_SHUTTING_DOWN. Cleaner.run() can change 
directly to STATE_SHUTDOWN and reset() would just leave it there, but 
perform the reset anyway.

Will create another variant including your test too.

Regards, Peter

>
> BTW - in the end - one of us will need to push this together with
> the new test :-)
>
> cheers,
>
> -- daniel
>
>
>>
>> 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
>>>
>>
>