FULL PRODUCT VERSION :
java version "1.5.0"
Java(TM) 2 Runtime Environment, Standard Edition (build 1.5.0-b64)
Java HotSpot(TM) Client VM (build 1.5.0-b64, mixed mode, sharing)


ADDITIONAL OS VERSION INFORMATION :
Linux localhost.localdomain 2.4.20-8 #1 Thu Mar 13 17:54:28 EST 2003 i686 i686 i386 GNU/Linux


A DESCRIPTION OF THE PROBLEM :
If we create a ThreadLocal and set it to reference itself, either directly or indirectly, then until the thread terminates, the objects cannot be garbage collected.

The Thread.threadLocals weak map keeps a strong reference to the value of thread locals initialised in that thread. If the value in turn keeps a strong reference to the ThreadLocal instance, then key will never expire. In addition, if the ThreadLocal cannot otherwise be referenced, it requires some activity in threadLocals to clear.

As a very simple example the following leaks:

                ThreadLocal local = new ThreadLocal();
                local.set(local);
                local = null;

A more realistic, indeed typical, example is that a ThreadLocal is assigned to a static variable and has a value of a class that's ClassLoader also loaded the class with the static ThreadLocal reference. This is normal in, say, a web application. When the webapp is reloaded, the old classes are not collected, so long as the web server pools the same threads.

I have written a patch to fix the problem. The patch also fixes both causes of (my interpretation of) Bug #5025230.

Construction and setting of ThreadLocals becomes very significantly slower. However, sensible uses should not be setting the value often (I think) and this patch has the advantage that it ThreadLocal now actually appears to work correctly. Setting time could be reduced by using a value holder, at the expense of an extra layer of indirection for every get. A concurrent map could be used instead of synchronising, but the overhead would be excessive. Probably should use IdentityHashMap, actually.

This patch has not been reviewed nor significantly tested.

java/lang/Thread.java

<         threadLocals = null;
<         inheritableThreadLocals = null;
---
>         if (threadLocals != null) {
>             threadLocals.threadExited(this);
>             threadLocals = null;
>         }
>         if (inheritableThreadLocals != null) {
>             inheritableThreadLocals.threadExited(this);
>             inheritableThreadLocals = null;
>         }


java/lang/ThreadLocal.java

9a10
> import java.util.Map;
62a64,70
>     /**
>      * The sole purpose of this map is to ensure there is a strong reference
>      * to our entires in {link Thread#threadLocals} while there is a strong
>      * reference to this. This map should never be read from.
>      */
>     private final Map<Thread,T> threadValues =
>        new java.util.HashMap<Thread,T>(8);
124,131c132,155
<         Thread t = Thread.currentThread();
<         ThreadLocalMap map = getMap(t);
<         if (map != null)
<             return (T)map.get(this);
<
<         // Maps are constructed lazily.  if the map for this thread
<         // doesn't exist, create it, with this ThreadLocal and its
<         // initial value as its only entry.
---
>         ThreadLocalMap map = getMap(Thread.currentThread());
>         if (map != null) {
>             // Thread's map is already constructed.
>             WeakReference valueRef = map.get(this);
>             if (valueRef != null) {
>                 // Already initialised for this thread.
>                 return (T)valueRef.get();
>             }
>         }
>         // Not initialised, and map may not have been created yet.
>         // Maps are constructed lazily.
>         return initialize();
>     }
>
>     /**
>      * Initializes the value for this thread from {@link #initialValue()}.
>      * @return the current thread's value of this thread-local
>      */
>     private T initialize() {
>         // Typically the map for this thread will be created with ThreadLocal
>         // and its initial value as its only entry.
>         // For first ThreadLocal for this Thread, if #initialValue()
>         // initalizes another ThreadLocal then a map will be created by the time
>         // initialValue() returns. Fix for Bug #5025230.
133c157
<         createMap(t, value);
---
>         set(value);
147a172,177
>
>         // Ensure map has entry allocated.
>         synchronized (threadValues) {
>             threadValues.put(t, null);
>         }
>
148a179
>         WeakReference<T> valueRef = new WeakReference<T>(value);
150c181
<             map.set(this, value);
---
>             map.set(this, valueRef);
152c183,187
<             createMap(t, value);
---
>             createMap(t, valueRef);
>
>         synchronized (threadValues) {
>             threadValues.put(t, value);
>         }
164,165c199,201
<          ThreadLocalMap m = getMap(Thread.currentThread());
<          if (m != null)
---
>          Thread t = Thread.currentThread();
>          ThreadLocalMap m = getMap(t);
>          if (m != null) {
166a203,207
>
>             synchronized (threadValues) {
>                 threadValues.remove(t);
>             }
>          }
188c229
<     void createMap(Thread t, T firstValue) {
---
>     void createMap(Thread t, WeakReference<T> firstValue) {
237c278
<             private Object value;
---
>             private WeakReference value;
239c280
<             private Entry(ThreadLocal k, Object v) {
---
>             private Entry(ThreadLocal k, WeakReference v) {
292c333
<         ThreadLocalMap(ThreadLocal firstKey, Object firstValue) {
---
>         ThreadLocalMap(ThreadLocal firstKey, WeakReference firstValue) {
319c360
<                         Entry c = new Entry(key, value);
---
>                         Entry c = new Entry(key, new WeakReference(value));
328a370,385
>
>         /**
>          * Invoked as thread exits, so strong references to values through
>          * {link ThreadLocal.threadValues} can be cleared.
>          */
>         void threadExited(Thread t) {
>             for (Entry e : table) {
>                 ThreadLocal local = e.get();
>                 if (local != null) {
>                     Map<Thread,?> threadValues = local.threadValues;
>                     synchronized (threadValues) {
>                         threadValues.remove(this);
>                     }
>                 }
>             }
>         }
341c398
<         private Object get(ThreadLocal key) {
---
>         private WeakReference get(ThreadLocal key) {
359c416
<         private Object getAfterMiss(ThreadLocal key, int i, Entry e) {
---
>         private WeakReference getAfterMiss(ThreadLocal key, int i, Entry e) {
374,376c431
<             Object value = key.initialValue();
<             tab[i] = new Entry(key, value);
<             int sz = ++size;
---
>             int sz = size+1;
380c435
<             return value;
---
>             return null;
390c445
<         private void set(ThreadLocal key, Object value) {
---
>         private void set(ThreadLocal key, WeakReference value) {
463c518
<         private Object replaceStaleEntry(ThreadLocal key, Object value,
---
>         private WeakReference replaceStaleEntry(ThreadLocal key, WeakReference value,
517c572
<                 value = key.initialValue();
---
>                 value = new WeakReference(key.initialValue());

[This bug was brought to my attention by Alef Arendsen.]


STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
Compile the code below and run with:

java -Xmx8m Leak

java Loader

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
Leak:
10000000

Loader/Weakling:
..F.F.F.F....FFFF.F...FFF.....FFFFF..FF...................FFFFFFFFFFFFFFFFFFF...
...

ACTUAL -
Leak:
14562
Exception in thread "main" java.lang.OutOfMemoryError: Java heap space

Loader/Weakling:
..................................................................Exception in thread "main" java.lang.OutOfMemoryError: Java heap space



REPRODUCIBILITY :
This bug can be reproduced always.

---------- BEGIN SOURCE ----------
class Leak {
    public static void main(String[] args) {
        int ct = 0;
        try {
            for (; ct<10*1000*1000; ++ct) {
                ThreadLocal local = new ThreadLocal();
                local.set(local);
            }
        } finally {
            System.out.println(ct);
        }
    }
}



import java.net.URL;
 
class Loader {
    public static void main(String[] args) throws Exception {
        for (;;) {
            System.gc();
            System.out.print(".");
            System.out.flush();
            new java.net.URLClassLoader(
                new URL[] { new java.io.File(".").toURL() },
                ClassLoader.getSystemClassLoader().getParent()
            ).loadClass("Weakling").newInstance();
        }
    }
}
public class Weakling {
    private static ThreadLocal<Object> local;
    private static Weakling staticRef;
    private Object var = new byte[1000*1000];
    public Weakling() {
        local = new ThreadLocal<Object>();
        local.set(this);
        staticRef = this;
    }
     
    @Override
    protected void finalize() {
        System.out.print("F");
        System.out.flush();
    }
}

---------- END SOURCE ----------

CUSTOMER SUBMITTED WORKAROUND :
 o Avoid ThreadLocal.
 o Use ThreadLocal.remove when shutting down.
  xxxxx@xxxxx   2005-04-13 11:12:41 GMT

N/A

ThreadLocal simply wasn't designed for the kind of usage mentioned in the description. The effort and expertise put into the suggested fix is appreciated. However a redesign of the class to meet the new requirements would necessarily impose a large performance penalty on all users of ThreadLocal. This penalty can't be justified for the sake of the special use case.

This CR has been changed to a documentation change. The ThreadLocal.remove method doc will explain the importance of this method to avoid leakage of self-referential structures.
Posted Date : 2005-10-26 14:06:30.0

.
Posted Date : 2006-03-23 19:14:11.0

It was a big mistake to switch this change request to address the documentation improvement aspect instead of using a new, separate request for that change. That separate request will be filed and become a "see also" for this request. This request has been changed to a request for enhancement targeted to Dolphin. Excellent technical discussion has been going on between the ThreadLocal author and other experts and this CR's submitter. The submitter has prototyped a high quality solution that can subclass ThreadLocal and support effective reclamation of objects without requiring Thread termination or impacting users dependent on ThreadLocal characteristics. See the URL below for details. Exercise of this prototype solution would be very helpful to confirm its usefulness and help its author determine any areas needing further attention.

   http://jroller.com/page/tackline?entry=working_around_the_threadlocal_leak
Posted Date : 2006-05-08 14:48:23.0

The evaluation for this bug is without merit. Typical usage creates an indirect strong reference from values to ThreadLocal. ThreadLocal are usually assigned to a static variable and reference values are (or are a collection) of user loaded classes. At the bottom of this comment is a list of ThreadLocals, from the Java library itself, that would leak if they were loaded by a user class loader.

To force code that uses ThreadLocals to dispose of them is nuts, and right across the grain of Java. The code would be forced to expose dispose methods itself, and impose manual memory management on the code that used it. And so on up.

The performance argument is extremely weak, particularly with the patch supplied through the jdk-collaboration java.net project. get should run at around the same speed as the existing buggy code. set in the revised patch should be back up to speed. Creation shouldn't happen to often, but if it does I guess those ThreadLocals are usually not shared between threads, so that case is trivial to optimise if necessary.



com.sun.java.util.jar.pack.Utils.currentInstance
com.sun.jmx.snmp.ThreadContext.localContext
com.sun.org.apache.xml.internal.security.utils.CachedXPathAPIHolder.local
com.sun.org.apache.xml.internal.security.utils.CachedXPathAPIHolder.localDoc
com.sun.sql.QueryObjectGeneratorImpl.perThreadInstance
com.sun.tools.xjc.reader.Ring.instances
com.sun.xml.bind.v2.ClassFactory.tls
com.sun.xml.bind.v2.runtime.Coordinator.activeTable
com.sun.xml.messaging.saaj.util.transform.EfficientStreamingTransformer.effTransformer
com.sun.xml.ws.streaming.SourceReaderFactory.domStreamReader
com.sun.xml.ws.streaming.XMLStreamReaderFactory.fiStreamReader
com.sun.xml.ws.streaming.XMLStreamReaderFactory.xmlStreamReader
com.sun.xml.ws.streaming.XMLStreamWriterFactory.fiStreamWriter
java.util.TimeZone.defaultZoneTL
javax.management.QueryEval.server
sun.rmi.transport.tcp.TCPTransport.threadConnectionHandler
sun.rmi.transport.Transport.currentTransport
sun.security.jca.Providers.ProviderList

ClassFactory.tls is interesting in that it creates a WeakHashMap in which it appears that the value will always reference the key.

This bug usually makes reloading webapps in Tomcat leak your whole classloader's referenced memory. Since the ClassLoader will never be reaped, it will forever keep references to all loaded classes, which again keeps references to all statics, which again might hold object caches, and so on, and there is really no way to clean it up, as the ThreadLocals (and caches) might reside in 3rd party libs.

One might say that it is Tomcat's fault for "reusing" its thread-pool (not killing all threads on every webapp reload). But java shouldn't behave in this somewhat strange and unpredictable way at any rate.

"ThreadLocal simply wasn't designed for the kind of usage mentioned in the description." 

Perhaps. But a consequence of the way it was designed is that app servers crash with OutOfMemory errors due to unbounded growth in the permgen on a context reload.

"This penalty can't be justified for the sake of the special use case."

Reloading webapps in app servers is hardly a "special use case". The evaluation does not offer any insight into what use cases were considered in the original design, but I highly doubt that any of them could be more important than app server stability. Without stating what these use cases are and why app server stability should not be included in them, the evaluation is vacuous.

"However a redesign of the class to meet the new requirements would necessarily impose a large performance penalty on all users of ThreadLocal."

Without providing any specific use cases which motivate the design of ThreadLocal, the veracity of this statement is unverifiable.This arguement seems like premature optimization. Is there any evidence whatsoever that construction performance of ThreadLocals is so critical? I find this a far fetched conclusion.

Furthermore, simply stating that ThreadLocal was designed for other use cases does not invalidate the use case provided. If this is a "special case" of ThreadLocal, then the correct thing to do is to extend ThreadLocal to implement a solution for the special case. The patch provided could appears to be suitable for this via a class, perhaps called WeakThreadLocal that extends ThreadLocal.

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6558265