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

[Stanimir Simeonoff]

Hi,

 917                 final boolean ensureCloseOnReset = names.length
== 0 ? false
 918                         : getBooleanProperty(handlersPropertyName
+ ".ensureCloseOnReset",
 919                                 names.length > 0);

I think the statement would be a lot easier to read as: final boolean
ensureCloseOnReset = names.length > 0  &&
getBooleanProperty(handlersPropertyName+ ".ensureCloseOnReset", true)

Also, probably adding a default  for ensureCloseOnReset (via system
property -Djava.util.logging.ensureCloseOnReset) would be less daunting to
revert to the default behavior than setting it in the concrete properties
files (that might be bundled a jar already).

Stanimir


On Tue, Nov 4, 2014 at 6:48 PM, Daniel Fuchs <daniel.fuchs at oracle.com>
wrote:

> 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
>>>
>>
>>
>