On Thu, Feb 14, 2013 at 4:43 PM, Galder Zamarreño <[email protected]> wrote:
> > > On Feb 8, 2013, at 3:35 PM, Dan Berindei <[email protected]> wrote: > > > > > > > > > On Fri, Feb 8, 2013 at 3:41 PM, Galder Zamarreño <[email protected]> > wrote: > > Hi all, > > > > We've got a small class loading puzzle to solve in our JSR-107 > implementation. > > > > 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: > > > https://github.com/jsr107/jsr107spec/blob/master/src/main/java/javax/cache/Caching.java > > > > 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. > > > > 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. > > > > 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...). > > > > One way to break this strong reference is for CMF implementation to hold > a weak reference on the CM as done here: > > > https://github.com/galderz/infinispan/blob/t_2639/jsr107/src/main/java/org/infinispan/jsr107/cache/InfinispanCacheManagerFactory.java#L56 > > > > 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). > > > > 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. > > > > I've found a good example of this in > https://github.com/jbossas/jboss-as/blob/master/controller-client/src/main/java/org/jboss/as/controller/client/impl/RemotingModelControllerClient.java- > 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. > > > > If anyone has any other thoughts, it'd be interesting to hear about them. > > > > > > > > The Caching javadoc seems to prohibit stopping the CacheManagers without > user intervention ( > https://github.com/jsr107/jsr107spec/blob/master/src/main/java/javax/cache/Caching.java#L35 > ): > > > > * Also keeps track of all CacheManagers created by the factory. > Subsequent calls > > * to {@link #getCacheManager()} return the same CacheManager. > > > > > > > > > > And in the javadoc of Caching.close() ( > https://github.com/jsr107/jsr107spec/blob/master/src/main/java/javax/cache/Caching.java#L153 > ): > > * All cache managers obtained from the factory are shutdown. > > * <p/> > > * Subsequent requests from this factory will return different cache > managers than would have been obtained before > > > > > > > > * shutdown. So for example > > * <pre> > > * CacheManager cacheManager = CacheFactory.getCacheManager(); > > > > > > > > * assertSame(cacheManager, CacheFactory.getCacheManager()); > > * CacheFactory.close(); > > > > > > > > * assertNotSame(cacheManager, CacheFactory.getCacheManager()); > > * </pre> > > > > 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(). > > 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. > > Yeah, that's true. 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... > 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. > > An alternative way to solve this is to have a weak hash map with > classloader as weak key: > > private final Map<ClassLoader, Map<String, InfinispanCacheManager>> > cacheManagers = > new WeakHashMap<ClassLoader, Map<String, > InfinispanCacheManager>>(); > > 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. > > I'm glad to hear that it works! 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... > 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. > > [1] > https://github.com/galderz/infinispan/blob/t_2639/jsr107/src/main/java/org/infinispan/jsr107/cache/InfinispanCacheManagerFactory.java#L56 > > > > > Cheers > > Dan > > > > > > Cheers, > > -- > > Galder Zamarreño > > [email protected] > > twitter.com/galderz > > > > Project Lead, Escalante > > http://escalante.io > > > > Engineer, Infinispan > > http://infinispan.org > > > > > > _______________________________________________ > > infinispan-dev mailing list > > [email protected] > > https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > > _______________________________________________ > > infinispan-dev mailing list > > [email protected] > > https://lists.jboss.org/mailman/listinfo/infinispan-dev > > > -- > Galder Zamarreño > [email protected] > twitter.com/galderz > > Project Lead, Escalante > http://escalante.io > > Engineer, Infinispan > http://infinispan.org > > > _______________________________________________ > infinispan-dev mailing list > [email protected] > https://lists.jboss.org/mailman/listinfo/infinispan-dev >
_______________________________________________ infinispan-dev mailing list [email protected] https://lists.jboss.org/mailman/listinfo/infinispan-dev
