Re: memory leak

[James M Snell <ja...@gmail.com>]

Hey Bryon,

Thanks for the detailed note.  This is something that I've been
suspecting for a while now but just never had the time to explore in detail.

The main issue is static extensions.  Every time I work with a static
extension, I'm working with a wrapper.  We have no way of knowing how
expensive it would be or what side effects there could be to invoking an
ExtensionFactory numerous times for a single extension element.

That said, given the fact that ElementWrapper calls the internal equals
and hashcode methods, eliminating the cache would likely make the most
sense.  That will just leave it up to ExtensionFactory implementors to
make sure they're not doing anything silly in either the equals,
hashcode, or getelementwrapper methods.

I just made the changes locally to remove the cache and tested things
out, things still operate well without the cache and all tests are
passing.  I say we go ahead and make this change.

- James

Bryon Jacob wrote:

> [snip]
> 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)
> 
>