JDK-6425537 : (coll) WeakHashMap thread safety
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util:collections
  • Affected Version: 6
  • Priority: P3
  • Status: Closed
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2006-05-13
  • Updated: 2013-12-16
  • Resolved: 2011-05-18
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 6 JDK 7
6u75Fixed 7 b13Fixed
Description
Doug Lea writes:

---------
In an exchange with Hans Boehm, he pointed out that contrary to
reasonable expectations users probably have, a WeakHashMap that
is not being modified is NOT concurrently readable by multiple threads.

As he said...


>> On the other hand, with two threads concurrently reading the WeakHashMap
>> (e.g. by calling size()), two threads end up concurrently removing
>> enqueued elements from the WeakHashMap at the same time, which I very
>> strongly suspect can result in a mangled data structure.
>> 


The current wording of the javadoc spec might make you believe otherwise.
Rather than changing the spec, we should fix the code. It's easy --
just use a lock around the deletion code in expungeStaleEntries.
(And conveniently, the "queue" makes a perfectly good lock-object.
(The diffs below are mostly just indents.)

As I said to Hans...


>> It's a little weird
>> making this class only a little thread-safe, but you are right that as it
>> stands, someone will someday be unhappily surprised. 


Could you file a bug report on this.

% diff 1.6.0/j2se/martin/j2se/src/share/classes/java/util/WeakHashMap.java 
jsr166/src/main/java/util/
275,290c275,294
<             int h = e.hash;
<             int i = indexFor(h, table.length);
<
<             Entry<K,V> prev = table[i];
<             Entry<K,V> p = prev;
<             while (p != null) {
<                 Entry<K,V> next = p.next;
<                 if (p == e) {
<                     if (prev == e)
<                         table[i] = next;
<                     else
<                         prev.next = next;
<                     e.next = null;  // Help GC
<                     e.value = null; //  "   "
<                     size--;
<                     break;
---
 >             synchronized(queue) {
 >                 int h = e.hash;
 >                 int i = indexFor(h, table.length);
 >
 >                 Entry<K,V> prev = table[i];
 >                 Entry<K,V> p = prev;
 >                 while (p != null) {
 >                     Entry<K,V> next = p.next;
 >                     if (p == e) {
 >                         if (prev == e)
 >                             table[i] = next;
 >                         else
 >                             prev.next = next;
 >                         e.next = null;  // Help GC
 >                         e.value = null; //  "   "
 >                         size--;
 >                         break;
 >                     }
 >                     prev = p;
 >                     p = next;
292,293d295
<                 prev = p;
<                 p = next;
---------

Comments
EVALUATION The experts are right.
2006-05-13