JDK-6258302 : (coll) LinkedHashIterator doesn't detect ConcurrentModificationEx during last elt
  • Type: Bug
  • Component: core-libs
  • Sub-Component: java.util:collections
  • Affected Version: 5.0
  • Priority: P4
  • Status: Closed
  • Resolution: Won't Fix
  • OS: windows_nt
  • CPU: x86
  • Submitted: 2005-04-20
  • Updated: 2012-10-08
  • Resolved: 2011-01-06
Related Reports
Relates :  
Description
FULL PRODUCT VERSION :
java version "1.4.2"
Java(TM) 2 Runtime Environment, Standard Edition (build 1.4.2-b28)
Java HotSpot(TM) Client VM (build 1.4.2-b28, mixed mode)

(Also checked 1.5.0 sources)

ADDITIONAL OS VERSION INFORMATION :
Windows NT Version 4.0

A DESCRIPTION OF THE PROBLEM :
The LinkedHashIterator, defined in java.util.LinkedHashMap, does not properly detect and throw a ConcurrentModificationException when iterating on the last element in the list.  If items are added while processing elements (1..n-1), it is thrown. But if while processing the last element, items are added, the Exception is not decected.

The Iterator for LinkedHashSet is LinkedHashMap.LinkedHashIterator. In LinkedHashIterator.nextEntry(), the last thing it does before returning the Entry e is to set a member var nextEntry to e.after. At the following call to hasNext(), it checks nextEntry != header.

So, if nextEntry() is currently returning the last element in the list, its .after is presently header. Now, if in processing the last element, something gets added to the list, the value being checked by hasNext() is the old nextEntry.after (which was the header but now has a value.) hasNext() should be looking at the current value of .after, not what it was last time nextEntry() was called. (perhaps it needs to instead look at lastReturned.after?) This would allow hasNext() to return true, then the subsequent nextEntry() to correctly detect and throw a ConModEx.

Actually, since the LinkedHashSet only adds to the end, I think nextEntry()'s condition for throwing a ConModEx should be (modCount < expectedModCount), which would detect removes but allow for adds.

STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
See program source.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
1
2
Caught expected ConcurrentModificationException
1
2
3
4
Caught expected ConcurrentModificationException


Actually, I debate whether an add() should trigger a ConModEx. I believe it's a safe action, in which case, I'd prefer to see:
1
2
3
4
1
2
3
4
5
with no Exceptions thrown.

ACTUAL -
1
2
Caught expected ConcurrentModificationException
1
2
3
4
Failed to catch expected ConcurrentModificationException

REPRODUCIBILITY :
This bug can be reproduced always.

---------- BEGIN SOURCE ----------
import java.util.ConcurrentModificationException;
import java.util.LinkedHashSet;
import java.util.Iterator;

public class LHSBug
{
  public LHSBug()
  {
  }
  
  public static void main(String[] args)
  {
    LinkedHashSet lhSet = new LinkedHashSet();
    Integer one = new Integer(1);
    Integer two = new Integer(2);
    Integer three = new Integer(3);
    Integer four = new Integer(4);
    Integer cinco = new Integer(5);
    
    // Add the three objects to the LinkedHashSet.
    // By its nature, the LinkedHashSet will iterate in
    // order of insertion.
    lhSet.add(one);
    lhSet.add(two);
    lhSet.add(three);
    
    // 1. Iterate over set. try to insert while processing the
    // second item. This should throw a ConcurrentModificationEx
    try
    {
      for (Iterator it = lhSet.iterator(); it.hasNext(); )
      {
        Integer num = (Integer) it.next();
        if (num == two)
        {
          lhSet.add(four);
        }
        System.out.println(num);
      }
    }
    catch (ConcurrentModificationException ex)
    {
      System.out.println("Caught expected ConcurrentModificationException");
    }
    
    // 2. Iterate again, this time inserting on the (old) 'last'
    //    element. This too should throw a ConcurrentModificationEx.
    //    But it doesn't.
    try
    {
      for (Iterator it = lhSet.iterator(); it.hasNext(); )
      {
        Integer num = (Integer) it.next();
        if (num == four)
        {
          lhSet.add(cinco);
        }
        System.out.println(num);
      }
      
      // The exception isn't detected in this case because the
      // LinkedHashIterator.nextEntry(), when processing elt four,
      // sets nextEntry = e.after. After the .next(), but before the
      // add, this is header. But the add() changes the .after for
      // elt four.   But this isn't detected, because the following call
      // to hasNext() checks the previously stored value of nextEntry.
      System.out.println("Failed to catch expected ConcurrentModificationException");
    }
    catch (ConcurrentModificationException ex)
    {
      System.out.println("Caught expected ConcurrentModificationException");
    }
    
    // Actually, I question why an add should trigger a ConModEx.
    // Since the Iterator is traversing the linked list, and items
    // are only added to the end of the list, an add should be safe.

  }
}
---------- END SOURCE ----------

CUSTOMER SUBMITTED WORKAROUND :
Either avoid using a LinkedHashSet.iterator() and get by index, or stop using LinkedHashSet and use another collection (both of which have performance implications)
###@###.### 2005-04-20 11:06:41 GMT

Comments
EVALUATION Per the decision on CR #4902078 this problem can unfortuantely not be fixed as it would cause additional problems. From the CR #4902078 evaluation: This change will not be made because a Sun-internal regulatory body rejected it. The formal ruling indicated that the change "has demonstrated the potential to have significant compatibility impact upon existing code." (The "compatibility impact" is that the fix has the potential to replace silent misbehavior with a ConcurrentModificationException.)
06-01-2011

EVALUATION Although there's never a guarantee that CME will be thrown, it's worth doing so when the check is inexpensive. The LinkedHashIterator implementation can easily be restructured to do this. CME should continue to be thrown when the list grows concurrently. While in many cases the client might be perfectly happy otherwise, it's more consistent and straightforward to report any concurrent modification that's detected.
15-02-2006

SUGGESTED FIX private abstract class LinkedHashIterator<T> implements Iterator<T> { Entry<K,V> entryBeforeNext = header; boolean removable = false; // is entryBeforeNext eligible for removal? /** * The modCount value that the iterator believes that the backing * List should have. If this expectation is violated, the iterator * has detected concurrent modification. */ int expectedModCount = modCount; public boolean hasNext() { return entryBeforeNext.after != header; } public void remove() { if (!removable) throw new IllegalStateException(); if (modCount != expectedModCount) throw new ConcurrentModificationException(); LinkedHashMap.this.remove(entryBeforeNext.key); removable = false; expectedModCount = modCount; } Entry<K,V> nextEntry() { if (modCount != expectedModCount) throw new ConcurrentModificationException(); entryBeforeNext = entryBeforeNext.after; if (entryBeforeNext == header) throw new NoSuchElementException(); removable = true; return entryBeforeNext; } }
15-02-2006