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

[Daniel Fuchs]

Hi Peter,

Sorry I didn't reply to your last mail.
I still intend to.

more on the current mail below...

On 30/04/15 13:08, Peter Levart wrote:
>
>
> On 04/28/2015 05:46 PM, 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/
>>
>>
>> Regards, Peter
>>
>
> Hi Daniel,
>
> Just some observations...
>
> I noticed you changed LogManager.Cleaner superclass from
> ManagedLocalsThread -> Thread. Is that intentional?

I didn't -

http://cr.openjdk.java.net/~dfuchs/webrev_8077846/webrev.00/

but you did :-)

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

The repo on which I prepared the patch didn't have this change yet.
It's probably the result of a bad merge when you upgraded/applied
my patch.


> The Cleaner is an
> unstarted Thread that gets registered as a shutdown hook in LogManager
> constructor. So at LogManager construction time it gets hold on
> inheritable thread locals and keeps them for the entire JVM lifetime. Do
> we have control on what thread performs LogManager construction and that
> no inheritable thread-local leaks are possible?
>
> The other thing to note is that Cleaner class is not static, so it has
> an implicit reference to LogManager instance. So each LogManager
> instance constructed during lifetime of JVM is actually retained for the
> entire JVM lifetime.

Yes. I would say that's not an issue because the LogManager is supposed
to be a singleton class. There should be only one instance.
This is sadly not enforced and there are applications out there
that have taken that opportunity to create several LogManagers - but
that is not actually supported (we just try to not cause regressions).

> The use of shutdown hook to call reset() directly is questionable. Why?
> Because other shutdown hooks might want to log their shutdown processing
> and they will race with LogManager shutdown hook executing reset(),
> shutting down all log handlers and effectively preventing shutdown
> process form being logged. Why is it so important to reset()
> LogManager(s) before exiting VM?

It is questionable, agreed.
And there are applications which subclass
LogManager just for the purpose of preventing reset() from doing
anything - so that they can use loggers in other shutdown hook.
There's even an issue in JBS.

The problem here is to cleanly close the handlers when the VM exits
(there are locks to be released and XML tails to be written).

Using a mechanism similar to sun.misc.Cleaner in order to
close handlers when they are no longer referenced is
unfortunately not trivial, because you precisely need a
reference to the Handler in order to call close() on it.
And because of compatibility issues we're more or less stuck
with the current Cleaner thread anyway.

best regards

-- daniel

>
> Regards, Peter
>