Re: [jonas-team] Monolog on gcj

[Vadim Nasardinov]

On Thursday 18 August 2005 18:07, Florent BENOIT wrote:
>  I applied the change (+ some doc) in the current CVS head of
>  Monolog. 
The patch is a tad suboptimal.

http://cvs.forge.objectweb.org/cgi-bin/viewcvs.cgi/monolog/monolog/src/or...


The reason java.util.logging.LogManager only keeps weak references to
the loggers it manages is in order to avoid a resource leak.  If the
application chooses not to retain a hard reference to a logger, then
the LogManager should not prevent the unneeded logger from getting
GC-ed.  Makes sense.

The exact same logic applies to Monolog's LoggerFactory.  It should
not keep hard references to loggers it creates.  It is up to the
calling application to retain a reference if the app wishes to avoid
the cost of creating a new logger instance every time it needs one.

Where does that leave us?  Andrew ran into the above problem in an old
version of Carol.  Recent versions do not have this problem for the
simple reason that Carol no longer uses Monolog.  It's been
Monolog-free since March 15 (good job, Florent!):

  http://www.objectweb.org/wws/arc/carol-commits/2005-03/msg00078.html

However, if we want to patch Monolog for those remaining parts of
Jonas that continue to depend on it, then someone should take a closer
look at whether or not the following class should be responsible for
retaining hard references to loggers it creates:

  org.objectweb.util.monolog.file.monolog.PropertiesConfAccess

This responsibility should most certainly not be born by the
LoggerFactory class.



>  Andrew Haley wrote: 
...
>  > Logger getLogger(String name)
>  >
>  > but LogManagers only keep weak references to their logs.  So, as
>  > soon as a log has been created you need to keep a hard reference
>  > to it.
>  >
>  > In Monolog there is
>  > org.objectweb.util.monolog.wrapper.javaLog.LoggerFactory, and that
>  > uses a LogManager to keep track of the instances of Logger it
>  > creates. 
...
>  >---
> src/org/objectweb/util/monolog/wrapper/javaLog/LoggerFactory.java~
>  >+++ src/org/objectweb/util/monolog/wrapper/javaLog/LoggerFactory.java
>  >@@ -45,6 +45,8 @@
>  >         */
>  >        protected static Logger rootLogger = null;
>  > 
>  >+  private ArrayList loggers = new ArrayList();
>  >+
>  >     /**
>  >      * This static code initialize the value of the variable defined into
>  >      * BasicLevel.
>  >@@ -145,6 +147,7 @@
>  >         if (o == null) {
>  >             // It doesn't exist => creates and adds it
>  >             Logger result = new Logger(name, resName);
>  >+           loggers.add(result);
>  >             Monolog.debug("Instanciate the logger " + name);
>  >             manager.addLogger(result);
>  >             return result;

[jonas-team] Monolog on gcj

[Andrew Haley]

Vadim Nasardinov writes:
>  On Thursday 18 August 2005 18:07, Florent BENOIT wrote:
>  > I applied the change (+ some doc) in the current CVS head of
>  > Monolog.
>  
>  The patch is a tad suboptimal.
>
>
> http://cvs.forge.objectweb.org/cgi-bin/viewcvs.cgi/monolog/monolog/src/or...
>  
>  
>  The reason java.util.logging.LogManager only keeps weak references to
>  the loggers it manages is in order to avoid a resource leak.  If the
>  application chooses not to retain a hard reference to a logger, then
>  the LogManager should not prevent the unneeded logger from getting
>  GC-ed.  Makes sense.
>  
>  The exact same logic applies to Monolog's LoggerFactory.  It should
>  not keep hard references to loggers it creates.  It is up to the
>  calling application to retain a reference if the app wishes to avoid
>  the cost of creating a new logger instance every time it needs one.
>  
>  Where does that leave us?  Andrew ran into the above problem in an old
>  version of Carol.  Recent versions do not have this problem for the
>  simple reason that Carol no longer uses Monolog.  It's been
>  Monolog-free since March 15 (good job, Florent!):
>  
>    http://www.objectweb.org/wws/arc/carol-commits/2005-03/msg00078.html
>  
>  However, if we want to patch Monolog for those remaining parts of
>  Jonas that continue to depend on it, then someone should take a closer
>  look at whether or not the following class should be responsible for
>  retaining hard references to loggers it creates:
>  
>    org.objectweb.util.monolog.file.monolog.PropertiesConfAccess
>  
>  This responsibility should most certainly not be born by the
>  LoggerFactory class. 
I don't think that makes sense, because PropertiesConfAccess might
well die before the clients call LoggerFactory.getLogger().  

The method getLoggerFactory() calls PropertiesConfAccess.load(), which
does

        new PropertiesConfAccess().read(prop, mf, mf, mf);

so, the instance of PropertiesConfAccess is dead even before the
method LoggerFactory.getLogger() returns.  

Andrew.

[jonas-team] Monolog on gcj

[Vadim Nasardinov]

On Friday 19 August 2005 11:20, Andrew Haley wrote:
>   > This responsibility should most certainly not be born by the
>   > LoggerFactory class.
>  
>  I don't think that makes sense, because PropertiesConfAccess might
>  well die before the clients call LoggerFactory.getLogger().
>  
>  The method getLoggerFactory() calls PropertiesConfAccess.load(),
>  which does
>  
>          new PropertiesConfAccess().read(prop, mf, mf, mf);
>  
>  so, the instance of PropertiesConfAccess is dead even before the
>  method LoggerFactory.getLogger() returns. 
This doesn't change the fact that LoggerFactory is the wrong place to
intervene.  All this says is that the PropertiesConfAccess (PCA)
instance may have to be kept undead.  Or, it says that the PCA, before
dying a quiet death, has to return the newly instantiated collection
of loggers to whoever made the call to the PCA so that they -- the
caller -- can decide whether or not they want to keep said loggers
around.

Whatever the right patch turns out to be, the one we have on the table
right now is certainly not it.

That said, I don't really care if Monolog leaks memory.  If everyone
else is happy with the patch, then so be it.


P.S. I've removed fedora-devel-java-list from the Cc list to avoid
followups that seem be off-topic for fedora-java.