Bug 38579 - Tomcat nulling internal state of objects referenced by static finals
Summary: Tomcat nulling internal state of objects referenced by static finals
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 5
Classification: Unclassified
Component: Catalina (show other bugs)
Version: Unknown
Hardware: All All
: P2 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-02-08 21:22 UTC by Matt Jensen
Modified: 2006-02-16 06:19 UTC (History)
0 users



Attachments
Modified WebappClassLoader.java (75.60 KB, text/plain)
2006-02-09 05:50 UTC, Matt Jensen
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Jensen 2006-02-08 21:22:29 UTC
I have run into an issue with Tomcat's handling of static fields in webapp
classes on context shutdown.  On line 1585 of WebappClassLoader.java, Tomcat
embarks upon a well-intentioned mission to clear static state out of webapp
classes as it unloads them.  This can lead to a problem in at least one case; I
have run into this problem while attempting to replace Tomcat's use of
commons-logging with a solution based upon SLF4J and Log4J.

Tomcat would appear to proceed in an innocuous way so long as the fields it
encounters are static but NOT final.  In these cases, it simply nulls the static
reference.  Completely harmless since the class that holds it is about to be
released.  However, when a field is encountered which is static AND final, it
descends into the referenced object itself and nulls out all of its fields. 
This is a huge problem when those objects are shared (implying, of course, that
they are of classes loaded via the 'shared' or 'common' classloaders.)  As it
iterates over the fields of the referenced objects, it calls 
AccessibleObject.setAccessible() on each Field before it sets the null value,
meaning that referenced objects cannot defend themselves even by marking their
internal state as final.

The reason for descending into referenced objects for static final fields would
appear to be a hedge against the fact that setting the static final reference
itself to null could fail (obviously: it's final.)  But if Tomcat can--and
obviously does--overwrite final *instance* state by calling setAccessible() on
the *instance* Field, could it not instead call setAccessible() on the *static*
Field and nullify it instead?

Tomcat's current behavior can potentially (and DOES) strip state out of living
objects.  This is definitely a bad thing.  I will try to describe the scenario.

SLF4J (specifically, its jcl104-over-slf4j.jar) implements all of the
commons-logging interface in a static way.  This avoids the various
commons-logging classloader issues while still supporting the API.  I have
deployed the SLF4J and Log4J jars into the common classloader so that both
Tomcat and my webapps can use it.  This puts SLF4J and its statics and caches
into shared space.

Some of the classes in my webapp have static final
org.apache.commons.logging.Log instances in them.  (This is unavoidable, as in
most cases they occur in external libraries, such as Spring Framework.)  SLF4J
provides implementations of the LogFactory and Log classes, named
SLF4JLogFactory and SLF4JLog, respectively.  The problem is that SLF4JLogFactory
holds a cache of all Log instances that it has created--and as I mentioned
above, this cache is held in shared memory.

Now take a look at what happens when a webapp class holding one of these static
final Log instances is unloaded and then reloaded: on first load, the Log
instance is created, SLF4J caches it, and it is also stored in the static final
field on the webapp class in question.  Now the application is unloaded.  Tomcat
comes across that static final field during its cleanup process and descends
into the Log (SF4JLog) instance and nulls out all of its fields--one of which is
a wrapped Log4J Logger instance.  Next the application is reloaded.  When
SLF4J's LogFactory.getLog() method is invoked this second time, it obtains the
existing Log instance from the cache and returns it--however, this instance has
had its internal state destroyed by Tomcat during the preceding context unload.
 In the particular case of SLF4J, this causes a NullPointerException to be
thrown the first time that Log instance is used to output a log message.

I have mentioned SLF4J several times in this message, but this is only as an
example.  This is NOT an SLF4J issue.  Tomcat should not be trashing the
internal state of live objects without their knowledge.  SLF4J just happens to
be a victim because of the way it handles its internal caching.  Tomcat is the
root of the problem.

I can think of at least two possible fixes for this.  The first would be to call
setAccessible(true) on the static final Field and then setting *it* to null
instead of descending into the object it references.  If doing so allows
instance fields to be modified, wouldn't it also work for static fields?  If so,
this problem would disappear.

A second possibility, which may or may not work for Tomcat's purposes, would be
to check the classloader for an object before trashing its internal state.  Only
do the "nullifying" if the object is of a class that was loaded by a context
classloader.  Unfortunately, this may fail to address the leaks that Tomcat is
trying to prevent in the first place.  I am not familiar with this area of Tomcat.

I would be more than happy to supply additional information on request.  I will
also do some experimentation with the WebappClassLoader source to see if I can
put together a patch.

Thanks for your time,
--Matt Jensen
Comment 1 Matt Jensen 2006-02-09 05:47:13 UTC
I did some research this evening and found out that, sure enough, Java 5 allows
instance final fields to be overwritten but does NOT allow this for static final
fields.  Therefore my suggestion of simply overwriting the static final
reference will not work.

I made some modifications to the cleanup code in WebappClassLoader.java, which
cause it to only nullify fields of descended-into objects if they refer to
objects of classes which were loaded by 'this' class loader or one of its
children (if it has any.)  I am attaching the modified source file.  Please take
a look at it and see if it still achieves the desired effect with respect to
Tomcat's cleanup process.  I am new to class loader debugging.
Comment 2 Matt Jensen 2006-02-09 05:50:47 UTC
Created attachment 17624 [details]
Modified WebappClassLoader.java

Modified method nullInstance() and added method loadedByThisOrChild().
Comment 3 Remy Maucherat 2006-02-16 15:19:11 UTC
The logging setup you are using is broken.

As for your proposed "fix", I am ok with it, as the problem I was trying to
address was not caused by classes loaded by the shared classloaders.