RFR: 8060132: Handlers configured on abstract nodes in logging.properties are not always properly closed
Daniel Fuchs
daniel.fuchs at oracle.com
Tue Nov 4 18:35:10 UTC 2014
On 04/11/14 18:54, Stanimir Simeonoff wrote:
> 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).
Hi Stanimir,
Good points.
I was thinking of adding a default value for ensureCloseOnReset in the
property file - rather than hardcoding it to 'true'.
Something like:
final boolean defaultValue =
getBooleanProperty("java.util.logging.ensureCloseOnReset", true);
final boolean ensureCloseOnReset = names.length > 0
&& getBooleanProperty(handlersPropertyName + ".ensureCloseOnReset",
defaultValue);
So 'true' would instead be the default of the default...
-- daniel
>
> Stanimir
>
>
> On Tue, Nov 4, 2014 at 6:48 PM, Daniel Fuchs <daniel.fuchs at oracle.com
> <mailto: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/
> <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
> <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
>
>
>
>
More information about the core-libs-dev
mailing list