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

[Daniel Fuchs]

Hi,

Here is a revised patch that introduces a new
<logger>.handlers.ensureCloseOnReset=true|false
property.

This property can be used as a fallback to turn the new behavior
off in the improbable case where an application may need to do
so. I have updated the LogManager class-level API documentation.

http://cr.openjdk.java.net/~dfuchs/webrev_8060132/webrev.02/

best regards,

-- daniel

On 24/10/14 23:50, Daniel Fuchs wrote:
> 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
>