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

[Colin S. <col...@ex...>]

The IntrospectorCleanupListener and IntrospectorCleanupServlet sound=20
reasonable. At the end of the day, as has been discussed, this is=20
probably going to be of most use to people in development; I'm not sure=20
how many people trust hot-deploy in production. Certainly I would never=20
do it with something like JBoss. The only option I am pretty strongly=20
opposed to is to have the default behaviour to be to call flushCaches on=20
shutdown, since it can affect so many other apps in the container; I=20
realize you don't think this is a good default either .

I should mention Juergen that your changes appear to have had an=20
interesting effect on hot-redeploy of my main app in JBoss. As of about=20
maybe 1.5 months ago, hot redeploy in JBoss stopped working for me, with=20
JBoss complaining on reload about ClassCastExceptions on a number of=20
classes. As of about a week ago, it's working again. I never really had=20
time to track down why the hot-redeploy stopped working (the JBoss=20
unified classloader is a fairly random, unpredictable beast, especially=20
with EJBs involved), but the timing with it working again now makes me=20
think that the recent changes are responsible for it working again...

Regards,
Colin

j=FCrgen h=F6ller [werk3AT] wrote:

>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 whi=
ch in turn uses the JavaBeans Introspector without flushing... So on web =
app shutdown, the Introspector cache holds references to Struts Actions f=
rom the web app classloder. This prevents garbage collection of JDOM's si=
ngletons because the web app classloader (which holds its loaded classes =
and thus their static references) is still around. So it's not JDOM's fau=
lt; 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 fr=
om yesterday, Spring does not need that anymore as it flushes the JavaBea=
ns Introspector for each class that it caches introspection results for i=
tself. If you still see Spring classes hanging around after web app shutd=
own, that's caused by leaks of *other* tools that prevent the class loade=
r (which is referencing Spring classes) from being disposed.
>=20
>So the remaining question is: Should we provide a way to call Introspect=
or.flushCaches on web app shutdown? Doing this in ContextLoaderListener w=
ill just affect apps that actually use ContextLoaderListener: What about =
DispatcherServlet-only apps? What about Struts apps that load a Spring co=
ntext via a plugin? What about Servlet 2.2 apps that can't register liste=
ners? I guess it would be better to provide separate IntrospectorCleanupL=
istener and IntrospectorCleanupServlet classes (analogous to Log4jConfigL=
istener and Log4jConfigServlet), just doing a Introspector.flushCaches ca=
ll on shutdown.
>=20
>Of course, it would be ideal if the JavaBeans Introspector used a WeakHa=
shMap with WeakReference values - none of this would be necessary then. A=
fter 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 hel=
d in WeakReferences? If they wouldn't care about garbage collection in th=
e 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 servle=
t container did an Introspector.flushCaches call on web app shutdown, or =
preferably a flushFromCaches call for each class loader by the correspond=
ing class loader. However, it's unrealistic that all containers will do t=
his any time soon - and this wouldn't be necessary either if that d*** Ja=
vaBeans Introspector implementation properly used WeakReferences.
>=20
>So I guess I'll add IntrospectorCleanupListener and IntrospectorCleanupS=
ervlet classes... thoughts?
>=20
>Juergen
>=20
>
>________________________________
>
>Von: spr...@li... im Auftrag vo=
n 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 classlo=
ader check is that we avoid recaching introspection results during applic=
ation runtime, for the case where the garbage collector has eagerly remov=
ed them. However, the latter can only happen when there are no other refe=
rences to the given Class: This won't be the case with typical Spring usa=
ge, as both bean factories and web command controllers (i.e. the main usa=
ges of BeanWrapper) hold a reference to the bean class respectively comma=
nd 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 l=
eave CachedIntrospectionResults as it is, as there's no noteworthy differ=
ence 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 purpos=
e of the code.
>
>Juergen
>
>
>________________________________
>
>Von: spr...@li... im Auftrag vo=
n 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 s=
till
>the lesser of evils for that memory leak, the effect on other webapps wo=
uld
>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 b=
e 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 targe=
t
>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 actu=
ally
>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 an=
d
>GlobalAdvisorAdapterRegistry to classic singletons again, with an
>Introspector.flushCaches call on shutdown. While this didn't have any ef=
fect
>with Petclinic, it did result in proper cleanup with Image Database.
>
>So the Introspector.flushCaches call *does* have some effect, contrary t=
o 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 get=
s
>cleaned up properly.
>
>This means that you're perfectly right: Coding singletons with
>WeakReferences only cleans up that particular singleton object but of co=
urse
>doesn't do anything about the class loader itself with its remaining lea=
ks.
>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 an=
d
>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 curr=
ent
>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 afte=
r it
>fetched the BeanInfo for it (i.e. right after it's been cached). As we c=
ache
>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 getti=
ng
>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 curr=
ent
>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 flushFromCache=
s
>calls after their Introspector usage. I've also noticed that JDOM, as us=
ed
>within iBATIS SQL Maps 1.3, has a resource leak too. I haven't researche=
d
>where the Hibernate leaks come from in detail, but the root of the probl=
em
>seems to be CGLIB there.
>
>Many thanks for pointing this out! :-)
>
>Juergen
>
>
>________________________________
>
>Von: spr...@li... im Auftrag vo=
n
>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 lea=
k,
>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 kep=
t
>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 j=
ust
>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 ca=
use
>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 real=
ly
>kept alive because Introspector has an hard reference on the class insta=
nce
>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 a=
llow
>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 t=
hat
>assertion?  I realize that if there's something else than Introspector t=
hat
>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 wo=
uld
>still leak as fast.  But while the WeakReference on the singleton will d=
elay
>the OutOfMemoryError, does it really help that much, since anyway all th=
e
>Class definitions and static members cannot be collected?  You have to
>consider that coding defensively on this might reduce performance becaus=
e 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 tri=
ed
>various GC configuration options - always the same effect.
>
>Juergen
>
>
>________________________________
>
>Von: spr...@li... im Auftrag vo=
n
>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 nar=
row
>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 giv=
es
>info about (indirectly through BeanDescriptor and others), any time
>Introspector.getBeanInfo(Class) is used, that class and it's static memb=
ers
>will not ever be able to be garbage collected.  I kind of remember someo=
ne
>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 migh=
t
>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 colle=
cted
>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 t=
he
>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 instanc=
e to
>be gargabe collected when neither is being refered to by something else.
>
>Guillaume
> =20
>