Mailing-List: contact abdera-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: abdera-dev@incubator.apache.org
Received-SPF: pass (athena.apache.org: domain of chriswberry@gmail.com
 designates 64.233.166.178 as permitted sender)
DomainKey-Signature: a=rsa-sha1; c=nofws;
        d=gmail.com; s=gamma;
        h=message-id:from:to:in-reply-to:content-type:content-transfer-encoding:mime-version:subject:date:references:x-mailer;
        b=efAlG9pCNiB+nby22iAAkX/KRdL7GQrYinoRMCzWl/paFvR+RY4eHeF/S67zaAsOCOoRzegchAG3GrjfryTx0gfdRjlgjz0ldsEuwmcmoJ3F7io6OuPCqUORA0zCiKdJV87WQ6NK/K4Zu7/J3gR7++zuBiwCjKiTFfJx6dGEllw=
Message-Id: <19B3AD44-16A2-4B1A-93D0-74BDD07C163C@gmail.com>
From: Chris Berry <chriswberry@gmail.com>
To: abdera-dev@incubator.apache.org
In-Reply-To: <48176AD3.2060506@mulesource.com>
Content-Type: text/plain; charset=US-ASCII; format=flowed; delsp=yes
Content-Transfer-Encoding: 7bit
Mime-Version: 1.0 (Apple Message framework v919.2)
Subject: Re: patch to 0.3.0 ExtensionFactoryMap for serious memory leak
Date: Tue, 29 Apr 2008 13:47:10 -0500
References: <B660B542-CDDA-46EE-AFAF-1F194A7EA27C@gmail.com>
 <48176AD3.2060506@mulesource.com>

Thanks for responding Dan.
I may be wrong, but (IIRC) I think 0.3.0 was patched a few times in  
place (not that its a best practice ;-)
Would 0.3.1 have to go thru the same process as a minor patch release??
If so,  it probably is too much work.
Thanks,
-- Chris 

On Apr 29, 2008, at 1:37 PM, Dan Diephouse wrote:

> Hi Chris, due to the high amount of work it takes to get a release  
> out of the incubator, I really don't see this happening. I don't  
> know what you mean by applying this patch to 0.3.0, but we can't  
> really go back and change the release. It all has to go through the  
> incubator process. And I'm going to guess that if we're going  
> through that process we're going to focus on either 1.0-RC or 0.4.1.
>
> Dan
>
> Chris Berry wrote:
>> Greetings,
>> Some time ago we reported a very serious memory leak in Abdera 0.3.0.
>> It was reported to the abdera users list on Nov 7, 2007 as "memory  
>> leak".
>> That message is replayed below...
>> At the time we could workaround the bug in our client, but over  
>> time this became impossible, and we had to hack ExtensionFactoryMap.
>>
>> Attached is a patch to org.apache.abdera.factory.ExtensionFactoryMap
>> This patch simply removes the problem -- the WeakHashMap.
>> This change has a negligible effect on performance (as measured in  
>> load testing) But without it, abdera 0.3.0 will get  
>> OutOfMemoryExceptions quite quickly under load, rendering it  
>> unusable.
>> BTW: this code has been in Production for many weeks...
>>
>> We were hoping that you could apply this patch to 0.3.0??
>> Or better;  produce 0.3.1
>>
>> We are not able to upgrade to 0.4.0 yet, and would rather not have  
>> a hacked copy of 0.3.0.
>> Also, others not able to upgrade yet (since the upgrade is not  
>> entirely transparent) may benefit from the patch
>>
>> I will also open a JIRA on this.
>>
>> Thanks,
>> -- Chris 
>> ------------------------------------------------------------------------
>>
>>
>> *From: * bryon@jacob.net <mailto:bryon@jacob.net>
>>
>> *Subject: * *memory leak*
>>
>> *Date: * November 5, 2007 3:49:24 PM CST
>>
>> *To: * abdera-user@incubator.apache.org <mailto:abdera-user@incubator.apache.org 
>> >
>> *Reply-To: * abdera-user@incubator.apache.org <mailto:abdera-user@incubator.apache.org 
>> >
>>
>>
>>                    Hey All -
>>
>> I'm working on a data service based on Abdera (working with Chris  
>> Berry, who's a regular on these lists...)  When we were running our  
>> first battery of serious load testing on our system, we encountered  
>> memory-leaky behavior, and a profiler showed us that we were indeed  
>> leaking hundreds of megabytes a minute, all traceable back to the  
>> wrappers field on org.apache.abdera.factory.ExtensionFactoryMap.   
>> This field is a map from elements to their wrappers, if any.  At  
>> first, I was puzzled by the memory leak, as the field is  
>> initialized thusly:
>>
>> this.wrappers = Collections.synchronizedMap( new  
>> WeakHashMap<Element,Element>());
>>
>> clearly, the implementor took care to make sure that this cache  
>> would not leak by making it a WeakHashMap, which generally  
>> guarantees that the map itself will not keep a key and its  
>> corresponding entry from being garbage collected.  I dug throughout  
>> our application code to find if we were actually holding other  
>> references to these objects, and I googled for anyone having  
>> problems with esoteric interactions between  
>> Collections.synchronizedMap and WeakHashMaps - found nothing  
>> there.  Then I went back to square one and re-read the WeakHashMap  
>> javadoc very carefully.  Here's the relevant section:
>>
>> Implementation note: The value objects in a WeakHashMap are held by  
>> ordinary strong references. Thus care should be taken to ensure  
>> that value objects do not strongly refer to their own keys, either  
>> directly or indirectly, since that will prevent the keys from being  
>> discarded. Note that a value object may refer indirectly to its key  
>> via the WeakHashMap itself; that is, a value object may strongly  
>> refer to some other key object whose associated value object, in  
>> turn, strongly refers to the key of the first value object. One way  
>> to deal with this is to wrap values themselves within  
>> WeakReferences before inserting, as in: m.put(key, new  
>> WeakReference(value)), and then unwrapping upon each get.
>>
>> This is why there is a memory leak - the map is a mapping from  
>> elements to their wrappers - by the very nature of the object being  
>> a wrapper of the element, it will usually have a strong reference  
>> to the element itself, which is the key! You can verify that Abdera  
>> wrappers, in general, will do this by looking at  
>> org.apache.abdera.model.ElementWrapper, which takes the element  
>> being wrapped as its constructor argument, and holds a strong  
>> reference to it as an instance variable.
>>
>> This map is an optimization to memoize the calls to  
>> getElementWrapper() and not reconstruct them more than is necessary  
>> - it is not needed for abdera to function properly, so we have  
>> temporarily worked around the problem in our own application like  
>> so - we used to acquire our FOMFactory by calling  
>> abdera.getFactory() on our org.apache.abdera.Abdera instance, and  
>> re-using that singleton throughout our application.  Now we  
>> construct a new FOMFactory with new FOMFactory(abdera) once per  
>> request to the server, and since the only appreciable state on the  
>> factory is this map itself, this is a valid work-around.
>>
>> I'd initially planned to really fix this issue and submit a patch  
>> along with this message, but digging a little deeper, I'm not sure  
>> that the correct fix is crystal clear...  We could do as the  
>> javadoc above suggests, and wrap the values with WeakReferences to  
>> plug the leak, or we could use a LinkedHashMap configured as an LRU  
>> cache to just bound the cache, so it can't grow out of control -  
>> but right now, I don't think that either of those solutions would  
>> be correct, because it seems to me that none of the objects in the  
>> hierarchy rooted at FOMElement define equals() and/or hashCode()  
>> methods, so all of the objects are cached based on their actual  
>> object identity.  It seems that in the all likely use cases,  
>> instances of FOMElement and its descendants are re-parsed on every  
>> request to a server running abdera, and so what we will see is  
>> cache misses virtually 100% of the time, so even though we'd have  
>> plugged the leak, strictly speaking, we would have ignored the  
>> underlying issue that we're caching data on every request that will  
>> be fundamentally unable to be retrieved on subsequent requests.   
>> This is based only on my reading over the code for a few hours, so  
>> I could be missing something, and I also might be forgetting about  
>> a use case that demands and makes proper use of this memoization,  
>> but as it stands right now, my recommended fix would probably be to  
>> just cut out the cache altogether, and allow for wrappers to get  
>> constructed fresh every time they are requested.  One more  
>> possibility is that the cache is actually a useful optimization,  
>> but only during the scope of one request - in which case the "work- 
>> around" we are using now is actually the best practice, and the fix  
>> would be to remove the factory instance on the Abdera class...
>>
>> I'd like to hear from the Abdera developers what their thoughts are  
>> on this issue, and what the best resolution is likely to be.  This  
>> is no longer a pressing issue for our team, but it is potentially a  
>> time bomb waiting to blow up for any project dependent on Abdera.
>>
>> thanks!  (and thanks for Abdera, generally - we're easily a year  
>> ahead of where we'd be on this project without it!)
>>
>> -Bryon (long-time listener, first-time caller)
>
>
> -- 
> Dan Diephouse
> MuleSource
> http://mulesource.com | http://netzooid.com