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

Daniel Fuchs daniel.fuchs at oracle.com
Mon May 4 18:52:48 UTC 2015


On 04/05/15 16:46, Peter Levart wrote:
> Hi Daniel,
>
> Here it is:
>
> http://cr.openjdk.java.net/~plevart/misc/LogManager.synchronization/webrev.04/

Looks good for me Peter :-)
Hopefully Mandy will like it too!

> 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

I assume you may have a somewhat older version of jtreg that
doesn't support the  .* pattern in @library / @build

I have imported your patch and tested locally - on my machine all
the logging tests have passed.
I have also run a test campaign against it and things looked good
there as well.

best regards,

-- daniel

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




More information about the core-libs-dev mailing list