<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Feb 14, 2013 at 4:43 PM, Galder Zamarreño <span dir="ltr"><<a href="mailto:galder@redhat.com" target="_blank">galder@redhat.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5"><br>
<br>
On Feb 8, 2013, at 3:35 PM, Dan Berindei <<a href="mailto:dan.berindei@gmail.com">dan.berindei@gmail.com</a>> wrote:<br>
<br>
><br>
><br>
><br>
> On Fri, Feb 8, 2013 at 3:41 PM, Galder Zamarreño <<a href="mailto:galder@redhat.com">galder@redhat.com</a>> wrote:<br>
> Hi all,<br>
><br>
> We've got a small class loading puzzle to solve in our JSR-107 implementation.<br>
><br>
> JSR-107 has a class called Caching which keeps a singleton enum reference (AFAIK, has same semantics as static) to the systemt's CacheManagerFactory, which in our case it would be InfinispanCacheManagerFactory:<br>
> <a href="https://github.com/jsr107/jsr107spec/blob/master/src/main/java/javax/cache/Caching.java" target="_blank">https://github.com/jsr107/jsr107spec/blob/master/src/main/java/javax/cache/Caching.java</a><br>
><br>
> A naive user of JSR-107 could decide to use this Caching class in an app server environment and get a reference to the CMF through it, which could cause major classloading issues if we don't protect ourselves.<br>
><br>
> Within out CMF implementation, we need to keep some kind of mapping which given a name *and* a classloader, which can find the CacheManager instance associated to it.<br>
><br>
> This poses a potential risk of a static strong reference being held indirectly on the classloader associated with the Infinispan Cache Manager (amongst other sensible components...).<br>
><br>
> One way to break this strong reference is for CMF implementation to hold a weak reference on the CM as done here:<br>
> <a href="https://github.com/galderz/infinispan/blob/t_2639/jsr107/src/main/java/org/infinispan/jsr107/cache/InfinispanCacheManagerFactory.java#L56" target="_blank">https://github.com/galderz/infinispan/blob/t_2639/jsr107/src/main/java/org/infinispan/jsr107/cache/InfinispanCacheManagerFactory.java#L56</a><br>
><br>
> This poses a problem though in that the Infinispan Cache Manager can be evicted from memory without it's stop/shutdown method being called, leading to resources being left open (i.e. jgroups, jmx…etc).<br>
><br>
> The only safe way to deal with this that I've thought so far is to have a finalyze() method in InfinispanCacheManager (JSR-107 impl of CacheManager) that makes sure this cache manager is shut down. I'm fully aware this is an expensive operation, but so far is the only way I can see in which we can avoid leaking stuff, while not affecting the actual Infinispan core module.<br>
><br>
> I've found a good example of this in <a href="https://github.com/jbossas/jboss-as/blob/master/controller-client/src/main/java/org/jboss/as/controller/client/impl/RemotingModelControllerClient.java" target="_blank">https://github.com/jbossas/jboss-as/blob/master/controller-client/src/main/java/org/jboss/as/controller/client/impl/RemotingModelControllerClient.java</a> - It even tracks creation time so that if all references to InfinispanCacheManager are lost but the ICM instance is not closed, it will print a warm message.<br>
><br>
> If anyone has any other thoughts, it'd be interesting to hear about them.<br>
><br>
><br>
><br>
> The Caching javadoc seems to prohibit stopping the CacheManagers without user intervention (<a href="https://github.com/jsr107/jsr107spec/blob/master/src/main/java/javax/cache/Caching.java#L35" target="_blank">https://github.com/jsr107/jsr107spec/blob/master/src/main/java/javax/cache/Caching.java#L35</a>):<br>
><br>
> * Also keeps track of all CacheManagers created by the factory. Subsequent calls<br>
> * to {@link #getCacheManager()} return the same CacheManager.<br>
><br>
><br>
><br>
><br>
> And in the javadoc of Caching.close() (<a href="https://github.com/jsr107/jsr107spec/blob/master/src/main/java/javax/cache/Caching.java#L153" target="_blank">https://github.com/jsr107/jsr107spec/blob/master/src/main/java/javax/cache/Caching.java#L153</a>):<br>
> * All cache managers obtained from the factory are shutdown.<br>
> * <p/><br>
> * Subsequent requests from this factory will return different cache managers than would have been obtained before<br>
><br>
><br>
><br>
> * shutdown. So for example<br>
> * <pre><br>
> * CacheManager cacheManager = CacheFactory.getCacheManager();<br>
><br>
><br>
><br>
> * assertSame(cacheManager, CacheFactory.getCacheManager());<br>
> * CacheFactory.close();<br>
><br>
><br>
><br>
> * assertNotSame(cacheManager, CacheFactory.getCacheManager());<br>
> * </pre><br>
><br>
> We can't guarantee that getCacheManager() will return the same instance unless we keep a hard reference to it in our CacheManagerFactory. So I think the only option is to add a finalize() method to CacheManagerFactory that will stop all the CacheManagers if the user didn't explicitly call Caching.close().<br>
<br>
</div></div>A finalize() in CacheManagerFactory does not solve the problem since there's still a hard reference to the CacheManagerFactory impl from Caching, and as long as that's not cleared, finalize() won't be executed, so you're still exposed to a potential leak.<br>
<br></blockquote><div><br></div><div>Yeah, that's true.<br><br></div><div>But note that the opposite is also possible: the user can call Caching.close() from one web app and it will close all the cache managers opened from any other web app. I doubt we can protect ourselves against that...<br>
<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
My initial solution in [1] didn't pass the TCK because the tests don't keep any hard reference to the cache manager, so they could literally dissapear from the collection cos there was no other hard cache manager reference.<br>
<br>
An alternative way to solve this is to have a weak hash map with classloader as weak key:<br>
<br>
private final Map<ClassLoader, Map<String, InfinispanCacheManager>> cacheManagers =<br>
new WeakHashMap<ClassLoader, Map<String, InfinispanCacheManager>>();<br>
<br>
However, for this to work, all other references that we keep within Infinispan of cache managers have to also be weak. I've made this change and it all works fine now because the TCK does have a reference to the classloader.<br>
<br></blockquote><div> </div><div>I'm glad to hear that it works!<br><br>I didn't really consider this option, because I thought it would be too hard to change all the classloader references. Particularly the contextClassLoader references from all the threads we create...<br>
<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
With this, the user can forget to the close the cache manager, and as long as no other strong references to the classloader are kept, the Infinispan Cache Manager can be garbage collected, and it's finalize() method called resulting in proper shutdown.<br>
<br>
[1] <a href="https://github.com/galderz/infinispan/blob/t_2639/jsr107/src/main/java/org/infinispan/jsr107/cache/InfinispanCacheManagerFactory.java#L56" target="_blank">https://github.com/galderz/infinispan/blob/t_2639/jsr107/src/main/java/org/infinispan/jsr107/cache/InfinispanCacheManagerFactory.java#L56</a><br>
<div class=""><div class="h5"><br></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class=""><div class="h5">
><br>
> Cheers<br>
> Dan<br>
><br>
><br>
> Cheers,<br>
> --<br>
> Galder Zamarreño<br>
> <a href="mailto:galder@redhat.com">galder@redhat.com</a><br>
> <a href="http://twitter.com/galderz" target="_blank">twitter.com/galderz</a><br>
><br>
> Project Lead, Escalante<br>
> <a href="http://escalante.io" target="_blank">http://escalante.io</a><br>
><br>
> Engineer, Infinispan<br>
> <a href="http://infinispan.org" target="_blank">http://infinispan.org</a><br>
><br>
><br>
> _______________________________________________<br>
> infinispan-dev mailing list<br>
> <a href="mailto:infinispan-dev@lists.jboss.org">infinispan-dev@lists.jboss.org</a><br>
> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
><br>
> _______________________________________________<br>
> infinispan-dev mailing list<br>
> <a href="mailto:infinispan-dev@lists.jboss.org">infinispan-dev@lists.jboss.org</a><br>
> <a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
<br>
<br>
--<br>
Galder Zamarreño<br>
<a href="mailto:galder@redhat.com">galder@redhat.com</a><br>
<a href="http://twitter.com/galderz" target="_blank">twitter.com/galderz</a><br>
<br>
Project Lead, Escalante<br>
<a href="http://escalante.io" target="_blank">http://escalante.io</a><br>
<br>
Engineer, Infinispan<br>
<a href="http://infinispan.org" target="_blank">http://infinispan.org</a><br>
<br>
<br>
_______________________________________________<br>
infinispan-dev mailing list<br>
<a href="mailto:infinispan-dev@lists.jboss.org">infinispan-dev@lists.jboss.org</a><br>
<a href="https://lists.jboss.org/mailman/listinfo/infinispan-dev" target="_blank">https://lists.jboss.org/mailman/listinfo/infinispan-dev</a><br>
</div></div></blockquote></div><br></div></div>