Re: [Springframework-developer] Cleanup of context resources on webapp reload

SourceForge Headquarters 1320 Columbia Street Suite 310 San Diego, CA 92101 +1 (858) 422-6466

I've just found out that our JPetStore's remaining resource leak is not =
caused by iBATIS or JDOM but by Struts: Struts uses Commons BeanUtils =
which in turn uses the JavaBeans Introspector without flushing... So on =
web app shutdown, the Introspector cache holds references to Struts =
Actions from the web app classloder. This prevents garbage collection of =
JDOM's singletons because the web app classloader (which holds its =
loaded classes and thus their static references) is still around. So =
it's not JDOM's fault; it's rather like with Spring's =
SQLErrorCodesFactory and co.
=20
Which gives 2 candidates that would benefit from a general =
Introspector.flushCaches call on shutdown so far: Struts and Quartz. As =
per my mail from yesterday, Spring does not need that anymore as it =
flushes the JavaBeans Introspector for each class that it caches =
introspection results for itself. If you still see Spring classes =
hanging around after web app shutdown, that's caused by leaks of *other* =
tools that prevent the class loader (which is referencing Spring =
classes) from being disposed.
=20
So the remaining question is: Should we provide a way to call =
Introspector.flushCaches on web app shutdown? Doing this in =
ContextLoaderListener will just affect apps that actually use =
ContextLoaderListener: What about DispatcherServlet-only apps? What =
about Struts apps that load a Spring context via a plugin? What about =
Servlet 2.2 apps that can't register listeners? I guess it would be =
better to provide separate IntrospectorCleanupListener and =
IntrospectorCleanupServlet classes (analogous to Log4jConfigListener and =
Log4jConfigServlet), just doing a Introspector.flushCaches call on =
shutdown.
=20
Of course, it would be ideal if the JavaBeans Introspector used a =
WeakHashMap with WeakReference values - none of this would be necessary =
then. After all, it's a pretty obvious bug in the current JDK: What's =
the point in using a WeakHashMap with values that reference the key but =
are not held in WeakReferences? If they wouldn't care about garbage =
collection in the first place, they could use a plain HashMap... =
Currently, they seem to go just half the way.
=20
With the current JDK implementation, it would also be nice if the =
servlet container did an Introspector.flushCaches call on web app =
shutdown, or preferably a flushFromCaches call for each class loader by =
the corresponding class loader. However, it's unrealistic that all =
containers will do this any time soon - and this wouldn't be necessary =
either if that d*** JavaBeans Introspector implementation properly used =
WeakReferences.
=20
So I guess I'll add IntrospectorCleanupListener and =
IntrospectorCleanupServlet classes... thoughts?
=20
Juergen
=20

________________________________

Von: spr...@li... im Auftrag =
von j=FCrgen h=F6ller [werk3AT]
Gesendet: Mo 31.05.2004 13:08
An: spr...@li...
Betreff: Re: [Springframework-developer] Cleanup of context resources on =
webapp reload

Guillaume,

As far as I see, the only benefit that we would gain with such a =
classloader check is that we avoid recaching introspection results =
during application runtime, for the case where the garbage collector has =
eagerly removed them. However, the latter can only happen when there are =
no other references to the given Class: This won't be the case with =
typical Spring usage, as both bean factories and web command controllers =
(i.e. the main usages of BeanWrapper) hold a reference to the bean class =
respectively command class.

And if there are no other references to a specific Class, I suppose the =
garbage collector is free to remove the cached introspection results for =
it; this can be considered a desirable thing. So I guess it's better to =
leave CachedIntrospectionResults as it is, as there's no noteworthy =
difference in all other respects. Do you see any negative impact on =
performance, given the scenario from above? Your earlier point comes to =
my mind here: It's also about a simple implementation that doesn't =
obscure the purpose of the code.

Juergen

________________________________

Von: spr...@li... im Auftrag =
von Guillaume Poirier
Gesendet: Mo 31.05.2004 00:26
An: spr...@li...
Betreff: Re: [Springframework-developer] Cleanup of context resources on =
webapp reload

Juergen,

I think that Introspector.flushCaches() on a ServletContextListener it =
still
the lesser of evils for that memory leak, the effect on other webapps =
would
not be that bad anyway, just slow a little bit the next few calls to
Introspector.getBeanInfo().  However, I agree with you that this should
probably be dealt with by the application rather than Spring.  But may =
be it
could be an option with Spring's ContextLoaderListener (probably off by
default)?

Concerning your solution with CachedIntrospectionResults, I would have a
sugestion for performance.  For the majority of the use cases,
CachedIntrospectionResults would be in the same ClassLoader as the =
target
class.  So I would suggest to first check the ClassLoader to know if =
it's
safe to cache the class without WeakReference.  I've attached a patch to =
the
E-Mail if you would like to include my suggestion.

Thanks,
Guillaume

----- Original Message -----
From: "j=FCrgen h=F6ller [werk3AT]" <jue...@we...>
To: <spr...@li...>
Sent: Sunday, May 30, 2004 11:09 AM
Subject: Re: [Springframework-developer] Cleanup of context resources on
webapp reload

Guillaume,

I'm using a profiler, explicitly running garbage collection after
application shutdown and then seeing what remains on the heap. It's =
actually
my first really instance profiler work for about two years; haven't had =
any
need for it in the meantime.

Anyway, I did some further tests today, changing SQLErrorCodesFactory =
and
GlobalAdvisorAdapterRegistry to classic singletons again, with an
Introspector.flushCaches call on shutdown. While this didn't have any =
effect
with Petclinic, it did result in proper cleanup with Image Database.

So the Introspector.flushCaches call *does* have some effect, contrary =
to my
assumption from yesterday, but not in all scenarios. After some further
tests, I found out that Hibernate (-> Petclinic) has its own leaks, thus
keeps the web app class loader around, thus no cleanup of Spring's
singletons in that class loader. If Hibernate isn't used, everything =
gets
cleaned up properly.

This means that you're perfectly right: Coding singletons with
WeakReferences only cleans up that particular singleton object but of =
course
doesn't do anything about the class loader itself with its remaining =
leaks.
As the code *is* uglier with WeakReferences, I've permanently changed
SQLErrorCodesFactory and GlobalAdvisorAdapterRegistry back to classic
singletons (like they were until a week ago).

I'd like to leave CachedIntrospectionResults as-is with a WeakHashMap =
and
WeakReferences, as this holds references to classes and might reside in =
a
parent classloader of the referenced classes. That's how the JDK's
Introspector class should be coded too...

The remaining issue is: Where to invoke Introspector.flushCaches? Every
ApplicationContext shutdown? How to avoid cleaning all BeanInfos *of the
entire VM*, when all we want to do is clean up all BeanInfos of the =
current
app?
Sure, a general flushCaches call cleans up all BeanInfo leaks of the app
too, be it from Spring or Quartz or whatever, but all we want to provide
out-of-the-box is proper cleanup of Spring itself.

So my solution for Spring is simple: CachedIntrospectionResults flushes =
the
Introspector cache for the given class (and its superclasses) right =
after it
fetched the BeanInfo for it (i.e. right after it's been cached). As we =
cache
the introspection results ourselves anyway, we wouldn't benefit from the
Introspector cache in the first place. That shouldn't have any negative
impact on performance.

I did quite a lot of tests with this new strategy, and everything gets
cleaned up nicely within Spring. Just if you add Hibernate or Quartz to =
the
mix, you'll get leaks from them that prevent the class loader from =
getting
garbage collected. With Quartz, a general flushCaches call helps; with
Hibernate, even that doesn't.

So in certain scenarios, it might help to add a custom
ServletContextListener that does an Introspector.flushCaches call on
shutdown - but for Spring itself, this isn't necessary, as the beans
infrastructure does not cause any leaks anymore. I'm happy with the =
current
solution: classic singletons as before, just a flushFromCaches call for =
each
introspected class right when building the CachedIntrospectionResults
object.

We should probably tell the Quartz guys to apply specific =
flushFromCaches
calls after their Introspector usage. I've also noticed that JDOM, as =
used
within iBATIS SQL Maps 1.3, has a resource leak too. I haven't =
researched
where the Hibernate leaks come from in detail, but the root of the =
problem
seems to be CGLIB there.

Many thanks for pointing this out! :-)

Juergen

________________________________

Von: spr...@li... im Auftrag =
von
Guillaume Poirier
Gesendet: So 30.05.2004 07:01
An: spr...@li...
Betreff: Re: [Springframework-developer] Cleanup of context resources on
webapp reload

Juergen, I'm curious about how exactly you assert what is leaking, what =
is
not, and exactly what is it better with WeakReference?  Are you using a
profiler that tells you the non-garbage collected objects in the JVM or =
do
you use other means?

Concerning SQLErrorCodesFactory, while the class' constructor does cause =
a
leak (indirectly by the use of XmlBeanFactory), it is not itself the =
leak,
it's the class instance of SQLErrorCodes that prevent the ClassLoader's
collection.  I did a test calling
SQLErrorCodesFactory.getInstance().getErrorCodes("DB2") in a child
ClassLoader, and it caused a leak when the ClassLoader is thrown away.  =
Then
I tried to create my own SQLErrorCodesFactory implementation, and it =
kept
leaking until I stopped using the XmlBeanFactory to create the instances =
of
SQLErrorCodes.  Then I tried to just use an Introspector directly to set =
the
properties on the SQLErrorCodes instances by reflection, and it leaked =
just
as the SQLErrorCodesFactory. You have to keep in mind that if e.g.
Introspector has an hard reference on SQLErrorCodes.class, then none of =
the
class loaded by its ClassLoader can be collected.  So it would be the =
cause
of SQLErrorCodesFactory singleton to be kept alive, not because
SQLFactoryErrorCodes has an hard reference on it, but because the
ClassLoader does, and SQLErrorCodes.class has an hard reference on the
ClassLoader, and the Introspector has an hard reference on
SQLErrorCodes.class.  So the singleton of SQLFactoryCodesFactory is =
really
kept alive because Introspector has an hard reference on the class =
instance
of SQLErrorCodes.

I might misunderstand the situation, but as I see it, you're working on =
the
symptom rather than the cause.  If you allow the singleton to be garbage
collected when the class itself is retained, then you might save some
memory, but it's just like adding memory to the JVM to solve a leak, it =
will
only delay the OutOfMemoryError, it won't prevent it.  However, if you =
allow
the ClassLoader to be collected by removing any hard reference to its
classes, that would prevent the leak to even take place at all.

That's why I'm curious as to why you say it's better with WeakReference =
on
singleton and Introspector.flushCaches() does nothing, how do you make =
that
assertion?  I realize that if there's something else than Introspector =
that
has an hard reference on a class of the ClassLoader being thown away,
flushing the Introspector's cache will have no effect on the leak, it =
would
still leak as fast.  But while the WeakReference on the singleton will =
delay
the OutOfMemoryError, does it really help that much, since anyway all =
the
Class definitions and static members cannot be collected?  You have to
consider that coding defensively on this might reduce performance =
because of
more object creation and use of synchronization, while also complicating =
the
code.  Is it really worth it, did your tests really showed a significant
effect on a typical application?

Guillaume

----- Original Message -----
From: "j=FCrgen h=F6ller [werk3AT]" <jue...@we...>
To: <spr...@li...>
Sent: Saturday, May 29, 2004 2:28 PM
Subject: Re: [Springframework-developer] Cleanup of context resources on
webapp reload

Guillaume,

I've prototypically added an Introspector.flushCaches call to context
shutdown: I don't see any difference in the profiler. That's not too
surprising, as the originally leaking classes are not managed by beans
facilities in the first place: for example, SQLErrorCodesFactory, which =
is
just used internally by SQLErrorCodeSQLExceptionTranslator.

Of course, since my changes from a week ago, those classes don't leak
anymore, as they hold their singleton instance in a WeakReference now... =
I
still don't understand why this is necessary, but I'm 100% sure that it =
does
make a difference on both Sun JDK 1.4.2 and Sun JDK 1.3.1. I've also =
tried
various GC configuration options - always the same effect.

Juergen

________________________________

Von: spr...@li... im Auftrag =
von
Guillaume Poirier
Gesendet: Sa 29.05.2004 06:12
An: spr...@li...
Betreff: Re: [Springframework-developer] Cleanup of context resources on
webapp reload

I experimented some more about this cleanup issue, and I was able to =
narrow
down the problem to the caching done by the java.beans.Introspector.  It
stores the BeanInfo instances in a WeakHashMap, but in that Map
implementation, only the keys uses WeakReference, the values are stored =
with
hard references since BeanInfo has an hard reference on the class it =
gives
info about (indirectly through BeanDescriptor and others), any time
Introspector.getBeanInfo(Class) is used, that class and it's static =
members
will not ever be able to be garbage collected.  I kind of remember =
someone
mentioning something related to this in the mailling list, but I cannot =
find
the mail.  I wonder if there's other case where the java[x] classes =
might
have an hard reference on a class or its instances.  A fix for this
particular problem is to have a ServletContextListener call
Introspector.flushCaches() when the context is destroyed.

It seems like a known issue at Sun :

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=3D4291376
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=3D4730581
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=3D4809008

So, unless I'm missing something here, that means fixes like using
synchronization and WeakReference on singleton probably won't help much =
if
at all.  The only way that I can see for a class not to be gargage =
collected
when no more active thread use it, is if another ClassLoader has an hard
reference to the class instance, or an instance of that class.  Having =
the
class itself have an hard reference on a its own singleton has no =
effect,
it's a circular reference that will not prevent the class or the =
instance to
be gargabe collected when neither is being refered to by something else.

Guillaume

-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g.
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3D3149&alloc_id=3D8166&op=3Dclick
_______________________________________________
Springframework-developer mailing list
Spr...@li...
https://lists.sourceforge.net/lists/listinfo/springframework-developer

-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g.
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id149&alloc_id=8166&op=3Dick
_______________________________________________
Springframework-developer mailing list
Spr...@li...
https://lists.sourceforge.net/lists/listinfo/springframework-developer

-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g.
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id=3D3149&alloc_id=3D8166&op=3Dclick
_______________________________________________
Springframework-developer mailing list
Spr...@li...
https://lists.sourceforge.net/lists/listinfo/springframework-developer

-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g.
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id149&alloc_id=8166&op=3Dick
_______________________________________________
Springframework-developer mailing list
Spr...@li...
https://lists.sourceforge.net/lists/listinfo/springframework-developer

-------------------------------------------------------
This SF.Net email is sponsored by: Oracle 10g
Get certified on the hottest thing ever to hit the market... Oracle 10g.
Take an Oracle 10g class now, and we'll give you the exam FREE.
http://ads.osdn.com/?ad_id149&alloc_id=8166&op=3Dick
_______________________________________________
Springframework-developer mailing list
Spr...@li...
https://lists.sourceforge.net/lists/listinfo/springframework-developer