[infinispan-dev] Clean ThreadLocals

[Pedro Ruivo]

On 12/12/2013 10:35 AM, Sanne Grinovero wrote:
> It all looks so much more complex than getting rid of this ThreadLocal?

I would like to remove it but, by the comments, it looks like an 
optimization. It is keeping 6 Marshall and 6 UnMarshall instances to 
avoid creating them each time you need to (un)marshall an object.

>
> On 12 December 2013 09:01, Dan Berindei <dan.berindei at gmail.com> wrote:
>>
>>
>>
>> On Thu, Dec 12, 2013 at 12:52 AM, David M. Lloyd <david.lloyd at redhat.com>
>> wrote:
>>>
>>> On 12/11/2013 04:47 PM, Pedro Ruivo wrote:
>>>> Hi,
>>>>
>>>> I've created a method to clean a specific ThreadLocal variable from all
>>>> live threads [1].
>>>>
>>>> My goal is to clean the ThreadLocal variables after a cache stops. It's
>>>> kind expensive method (it uses reflection) but I think it is fine.
>>>>
>>>> Currently, I'm using it to clear the Marshaller ThreadLocal in here [2].
>>>>
>>>> Does anyone see any problem with this approach? Maybe it can be used in
>>>> other possible leaking ThreadLocals?
>>>
>>> It's an interesting idea (I've done something similar in the past to
>>> simply drop all thread locals).  I would recommend that you check that
>>> all the JDKs you want to support use the same technique for thread
>>> locals though.  Because these fields are not part of the JDK, they may
>>> not always exist in all environments.
>>
>>
>> Yeah, the implementation of ThreadLocal has changed in the past and is
>> likely to change again in the future. If that happens and we have to support
>> different JDKs with different methods for clearing ThreadLocals, it won't be
>> pretty.

yep, both of you are right. I'm going to try another way :)

>>
>>>
>>> Also, it is important to only ever clean the thread locals of your
>>> current thread, or you're inviting all kinds of bizarre concurrency
>>> problems (maybe even locking threads into infinite loops), especially if
>>> the thread is running and actively using thread locals at the time.
>>
>>
>> Indeed, ThreadLocalMap doesn't look to be thread-safe. I'm not sure if a
>> remote() from another thread can cause an infinite loop like in HashMap
>> because it uses open addressing instead of chaining, but it looks like it
>> may cause a get() for a different ThreadLocal to return the wrong instance.
>> (I would be ok with it if it would only cause breakage for threads using the
>> cache that's being stopped, but it looks like it can touch more than one
>> entry.)
>>
>> I think we shouldn't concern ourselves with actually removing the
>> ThreadLocal from the map, we just have to keep track of the ThreadLocal
>> instances and clean any references to other objects they might have (a la
>> ExternalizerTableProxy). Or maybe give up on ThreadLocals and use a
>> ConcurrentWeakKeyHashMap<Thread, PerThreadInstanceHolder> instead.

I tried to use the proxy approach, but it looks worst since the core 
test suite started to throw OOME. I created a 
PerThreadInstanceHolderProxy with a reference to 
PerThreadInstanceHolder. Each time I created a Proxy, I put it in a list 
and in the stop I clear to PerThreadInstanceHolder.

I'll give it a try with the ConcurrentWeakKeyHashMap.

>>
>> Dan
>>
>>
>>>
>>> --
>>> - DML
>>> _______________________________________________
>>> 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
>