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
> 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
