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.fu...@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 >>> >> >> >
