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.fu...@oracle.com
<mailto: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/
<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
