RFR: 8060132: Handlers configured on abstract nodes in logging.properties are not always properly closed

[Daniel Fuchs]

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