On Thu, Dec 12, 2013 at 12:52 AM, David M. Lloyd <david.lloyd@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.


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.

Dan

 
--
- DML
_______________________________________________
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev