[infinispan-dev] Protecting ourselves against naive JSR-107 usages in app server environments

Thu Feb 14 12:58:25 EST 2013

On Thu, Feb 14, 2013 at 4:43 PM, Galder Zamarreño <galder at redhat.com> wrote:

>
>
> On Feb 8, 2013, at 3:35 PM, Dan Berindei <dan.berindei at gmail.com> wrote:
>
> >
> >
> >
> > On Fri, Feb 8, 2013 at 3:41 PM, Galder Zamarreño <galder at redhat.com>
> 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
> > galder at redhat.com
> > twitter.com/galderz
> >
> > Project Lead, Escalante
> > http://escalante.io
> >
> > Engineer, Infinispan
> > http://infinispan.org
> >
> >
> > _______________________________________________
> > infinispan-dev mailing list
> > infinispan-dev at lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
> >
> > _______________________________________________
> > infinispan-dev mailing list
> > infinispan-dev at lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
> --
> Galder Zamarreño
> galder at redhat.com
> twitter.com/galderz
>
> Project Lead, Escalante
> http://escalante.io
>
> Engineer, Infinispan
> http://infinispan.org
>
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/infinispan-dev/attachments/20130214/785c563a/attachment.html 