[logging] Static references to Log objects

[Brian Stansberry <be...@yahoo.com>]

>From the thread "Re: Idea: combine JCL 2.0 and UGLI in
Logging Services' CL2"...

--- Simon Kitching <sk...@apache.org> wrote:
<snip>

> Remy Maucherat wrote:
> > (suggesting it is a good idea, on any logging
> framework, to call
> > getLogger inside your app's critical path is quite
> funny).
> 
> Really? When getLogger can return different Log
> objects depending upon 
> the current context classloader? No class that could
> be deployed via a 
> shared classloader should cache a Log object in a
> static field...

Very true.  And since the intended users of JCL are
reusable components that could be deployed in a shared
classloader, your statement implies that in the uses
for which JCL is intended, no static references to Log
objects should be kept.

I think this point certainly deserves mention in the
JCL user guide (I'll submit a patch).


This line of thought led me to reconsider an idea I'd
rejected a couple weeks back related to the JCL memory
leak problem.  Basically the leak occurs if
LogFactoryImpl is defined by a parent classloader
while the class of one of its Log instances is defined
by a child loader.  The chain of references from the
Log instance to its classloader will prevent gc of the
child loader on undeploy.

This chain could (potentially) be broken if
LogFactoryImpl only held WeakReferences to its Log
instances as follows:

  public Log getInstance(String name) throws
LogConfigurationException {
-
-    Log instance = (Log) instances.get(name);
-    if (instance == null) {
-      instance = newInstance(name);
-      instances.put(name, instance);
+        
+    Log instance = null;
+    WeakReference ref = (WeakReference)
instances.get(name);
+    if (ref != null) {
+      instance = (Log) instances.get(name);
+      if (instance == null) {
+        instance = newInstance(name);
+        instances.put(name, new
WeakReference(instance));
+      }
   }
   return (instance);


I'd rejected this approach because:

1) It adds overhead to getInstance().
2) It adds a dependency on JDK 1.2 (although JCL has
others).
3) If calling code does not cache the Log object, it
may be recreated multiple times as the WeakReference
is cleared.  Don't know if this is a serious issue in
the real world.
4) Most importantly, this approach is based on the
idea that all objects holding a hard reference to a
Log will be gc'ed on undeploy, allowing the
WeakReference to clear.  This will fail if even 1
static reference to a Log whose class was defined by
the child loader is held somewhere.

So, negative performance implications + prone to not
working = bad idea.  But, Simon's comment on caching
Log objects in static fields led me to reconsider
enough to throw the concept out to the community for
comment.

Or maybe it's just that the memory leak issue is what
led me to wander into JCL-land in the first place and
now it's become my great white whale...

Brian


		
__________________________________ 
Do you Yahoo!? 
Make Yahoo! your home page 
http://www.yahoo.com/r/hs

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org

Re: [logging] Static references to Log objects

[Brian Stansberry <be...@yahoo.com>]

--- Brian Stansberry <be...@yahoo.com>
wrote:
<snip>
> 
> This line of thought led me to reconsider an idea
> I'd
> rejected a couple weeks back related to the JCL
> memory
> leak problem.  Basically the leak occurs if
> LogFactoryImpl is defined by a parent classloader
> while the class of one of its Log instances is
> defined
> by a child loader.  The chain of references from the
> Log instance to its classloader will prevent gc of
> the
> child loader on undeploy.
> 
> This chain could (potentially) be broken if
> LogFactoryImpl only held WeakReferences to its Log
> instances as follows:
> 
>   public Log getInstance(String name) throws
> LogConfigurationException {
> -
> -    Log instance = (Log) instances.get(name);
> -    if (instance == null) {
> -      instance = newInstance(name);
> -      instances.put(name, instance);
> +        
> +    Log instance = null;
> +    WeakReference ref = (WeakReference)
> instances.get(name);
> +    if (ref != null) {
> +      instance = (Log) instances.get(name);
> +      if (instance == null) {
> +        instance = newInstance(name);
> +        instances.put(name, new
> WeakReference(instance));
> +      }
>    }
>    return (instance);

Oops.  Try:

  public Log getInstance(String name) throws
LogConfigurationException {
-
-    Log instance = (Log) instances.get(name);
-    if (instance == null) {
-      instance = newInstance(name);
-      instances.put(name, instance);
+        
+    Log instance = null;
+    WeakReference ref = (WeakReference)
instances.get(name);
+    if (ref != null) {
+      instance = (Log) instances.get(name);
+    }
+    if (instance == null) {
+      instance = newInstance(name);
+      instances.put(name, new
WeakReference(instance));
+    }
   return (instance);


Brian


		
__________________________________ 
Yahoo! Messenger 
Show us what our next emoticon should look like. Join the fun. 
http://www.advision.webevents.yahoo.com/emoticontest

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org