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.berin...@gmail.com> wrote: >> >> >> >> On Thu, Dec 12, 2013 at 12:52 AM, David M. Lloyd <david.ll...@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@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 > _______________________________________________ > 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