United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
JDK-6254531 : (thread) Provide reclaimable thread local values without Thread termination

Details
Type:
Enhancement
Submit Date:
2005-04-13
Status:
Open
Updated Date:
2011-02-16
Project Name:
JDK
Resolved Date:
Component:
core-libs
OS:
linux
Sub-Component:
java.lang
CPU:
x86
Priority:
P3
Resolution:
Unresolved
Affected Versions:
5.0
Targeted Versions:

Related Reports
Relates:
Relates:
Relates:
Relates:
Relates:
Relates:

Sub Tasks

Description
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

                                    

Comments
EVALUATION

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.
                                     
2005-07-22
EVALUATION

.
                                     
2006-03-15
EVALUATION

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
                                     
2006-05-05



Hardware and Software, Engineered to Work Together