[infinispan-dev] [Pull Request] Modular Classloading Compatibility

Thu May 19 04:04:31 EDT 2011

2011/5/19 Dan Berindei <dan.berindei at gmail.com>:
> On Wed, May 18, 2011 at 7:06 PM, Manik Surtani <manik at jboss.org> wrote:
>> Hi guys
>>
>> Sorry I've been absent from this thread for a while now (it's been growing faster than I've been able to deal with email backlog!)
>>
>> Anyway, this is a very interesting discussion.  To summarise - as Pete did at some point - there are 2 goals here:
>>
>> 1.  Safe and intuitive use of an appropriate classloader
>> 2.  Safe type system for return values.
>>
>> I think the far more pressing concern is (1) so I'd like to focus on that.  If we think (2) is pressing enough a concern, we should spawn a separate thread and discuss there.
>>
>> So, onto the issue of safe classloading.
>>
>> 1) Class loader per session/cache.
>>
>> I like Jason/Sanne/Trustin's suggestions of a session-like contract, and specifically I think this is best achieved as a delegate to a cache, again as suggested elsewhere by Pete, etc.  E.g.,
>>
>>        Cache<?, ?> myCache = cacheManager.getCache("myCache", myClassLoader);
>>
>> and what is returned is something that delegates to the actual cache, making sure the TCCL is set and re-set appropriately.  The handle to the cache is effectively your "session" and each webapp, etc in an EE environment will have its own handle.  I propose using the TCCL as an internal implementation detail within this delegate, helps with making sure it is carefully managed and cleaned up while not re-engineering loads of internals.
>>
>
> I like the API but I would not recommend using the TCCL for this. I
> was able to get a nice perf jump in the HotRod client by skipping 2
> Thread.setContextClassLoader() calls on each cache operation (1 to set
> the TCCL we wanted and 1 to restore the original TCCL). Setting the
> TCCL is a privileged operation, so it has to go through a
> SecurityManager and that is very slow.
>
>
>> I think EmbeddedCacheManager.getCache(String name, ClassLoader cl) is enough ... it is clear enough, and I don't see the need for overloaded getCache(name, classOfWhichClassLoaderIWishToUse).
>>
>
> I agree, a Cache.usingClassloader(classOfWhichClassLoaderIWishToUse)
> overload would have made sense because the method name already
> communicates the intention, but a getCache(name, clazz) overload is
> too obscure.
>
>
>> 2) Class loader per invocation.
>>
>> I've been less than happy with this, not just because it pollutes the API but that it adds a layer of confusion.  If all use cases discussed can be solved with (1) above, then I'd prefer to just do that.
>>
>> The way I see it, most user apps directly using Infinispan would only be exposed to a single class loader per cache reference (even if multiple references talk to the same cache).
>>
>> Frameworks, OTOH, are a bit tougher, Hibernate being a good example on this thread.  So this is a question for Galder - is it feasible to maintain several references to a cache, one for each app/persistence unit?
>>
>> 3) Can all OSGi requirements be handled by (1)? I would guess so, from what I have read here, since the class loader is explicitly passed in when getting a handle on a cache.
>>
>
> Yes, the only difference is that OSGi goesn't make any guarantees
> about the TCCL, so passing the classloader explicitly will work in all
> environments. However,
>
> 4) What about storeAsBinary="false"? Threads processing requests from
> other nodes are not associated with any CacheManager.getCache(name,
> classloader) call, and they also have to unmarshall values with this
> setting.
>
> Since Hibernate already mandates storeAsBinary="true" for its 2LC, we
> can probably get away with only supporting one classloader per cache
> in the storeAsBinary="false" case.
>
> Still, we can't rely on the TCCL of the background threads because
> those threads are shared between all the caches in a CacheManager. In
> fact we should probably set the TCCL to null for all background
> threads created by Infinispan, or we risk keeping the classes of the
> first application/bundle that called us alive as long as those threads
> are still running.
>
> Dan

Totally agree with all you said, great analysis!

So it looks like that all Caches being shared across different
classloaders (deployed applications)
should be used only with storeAsBinary="false", still I'm having some
doubts about how even that is safe.
How can we prevent two applications both using hibernate from mixing
the cache keys and values?
I assume the keys being used in the 2LC use entity names or class
names combined with the primary keys;
it's totally possible to have multiple applications using the same
entity names, which is quite common in my experience.

The configurations defined in the app server could be used as
templates to start/stop specific caches for each deployment,
generating a unique name depending on the ear/war/.. in such a way to
be consistent with names on other nodes.
Was this all designed as a workaround for the fact that we're still
having trouble in forming a cluster of cachemanagers having different
caches?

Sanne

>
>
>> Cheers
>> Manik
>>
>>
>> On 27 Apr 2011, at 17:39, Jason T. Greene wrote:
>>
>>> Available here:
>>> https://github.com/infinispan/infinispan/pull/278
>>>
>>> The problem is basically that infinispan currently is using TCCL for all
>>> class loading and resource loading. This has a lot of problems in
>>> modular containers (OSGi, AS7, etc), where you dont have framework
>>> implementation classes on the same classloader as user classes (that is
>>> how they achieve true isolation)
>>>
>>> You can read about this in more detail here:
>>> http://community.jboss.org/wiki/ModuleCompatibleClassloadingGuide
>>>
>>> The patch in the pull request is a first step, and should fix many
>>> issues, but it does not address all (there is still a lot of TCCL usage
>>> spread out among cacheloaders and so on), and ultimately it's just a
>>> work around. It should, however, be compatible in any other non-modular
>>> environment.
>>>
>>> Really the ultimate solution is to setup a proper demarcation between
>>> what the user is supposed to provide, and what is expected to be bundled
>>> with infinispan. Whenever there is something the user can provide a
>>> class, then the API should accept a classloader to load that class from.
>>> If it's a class that is just internal wiring of infinispan, then
>>> Infinispan's defining classloader should always be used.
>>>
>>> The one case I can think of in infnispan where TCCL really makes sense
>>> is in the case of lazy deserialization from an EE application. However
>>> that is only guaranteed to work when you are executing in a context that
>>> has that style of contract (like in an EE component). There are many
>>> other cases though where someone would expect it to work from a non-EE
>>> context (e.g. from a thread pool). What you really want is the caller's
>>> classloader, but that is not cheap to get at, so it's something that
>>> should be supplied as part of the API interaction (in the case where
>>> there is no EE context). Alternatively you can just just require that
>>> folks push/pop TCCL, but users often get this wrong.
>>>
>>> Thanks!
>>>
>>> --
>>> Jason T. Greene
>>> JBoss, a division of Red Hat
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> infinispan-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>> --
>> Manik Surtani
>> manik at jboss.org
>> twitter.com/maniksurtani
>>
>> Lead, Infinispan
>> http://www.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
>