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

Thu May 19 06:09:43 EDT 2011

2011/5/19 Galder Zamarreño <galder at redhat.com>:
>
> On May 19, 2011, at 10:04 AM, Sanne Grinovero wrote:
>
>> 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?
>
> How do you do that? Apps don't have any direct access to the Hibernate Cache.
>
>> 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.
>
> That's what region name comes in. Multiple apps shouldn't have the same region name. That's what discriminates entities in different apps.

ah, thank you I finally understand why the configuration property
hibernate.cache.region_prefix
can be very useful.
Actually, it would be nice to document this explicitly as otherwise
people might end up deserializing a binary stream in the wrong
application..

Sanne

>
>>
>> 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
>>>
>>
>> _______________________________________________
>> infinispan-dev mailing list
>> infinispan-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> --
> Galder Zamarreño
> Sr. Software Engineer
> Infinispan, JBoss Cache
>
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>