United StatesChange Country, Oracle Worldwide Web Sites Communities I am a... I want to...
JDK-6425537 : (coll) WeakHashMap thread safety

Details
Type:
Bug
Submit Date:
2006-05-13
Status:
Closed
Updated Date:
2013-12-16
Project Name:
JDK
Resolved Date:
2011-05-18
Component:
core-libs
OS:
generic
Sub-Component:
java.util:collections
CPU:
generic
Priority:
P3
Resolution:
Fixed
Affected Versions:
6
Fixed Versions:

Related Reports
Backport:

Sub Tasks

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



Hardware and Software, Engineered to Work Together