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

Daniel Fuchs daniel.fuchs at oracle.com
Mon May 4 13:46:32 UTC 2015


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.

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