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

[Peter Levart]

Hi Daniel,

Here it is:

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

All java/util/logging tests pass for me except 
java/util/logging/TestLoggerWeakRefLeak

#section:build
----------messages:(2/93)----------
command: build jdk.testlibrary.*
reason: User specified action: run build jdk.testlibrary.*
result: Not run. Test running...


test result: Error. Can't find source file: jdk/testlibrary/*.java in 
directory-list: /home/peter/work/hg/jdk9-dev/jdk/test/java/util/logging 
/home/peter/work/hg/jdk9-dev/jdk/test/lib/testlibrary


Regards, Peter


On 05/04/2015 04:09 PM, Peter Levart wrote:
>
>
> 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
>>>>
>>>
>>
>