Circular Reference on WeakHashMap

• Key: BEANUTILS-291
• Type: Bug
• Status: Closed
• Resolution: Fixed
• Priority: Major
• Assignee: Niall Pemberton
• Reporter: Niall Pemberton
• Component/s: Bean / Property Utils
• Affects Version/s: 1.8.0-BETA
• Fix Version/s: 1.8.0

Description

Clebert Suconic wrote on the dev@commons list ....
(see http://tinyurl.com/2a9gan)

I have been investigating WeakHashMaps on BeanUtils 1.8 as part of a investigation on this:

http://jira.jboss.com/jira/browse/JBAS-2299

(Which is not actually an issue with JBAS, but an issue when using BeanUtils as part of the classPath).

There is a circular reference on the WeakHashMap, The WeakHashMap will have the ClassLoader as the key, and it will have a reference back to the Key from one of the Reflection objects. This doesn't work! (Please.. no discussions about this point.. if you don't believe me, do some testing with simple stuff before discussing this and come back to me only after that)

org.jboss.web.tomcat.service.WebAppClassLoader@16334564
!--- sun.reflect.DelegatingClassLoader@27651708
!--- !--- class sun.reflect.GeneratedConstructorAccessor38
!--- !--- !--- [Ljava.lang.Object;@10800875
!--- !--- !--- !--- java.util.Vector@838806
!--- !--- !--- !--- !--- sun.reflect.DelegatingClassLoader@27651708
!--- !--- !--- !--- !--- !--- class sun.reflect.GeneratedConstructorAccessor38
!--- !--- !--- !--- !--- !--- !--- class java.lang.Class
!--- !--- !--- !--- !--- !--- !--- !--- org.apache.commons.beanutils.converters.ClassConverter@22616909
!--- !--- !--- !--- !--- !--- !--- !--- !--- org.apache.commons.beanutils.converters.ArrayConverter@18888821
!--- !--- !--- !--- !--- !--- !--- !--- !--- !--- org.apache.commons.beanutils.converters.ConverterFacade@13619754
!--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- java.util.HashMap$Entry@32434103
!--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- [Ljava.util.HashMap$Entry;@28236766
!--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- java.util.HashMap@14997495
!--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- org.apache.commons.beanutils.ConvertUtilsBean@2016953
!--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- !--- org.apache.commons.beanutils.BeanUtilsBean@30487951
Unable to render embedded object: File (--- !--- !--- !--- !--- !--- !--- !--- !-- -) not found.--- !--- !--- !--- !--- !--- !---FieldReference private java.lang.Object java.util.WeakHashMap$Entry.value=java.util.WeakHashMap$Entry@23775534 Detail

I don't know if I'm preaching to the choir, but just in case this is new information to someone... you should aways keep Reflection referenced as SoftReferences (if you really have to). Reflection is aways a new object so a WeakReference is too weak.

On JBossSerialization I have solved this using an interesting way. I called it PersistentReference. I'm using SoftReferences, and keeping the information to recreate it case the SoftReference is cleared:

http://fisheye.jboss.org/browse/JBoss/jboss-serialization/src/org/jboss/serial/references/PersistentReference.java?r=1.3

And also... you guys should write a testcase to validate if the Caching is being cleared. (I don't know if you have one).

http://anonsvn.jboss.org/repos/jbossserialization/trunk/tests/org/jboss/serial/memory/MemoryLeakTestCase.java

You don't need to use the jboss-profiler API for this.. just create a WeakReference to a new ClassLoader, and validate if it was released at the end after some exercizing some code on this caching. You will
probably need to fill your memory almost to 100% on the test as SoftReference are only gone when the memory is low.

Comments

[Clebert Rezende Suconic]

Testcase for this leak is attached!

[Clebert Rezende Suconic]

Proposed fix!

[Niall Pemberton]

Thanks for the patch Clebert - I'm off on holiday for 3 weeks - but I plan to pick this up when I get back.

[Gerald Holl]

Niall, when will this issue be fixed?

[Niall Pemberton]

Clebert,

My apologies that its taken so long to get round to looking at your patches. The memory leak test case, links to your wiki on Weak/SoftReferences and JBoss Profiler were all very useful - thanks.

I have just commited a MemoryLeakTestCase - based on the one you supplied:

http://svn.apache.org/viewvc?view=rev&revision=636989

...but with additional tests because, unfortunately, there are more scenarios within BeanUtils for memory leaks. I believe the following is the complete list (including those you're know):

• PropertyUtilsBean, descriptorsCache, FastHashMap<Class, PropertyDescriptor[]>
• PropertyUtilsBean, mappedDescriptorsCache, FastHashMap<Class, FastHashMap<String, MappedPropertyDescriptor>>
• MethodUtils, cache, WeakHashMap<MethodDescriptor, Method>
• WrapDynaClass, dynaClasses, Map<Class, WrapDynaClass>
• ConvertUtilsBean, converters, HashMap<Class, Converter>
• LocaleConvertUtilsBean, mapConverters, FastHashMap<Locale, FastHashMap<Class, LocaleConverter>>

1) PropertyUtilsBean
--------------------

• descriptorsCache: FastHashMap<Class, PropertyDescriptor[]>
• mappedDescriptorsCache: FastHashMap<Class, FastHashMap<String, MappedPropertyDescriptor>>

I looked at the changes you propose for PropertyUtilsBean and I don't believe its necessary in this case to use your ProxyHashMap - which is effectively a "cache of caches" keyed by ClassLoader. We already have the equivalent of this called ContextClassLoaderLocal[1] which stores an instance of BeanUtilsBean for each ClassLoader - and each BeanUtilsBean has a PropertyUtilsBean instance.

So looks to me like we simply need to change the descriptorsCache and mappedDescriptorsCache to be WeakHashMap instead of FastHashMap. I'm not even sure why those are FastHashMap because they have "fast" set to true and in "fast" mode FastHashMap operates as a HashMap, with no synchronization.

I tried this out using your test case - it seems to resolve the memory leak for descriptorsCache, but not mappedDescriptorsCache. From what I can tell there are two additional problems with mappedDescriptorsCache:

• It uses MethodUtils which also has a cache that can leak - ignore that for now I'll come to that below.
• MappedPropertyDescriptor(MPD) is a custom BeanUtils implementation of PropertyDescriptor that has one Class and two Method instance variables. Changing these to SoftReference seems to resolve the issue.

[1] http://svn.apache.org/repos/asf/commons/proper/beanutils/trunk/src/java/org/apache/commons/beanutils/ContextClassLoaderLocal.java

2) MethodUtils
--------------

cache: WeakHashMap<MethodDescriptor, Method>

The cache in MethodUtils is a static WeakHashMap instance and the Method value stores seems to prevent gc. Putting the Method in a WeakReference seems to resolve this. Perhaps it should be a SoftReference, but the only danger seems to be that it will get re-created more often.

3) WrapDynaClass
----------------

dynaClasses: Map<Class, WrapDynaClass>

I had already done some work to move the static dynaClasses cache into a "by ClassLoader" cache (using ContextClassLoaderLocal). Unfortunately its a hack since it was "protected" visibility and I'm avoiding breaking binary compatibility with the previous release. Anyway, your test showed that the problem still wasn't resolved and the issue seems to be the Class instance variable WrapDynaClass has. Moving this into a SoftReference seems to resolve it.

4) ConvertUtilsBean
-------------------

converters: HashMap<Class, Converter>

There are two issues here - the cache and the fact that AbstractConverter (which many of the Converters stored in the cache derive from) has a reference to Class. Changing the HashMap to a WeakHashMap and putting the AbstractConverter's Class reference into a SoftReference seems to resolve this. However I think it would be best to remove the Class reference from AbstractConverter all together, since if that reference is garbage collected then the Converters will fail, unless some magic is done to re-create it.

5) LocaleConvertUtilsBean
-------------------------

mapConverters: FastHashMap<Locale, FastHashMap<Class, LocaleConverter>>

I believe this should have been straight forward to fix, simply replacing FastHashMap<Class, LocaleConverter> with WeakHashMap<Class, LocaleConverter>. However the FastHashMap is exposed in the API so changing to WeakHashMap breaks compatibility. So I've tried creating a WeakFastHashMap, which is a nasty hack but seems to work.

I'm attaching a patch to this ticket with some proposed changes - although it uses SoftReference in AbstractConverter which I probably won't do as I said above. These changes all seem to make the memory leak tests pass using JDK 1.5.0_07. However I tried running the tests using JDK 1.4.2_12 and JDK 1.3.1_18 and three out of the six memory leak tests fail (both PropertyUtilsBean tests and the WrapDynaClass test) - so not sure why that is.

I'm hoping that you still have time and interest in this, despite how long its taken for me to get round to doing anything. Any feedback on my modified version of your tests and proposed changes would be very welcome.

Niall

[Niall Pemberton]

I have resolved one of the memory leak issues - Converters holding a reference to Class.
http://svn.apache.org/viewvc?view=rev&revision=640131

This is an incompatible change to BeanUtils 1.8.0-BETA (but not to the previous 1.7.0 release). The incompatible changes are:

• AbstractConverter - constructor signatures are different and getDefaultType() method is now abstract
• DateTimeConverter - now an abstract class and constructor signatures are different
• NumberConverter - now an abstract class and constructor signatures are different

Attaching a revised patch for the other proposed changes

[Clebert Rezende Suconic]

I see one problem on MappedPropertyDescriptor.java

There is nothing testing if mappedReadMethodRef.get()==null, then recreate it.

Reflection is an unique object on the JVM. Every time you do class.getmethod (or getField.. whatever), you get a new object.

Since this is using SoftReference, when the memory goes low, you will certainly loose the reference to ReadMethod, but the class will still be loaded (and valid). So, on that case you need to do a:

if (mappedReadMethodRef.get() == null)
{
mappedReadMethod = createReferenceAgain();
}

// this is a fictitious name... just to illustrate the idea:
public synchronized createReferenceAgain()
{
...
}

To correctly testcase this scenario on SoftReferences, you should get a BeanDescriptor (forcing the creation of MappedPropertyDescriptor), force a OutOfmemoryError, and then make an operation again. I didn't do any tests yet but I believe you would have a NPE on that scenario.

[Clebert Rezende Suconic]

This issue on reflection is "a little" bit more complicated than what appears.

You need to save the method or field information for an occasional recreation if necessary.

If you want to, I used this solution on JBossSerialization:

http://anonsvn.jboss.org/repos/jbossserialization/trunk/src/org/jboss/serial/references/

I called it PersistentReference. When you get a Reflection reference, I keep the names, the Class (on a WeakRefeference of course) and argument types in a way you recreate the reference when necessary.

I used this just for simple Arguments, so maybe we would need to expand ArgumentReference to also save the name types for the arguments.

[Niall Pemberton]

Clebrt, thanks for the feedback. It may take me a week or so to find time to look at this, but thanks for pointing out the issue. I'll look at caching the method info in (weak) references so that it can be re-created if required.

[Niall Pemberton]

Clebert, I have committed the changes to fix the memory issues, including recreating the method reference in MappedPropertyDescriptor.

http://svn.apache.org/viewvc?view=rev&revision=650215

If this all looks OK, then I think we can go ahead with a BeanUtils release

[Niall Pemberton]

I created a 1.8.0 RC1 for review here:
http://people.apache.org/~niallp/beanutils-1.8.0-rc1/

RC1 thread:
http://commons.markmail.org/message/lvy36frxdi2t2rxz

[Clebert Rezende Suconic]

I believe there is a thread isolation problem on PropertyUtilsBean. I was actually having few test hickups.. maybe caused by that.

I have changed FastHashMap a little bit to allow creations and constructions on a sub method, and I created a WeakHashMap that will use those methods.

I also changed PropertyUtilsBean to use the "new" WeakHashMap.

I have deleted that WeakHashMap inner class and used the new one created.

And I have also changed the testcase a little bit. It is now checking for releases on SoftReferences, as they are not aways released.

For the MemoryLeakTestCase, it would be good if you could change mvn or whatever starts the test, to run it with less memory (-Xmx25M for instance). This way this test would run in less than 10 seconds.

With those fixes I'm attaching now I got a 100% testsuite pass.

(Please review the patch before applying it)

[Clebert Rezende Suconic]

(Same as previous comment.. just simplified one method on the testcase)

[Niall Pemberton]

Thanks for the patch - I have applied the changes to MemoryLeakTestCase and configured maven to limit the memory as you suggest:
http://svn.apache.org/viewvc?view=rev&revision=651114

For the other changes (i.e. to PropertyUtilsBean, LocaleConvertUtilsBean, FastHashMap and the new WeakFastHashMap) I need to think about them. Your patch has highligted that I screwed up in removing the use of FastHashMap in PropertyUtilsBean - I had a false memory that in "fast" mode it was operating like a normal Map (rather than syncronizing/cloning). However I don't want to change the FastHashMap implementation since this is a copy from Commons Collections and if Collections and BeanUtils are both on the classpath, its arbitary which version will be loaded.

[Clebert Rezende Suconic]

What about creating a new WeahHashMap without extending FastHashMap, and propose this fix to Common Collections for a future release?

[Niall Pemberton]

I implemented your patch with a slight variation. I've copied/created a new package scope WeakFastHashMap implementation into org.apache.beanutils instead. Really we want to dump the org.apache.collections classes from BeanUtils - the only reason I haven't is that FastHashMap is unfortunately exposed in the BeanUtils API and doing so would break compatibility.

http://svn.apache.org/viewvc?view=rev&revision=687089

[Niall Pemberton]

Jörg Schaible found a problem with the JRockit JDK and the MappedPropertyDescriptor:

http://markmail.org/message/4mfxqesmxcvleqll

It appears that with the JRockit JDK the (weak) reference to the class is being lost, even though in that test there is still a strong reference to the class (and class loader). I couldn't find a valid link to download the JRockit JDK, but I can re-create the same exception by adding a test that removes all strong references to the class. I didn't really expect this exception to ever get thrown, but since this has occured I've added code to try and re-create the class reference (and a test for it):

http://svn.apache.org/viewvc?view=rev&revision=689831

[Clebert Rezende Suconic]

> even though in that test there is still a strong reference to the class

It would be nice someone from JRockit commenting on this. It looks like a bug, right?