Hi Mandy,
Is this only problem to the abstract node of Loggers (i.e. the ancestor
is in the logging configuration but the library/app never creates such
a logger)?
No - the problem is with any logger that has handlers configured from
the logging.properties, and which the application creates and releases
before the Cleaner thread is run.
Your patch will keep all loggers defined in the configuration strong
reference.
Not all loggers defined in the configuration. Only those loggers for
which a handler and is explicitly defined in the configuration and
which have been created at least once - directly or indirectly (=
through a child) by the application.
Would that cause memory leak?
The main source of memory leak - and probably the only one we should
care about is when a Logger that we strong reference has a reference to
an object whose class would be otherwise garbage collected.
In other words - if the logger is the only thing that prevents an
object, and
its class, and its class loader, from being garbage collected.
If I'm not mistaken - there are only three objects that could cause such
an issue:
- Instances of Level
- Instances of Handler
- Instances of ResourceBundle.
Instances of Level are already held by Level.knownLevel - this is a
known bug,
and until that is fixed Logger won't be the only thing preventing a custom
instance of Level from being garbage collected.
In addition such a custom instance of Level (an instance of a custom
Level subclass)
can't be set directly from the configuration - so this can only happen
if the
application has set this level programmatically on the Logger.
If I remember well Handlers created from the configuration are looked up
from the
System ClassLoader - so that shouldn't cause a memory leak either.
Remains resource bundles - which might be an issue.
However - the set of loggers for which a Handler is configured from the
configuration file is by nature bounded. Therefore - it is my opinion that
the proposed patch does not create a memory leak per-se:
A memory leak can only happen if an application constantly (and
programmatically) adds new Handler to the existing logger, believing
that it will get a new instance of Logger each time because the old
one should have been gc'ed.
In that case - I believe the fault would be in the application, not in the
Logger...
If you think that we should still leave the door open for an
application to explicitly request the old behavior - we could
define a new property for the configuration file.
e.g. something like:
<logger-name>.handlers = <handler list> // no changes here
<logger-name>.handlers.ensureCloseOnReset = false // new property
// True by default when a logger has handlers configured from
logging.properties.
// Can be explicitly turned off for a given logger, in the
configuration.
// Ignored if the logger has no handler configured.
// Not inherited by child loggers
If <logger-name>.handlers.ensureCloseOnReset is explicitly set to
false then the LogManager will not add the logger to the
persistentLoggers list.
Do you think we should do that?
best regards,
-- daniel
On 10/24/14 8:31 PM, Mandy Chung wrote:
On 10/10/2014 8:39 AM, Daniel Fuchs wrote:
http://cr.openjdk.java.net/~dfuchs/webrev_8060132/webrev.01
Sorry for the delay. I have been pondering if there is a better
alternative and looks like you have considered a few other options
that none of them is a good one.
typos:
174 explicitely
925: persitentLoggers
Is this only problem to the abstract node of Loggers (i.e. the ancestor
is in the logging configuration but the library/app never creates such
a logger)?
Your patch will keep all loggers defined in the configuration strong
reference. What if the application really wants the loggers say
com.foo.FooLogger to get GC'ed (e.g. it wants the logger class and
its defining class loader to be garbage collected) but the configuration
does name com.foo.FooLogger.handler = ....
Would that cause memory leak?
Mandy
