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

[Peter Levart]

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