From 6b320e8afe59af6abf51e1eb7c974c7c7125089c Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 9 May 2002 03:20:59 +0000 Subject: [PATCH] Fixed to have SequencedHashMap throw a ConcurrentModificationException from its iterators if the map is modified through something other than the iterator. git-svn-id: https://svn.apache.org/repos/asf/jakarta/commons/proper/collections/trunk@130700 13f79535-47bb-0310-9956-ffa450edef68 --- .../commons/collections/SequencedHashMap.java | 53 ++++++++++++++++--- .../commons/collections/TestLRUMap.java | 15 ++---- .../collections/TestSequencedHashMap.java | 20 +++---- 3 files changed, 62 insertions(+), 26 deletions(-) diff --git a/src/java/org/apache/commons/collections/SequencedHashMap.java b/src/java/org/apache/commons/collections/SequencedHashMap.java index b515a4f2b..5e7b7307b 100644 --- a/src/java/org/apache/commons/collections/SequencedHashMap.java +++ b/src/java/org/apache/commons/collections/SequencedHashMap.java @@ -1,7 +1,7 @@ /* - * $Header: /home/jerenkrantz/tmp/commons/commons-convert/cvs/home/cvs/jakarta-commons//collections/src/java/org/apache/commons/collections/SequencedHashMap.java,v 1.8 2002/02/22 04:58:17 mas Exp $ - * $Revision: 1.8 $ - * $Date: 2002/02/22 04:58:17 $ + * $Header: /home/jerenkrantz/tmp/commons/commons-convert/cvs/home/cvs/jakarta-commons//collections/src/java/org/apache/commons/collections/SequencedHashMap.java,v 1.9 2002/05/09 03:20:59 mas Exp $ + * $Revision: 1.9 $ + * $Date: 2002/05/09 03:20:59 $ * * ==================================================================== * @@ -77,6 +77,7 @@ import java.util.List; import java.util.Map; import java.util.Set; import java.util.NoSuchElementException; +import java.util.ConcurrentModificationException; /** * A map of objects whose mapping entries are sequenced based on the order in @@ -191,6 +192,14 @@ public class SequencedHashMap implements Map, Cloneable, Externalizable { **/ private HashMap entries; + /** + * Holds the number of modifications that have occurred to the map, + * excluding modifications made through a collection view's iterator + * (e.g. entrySet().iterator().remove()). This is used to create a + * fail-fast behavior with the iterators. + **/ + private transient long modCount = 0; + /** * Construct a new sequenced hash map with default initial size and load * factor. @@ -434,6 +443,7 @@ public class SequencedHashMap implements Map, Cloneable, Externalizable { // per Map.put(Object,Object) public Object put(Object key, Object value) { + modCount++; Object oldValue = null; @@ -468,6 +478,15 @@ public class SequencedHashMap implements Map, Cloneable, Externalizable { // per Map.remove(Object) public Object remove(Object key) { + modCount++; + return removeImpl(key); + } + + /** + * Removed an entry without changing the map's modification count. This + * method should only be called from a collection view's iterator + **/ + private Object removeImpl(Object key) { Entry e = (Entry)entries.remove(key); if(e == null) return null; removeEntry(e); @@ -494,6 +513,8 @@ public class SequencedHashMap implements Map, Cloneable, Externalizable { // per Map.clear() public void clear() { + modCount++; + // remove all from the underlying map entries.clear(); @@ -633,10 +654,17 @@ public class SequencedHashMap implements Map, Cloneable, Externalizable { private int returnType; /** - * Holds the "current" position in the iterator. when pos.next is the + * Holds the "current" position in the iterator. When pos.next is the * sentinel, we've reached the end of the list. **/ private Entry pos = sentinel; + + /** + * Holds the expected modification count. If the actual modification + * count of the map differs from this value, then a concurrent + * modification has occurred. + **/ + private transient long expectedModCount = modCount; /** * Construct an iterator over the sequenced elements in the order in which @@ -676,8 +704,14 @@ public class SequencedHashMap implements Map, Cloneable, Externalizable { * * @exception NoSuchElementException if there are no more elements in the * iterator. + * + * @exception ConcurrentModificationException if a modification occurs in + * the underlying map. **/ public Object next() { + if(modCount != expectedModCount) { + throw new ConcurrentModificationException(); + } if(pos.next == sentinel) { throw new NoSuchElementException(); } @@ -707,14 +741,21 @@ public class SequencedHashMap implements Map, Cloneable, Externalizable { * @exception IllegalStateException if there isn't a "last element" to be * removed. That is, if {@link #next()} has never been called, or if * {@link #remove()} was already called on the element. + * + * @exception ConcurrentModificationException if a modification occurs in + * the underlying map. **/ public void remove() { if((returnType & REMOVED_MASK) != 0) { throw new IllegalStateException("remove() must follow next()"); } + if(modCount != expectedModCount) { + throw new ConcurrentModificationException(); + } - // remove the entry - SequencedHashMap.this.remove(pos.getKey()); + // remove the entry by calling the removeImpl method which does not + // update the mod count. This allows the iterator to remain valid. + SequencedHashMap.this.removeImpl(pos.getKey()); // set the removed flag returnType = returnType | REMOVED_MASK; diff --git a/src/test/org/apache/commons/collections/TestLRUMap.java b/src/test/org/apache/commons/collections/TestLRUMap.java index cced30849..bba7d2ed1 100644 --- a/src/test/org/apache/commons/collections/TestLRUMap.java +++ b/src/test/org/apache/commons/collections/TestLRUMap.java @@ -1,7 +1,7 @@ /* - * $Header: /home/jerenkrantz/tmp/commons/commons-convert/cvs/home/cvs/jakarta-commons//collections/src/test/org/apache/commons/collections/TestLRUMap.java,v 1.19 2002/05/08 17:34:17 morgand Exp $ - * $Revision: 1.19 $ - * $Date: 2002/05/08 17:34:17 $ + * $Header: /home/jerenkrantz/tmp/commons/commons-convert/cvs/home/cvs/jakarta-commons//collections/src/test/org/apache/commons/collections/TestLRUMap.java,v 1.20 2002/05/09 03:20:59 mas Exp $ + * $Revision: 1.20 $ + * $Date: 2002/05/09 03:20:59 $ * * ==================================================================== * @@ -73,7 +73,7 @@ import java.util.HashMap; * * @author James Strachan * @author Morgan Delagrange - * @version $Id: TestLRUMap.java,v 1.19 2002/05/08 17:34:17 morgand Exp $ + * @version $Id: TestLRUMap.java,v 1.20 2002/05/09 03:20:59 mas Exp $ */ public class TestLRUMap extends TestSequencedHashMap { @@ -95,13 +95,6 @@ public class TestLRUMap extends TestSequencedHashMap return map; } - // had to override from TestSequencedHashMap, because the test performs a get - // inside a loop. Since get() alter the Map in this class, an infinite loop - // is produced - public void testSequenceMap() { - fail("trying to work out an infinite loop bug"); - } - public void testRemoveLRU() { LRUMap map2 = new LRUMap(3); map2.put(new Integer(1),"foo"); diff --git a/src/test/org/apache/commons/collections/TestSequencedHashMap.java b/src/test/org/apache/commons/collections/TestSequencedHashMap.java index fc5b0fb84..a198d5e25 100644 --- a/src/test/org/apache/commons/collections/TestSequencedHashMap.java +++ b/src/test/org/apache/commons/collections/TestSequencedHashMap.java @@ -137,18 +137,20 @@ implements TestMap.SupportsPut, TestMap.EntrySetSupportsRemove SequencedHashMap clone = (SequencedHashMap) labRat.clone(); assertEquals("Size of clone does not match original", labRat.size(), clone.size()); - Iterator origKeys = labRat.keySet().iterator(); - Iterator copiedKeys = clone.keySet().iterator(); - while (origKeys.hasNext()) { - Object origKey = origKeys.next(); - Object copiedKey = copiedKeys.next(); - assertEquals("Cloned key does not match orginal", - origKey, copiedKey); + Iterator origEntries = labRat.entrySet().iterator(); + Iterator copiedEntries = clone.entrySet().iterator(); + while (origEntries.hasNext()) { + Map.Entry origEntry = (Map.Entry)origEntries.next(); + Map.Entry copiedEntry = (Map.Entry)copiedEntries.next(); + assertEquals("Cloned key does not match original", + origEntry.getKey(), copiedEntry.getKey()); assertEquals("Cloned value does not match original", - labRat.get(origKey), clone.get(copiedKey)); + origEntry.getValue(), copiedEntry.getValue()); + assertEquals("Cloned entry does not match orginal", + origEntry, copiedEntry); } assertTrue("iterator() returned different number of elements than keys()", - !copiedKeys.hasNext()); + !copiedEntries.hasNext()); // Test sequence() List seq = labRat.sequence();