Java(TM) 2 Runtime Environment, Standard Edition (build 1.3.0-C)
Java HotSpot(TM) Server VM (build 2.0fcs-E, mixed mode)

The keys of ThreadLocals are added to, but never removed from, the
per thread Map.  Even if one explicitly sets the value to null the unique
thread local key is still in the Map.  This can create a problem for long
running threads.

This is a resubmit of a bug which was previously rejected because I did not
provide a test case.  Therefore I'm including the following test case which
will reproduce the problem:

public class test extends Thread {

  public static void main(String[] args) {
    test t = new test();
    t.start();
  }

  private static int counter = 0;

  public void run() {
    System.out.println("Starting");
    try {
      while (true) {
        makeNewThreadLocal();
      }
    } catch (OutOfMemoryError oome) {

      // Print this out in binary format to avoid doing any more allocation
      // after the OutOfMemoryError.  I'm too lazy to write a Hex converter
      // for a test case.

      System.out.print("Managed to allocate and release: '");
      System.out.write((counter >>> 24) & 0xFF);
      System.out.write((counter >>> 16) & 0xFF);
      System.out.write((counter >>>  8) & 0xFF);
      System.out.write((counter >>>  0) & 0xFF);
      System.out.println("' ThreadLocals before running out of memory");

      throw oome;
    }
  }

  // Note that when this method returns both the ThreadLocal and its value
  // will be candidates for collection.  The bug is that even when the
  // ThreadLocal is collected its key remains in the threadLocals Map.

  private void makeNewThreadLocal() {
    ThreadLocal t = new ThreadLocal();
    t.set(new Object());
    t.set(null);
    ++counter;
  }

}


When run this program produces an out of memory error:

Starting
Managed to allocate and release: ' ^Pj^C' ThreadLocals before running out of memory
java.lang.OutOfMemoryError
        <<no stack trace available>>
(Review ID: 116693) 
======================================================================

N/A

This is a performance issue.  For certain applications, a switch to
WeakHashMap might be warranted, as it does prevent a memory leak
in the rare case where an application generates unbounded numbers of
ThreadLocal instances.  However, it FAR more common that an applications uses
very few ThreadLocal instances and "references them" (i.e., calls the get()
method) very frequently.  Switching from IdentityHashMap to WeakHashMap would
make the performance of the common case far, far worse.

This is a space-for-time tradeoff - WeakHashMap is better for space, but the
advantage only shows up in applications that use huge numbers of ThreadLocals
IdentityHashMap is way, way faster than WeakHashMap, so applications using a
small, fixed number of ThreadLocals will fair much better with the current
implementation.

The best of both worlds would be to speed WeakHashMap up to the point where
its performance were comparable to that of IdentityHashMap (or perhaps to write
a weak variant of IdentityHashMap for internal use).  This may be possible for
Merlin Beta Refresh or Merlin FCS, but I doubt that it's possible for Merlin
Beta.

  xxxxx@xxxxx   2001-05-04

We have replaced the HashMap with a highly tuned weak identity hash map.  The resulting implementation fixes the memory leak and displays significantly iproved performance over the 1.3 (HashMap) implementation.

  xxxxx@xxxxx   2001-07-25

The OutOfMemoryError is not reproduced in merlin-beta2-b77.

  xxxxx@xxxxx   2001-10-23

The PostgreSQL JDBC driver uses ThreadLocal's. I've easily patched mine to not use it. Source available at www.postgresql.org and jdbc.postgresql.org.

It appears that changing the creation of HashMaps in java/lang/ThreadLocal.java (line 86 & 106) to creating 
WeakHashMaps will fix this problem. This means that ThreadLocal's "key" can be garbage collected if 
ThreadLocal has been garbage collected.

I don't think that using a WeakHashMap is a good solution.  
That will make thread locals a lot slower (because of the 
cost of traversing the weak ref).  I think doing something 
at finalization time is preferable.

Are you suggesting that the finalization of the ThreadLocal scan all the presently running Thread, removing 
their interest in the ThreadLocal? I think this would mean that a syncronized HashMap would be required.

I don't think that synchronizing between a finalizer thread 
and a normal thread is a good idea.  It is an easy way to 
deadlock the VM.  

I think that some mechanism like a reference queue 
implementation is the way to go.  Each thread will have a 
queue which it checks periodically (perhaps each time a 
thread local is accessed) for items it should remove from 
its local map.

I believe that it is possible to write a linked list such 
that puts (from the finalizer threads) are synchronized v. 
each other, but are not synchronized v. gets.  The trick 
here is to leave at least one element in the list at all 
times, then puts can append to this element while gets are 
reassigning the head.

Threadlocals can then be made to contain a list of all the 
threads they have been used by.  At finalization they 
enqueue their (now orphaned) key on the queues of all of 
the threads that have used it.  The next time a thread 
local is used by that thread it dequeues all but one 
(because of the synchronization issue) of the orphaned keys 
and removes them from the table before returning the 
requested value.

I disagree with the Evaluation (2001-05-04): This is not a simple choose of algorithm: fast and large 
memory usage verses slow and small memory usage. The present algorithm will cause certain application 
that worked under jdk-1.2.2 to fail (crash) under jdk-1.3.

My suggestion is to create another constructor that has a  
parameter suggesting that WeakHashMap be used rather than 
IdentityHashMap.

This would at least provide a workaround option for all 
long running, existing, commercial products that have 
been 'broken' because of Sun's undocumented change to the 
JDK.

This is a major problem with upward compatibility and needs 
to be fixed asap...!!!!!!

It causes us major problems. Please fix it.

It seems to me that sooner or later this will be a problem 
for any server code that uses ThreadLocal (don't we all 
intend for our serverside code to run virtually forever?).  
I think it might be more appropriate to 'mark' the 
IdentityHashMap entry as dead and allow the IdentityHashMap 
to be purged from time to time (garbage collection).

Was this fixed ? I have JDK 1.3.0_02 and I still see this 
problem.

I believe merlin-beta is JDK 1.4, so if you're still using 
1.3.0_02, you're still running the buggy code.