3:06 p.m.
We've decided that rather than swap out the TCCL (which is frankly a bit error prone),
to fix this properly and have it so that each time a class is loaded it must select the
classloaders it wants. To help, we will add an advancedCache.getClassLoader(), which
defaults to the TCCL but can be overridden as described below.
As a first step, I have removed the TCCL from the default lookup in Util (AFAICT all class
loading was going through there), and required that, if you want to lookup app classes (as
opposed to Infinispan classes) you explicitly pass in a classloader. I have then passed in
the TCCL as a parameter throughout the codebase. This now makes it explicit where the TCCL
is being used and should mean that any new work does think about whether they need to look
at app classes or not.
Under this new scheme we have 45 refs to the TCCL in the codebase. Some of these are:
a) not needed, we are only ever loading system classes
b) can, with a bit of a refactor, refer to the classloader we select using
withClassLoader()
c) other (these will be harder to fix :-()
My plan is to go through each one, and chat with the owner of the code and discuss which
of (a), (b) or (c) is relevant here.
On 8 Jun 2011, at 10:48, Manik Surtani wrote:
Apologies for my long absence from this thread.
On 20 May 2011, at 15:28, Jason T. Greene wrote:
> On 5/18/11 11:06 AM, Manik Surtani wrote:
>
>>
>> 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);
>
> -snip-
>
> I would recommend leaving this open to store other per-session configuration values
(perhaps with a builder), and some of them mutable.
> This will allow you to completely eliminate the ThreadLocal context stuff used today
which is both faster and more robust (the gc will clean up the state for you).
Are you suggesting something like:
cacheManager.withClassLoader(myClassLoader).getCache("myCache");
thus allowing future expansions such as:
cacheManager.withClassLoader(myClassLoader).withSomeOtherParam(param).getCache()
?
I do like this, since it doesn't pollute the basic cache manager API methods like
getCache().
>
>> 2) Class loader per invocation.
>>
>
> If this "session" notion has some mutable values, you could also make CL
mutable:
>
> cacheSession.setClassLoader(blah);
> cacheSession.setFlags(FORCE_WRITE_LOCK)
>
Yes, no reason why the delegate Cache can't do this. These setters would need to
exist on the Cache interface though. For now, we should just restrict to
setClassLoader().
> From an implementation perspective, given where we are with 5.0 now, I suggest we
implement by holding the ClassLoader in the CacheDelegate impl, and each method invocation
impl would:
1) Set TCCL with the instance's ClassLoader field
2) Do work
3) In a finally block, reset TCCL.
This is just a temp measure, since I don't want to re-work how Marshallers, etc get a
hold of the class loader. They use TCCLs right now, and while sub-optimal in many ways,
this approach gives us an easy mechanism to implement while still preventing any leaks,
etc otherwise common with TCCLs.
Cheers
Manik
--
Manik Surtani
manik(a)jboss.org
twitter.com/maniksurtani
Lead, Infinispan
http://www.infinispan.org
_______________________________________________
infinispan-dev mailing list
infinispan-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev