(thread) Provide reclaimable thread local values without Thread termination

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.
###@###.### 2005-04-13 11:12:41 GMT