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
