On Wed, May 18, 2011 at 7:06 PM, Manik Surtani <ma...@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
> 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@lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> --
> Manik Surtani
> ma...@jboss.org
> twitter.com/maniksurtani
>
> Lead, Infinispan
> http://www.infinispan.org
>
>
>
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
_______________________________________________
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
