8071667: HashMap.computeIfAbsent() adds entry that HashMap.get() does not find.
Summary: Throw ConcurrentModificationException from computeIfAbsent() & friends
Reviewed-by: chegar, psandoz
--- a/jdk/src/java.base/share/classes/java/util/HashMap.java Thu Apr 02 11:54:33 2015 -0700
+++ b/jdk/src/java.base/share/classes/java/util/HashMap.java Thu Apr 02 12:33:03 2015 -0700
@@ -1082,6 +1082,16 @@
return null;
}
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This method will, on a best-effort basis, throw a
+ * {@link ConcurrentModificationException} if it is detected that the
+ * mapping function modifies this map during computation.
+ *
+ * @throws ConcurrentModificationException if it is detected that the
+ * mapping function modified this map
+ */
@Override
public V computeIfAbsent(K key,
Function<? super K, ? extends V> mappingFunction) {
@@ -1115,7 +1125,9 @@
return oldValue;
}
}
+ int mc = modCount;
V v = mappingFunction.apply(key);
+ if (mc != modCount) { throw new ConcurrentModificationException(); }
if (v == null) {
return null;
} else if (old != null) {
@@ -1130,12 +1142,23 @@
if (binCount >= TREEIFY_THRESHOLD - 1)
treeifyBin(tab, hash);
}
- ++modCount;
+ modCount = mc + 1;
++size;
afterNodeInsertion(true);
return v;
}
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This method will, on a best-effort basis, throw a
+ * {@link ConcurrentModificationException} if it is detected that the
+ * remapping function modifies this map during computation.
+ *
+ * @throws ConcurrentModificationException if it is detected that the
+ * remapping function modified this map
+ */
+ @Override
public V computeIfPresent(K key,
BiFunction<? super K, ? super V, ? extends V> remappingFunction) {
if (remappingFunction == null)
@@ -1144,7 +1167,9 @@
int hash = hash(key);
if ((e = getNode(hash, key)) != null &&
(oldValue = e.value) != null) {
+ int mc = modCount;
V v = remappingFunction.apply(key, oldValue);
+ if (mc != modCount) { throw new ConcurrentModificationException(); }
if (v != null) {
e.value = v;
afterNodeAccess(e);
@@ -1156,6 +1181,16 @@
return null;
}
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This method will, on a best-effort basis, throw a
+ * {@link ConcurrentModificationException} if it is detected that the
+ * remapping function modifies this map during computation.
+ *
+ * @throws ConcurrentModificationException if it is detected that the
+ * remapping function modified this map
+ */
@Override
public V compute(K key,
BiFunction<? super K, ? super V, ? extends V> remappingFunction) {
@@ -1185,7 +1220,9 @@
}
}
V oldValue = (old == null) ? null : old.value;
+ int mc = modCount;
V v = remappingFunction.apply(key, oldValue);
+ if (mc != modCount) { throw new ConcurrentModificationException(); }
if (old != null) {
if (v != null) {
old.value = v;
@@ -1202,13 +1239,23 @@
if (binCount >= TREEIFY_THRESHOLD - 1)
treeifyBin(tab, hash);
}
- ++modCount;
+ modCount = mc + 1;
++size;
afterNodeInsertion(true);
}
return v;
}
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This method will, on a best-effort basis, throw a
+ * {@link ConcurrentModificationException} if it is detected that the
+ * remapping function modifies this map during computation.
+ *
+ * @throws ConcurrentModificationException if it is detected that the
+ * remapping function modified this map
+ */
@Override
public V merge(K key, V value,
BiFunction<? super V, ? super V, ? extends V> remappingFunction) {
@@ -1241,10 +1288,15 @@
}
if (old != null) {
V v;
- if (old.value != null)
+ if (old.value != null) {
+ int mc = modCount;
v = remappingFunction.apply(old.value, value);
- else
+ if (mc != modCount) {
+ throw new ConcurrentModificationException();
+ }
+ } else {
v = value;
+ }
if (v != null) {
old.value = v;
afterNodeAccess(old);
--- a/jdk/src/java.base/share/classes/java/util/Hashtable.java Thu Apr 02 11:54:33 2015 -0700
+++ b/jdk/src/java.base/share/classes/java/util/Hashtable.java Thu Apr 02 12:33:03 2015 -0700
@@ -1000,6 +1000,16 @@
return null;
}
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This method will, on a best-effort basis, throw a
+ * {@link java.util.ConcurrentModificationException} if the mapping
+ * function modified this map during computation.
+ *
+ * @throws ConcurrentModificationException if it is detected that the
+ * mapping function modified this map
+ */
@Override
public synchronized V computeIfAbsent(K key, Function<? super K, ? extends V> mappingFunction) {
Objects.requireNonNull(mappingFunction);
@@ -1016,7 +1026,9 @@
}
}
+ int mc = modCount;
V newValue = mappingFunction.apply(key);
+ if (mc != modCount) { throw new ConcurrentModificationException(); }
if (newValue != null) {
addEntry(hash, key, newValue, index);
}
@@ -1024,6 +1036,16 @@
return newValue;
}
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This method will, on a best-effort basis, throw a
+ * {@link java.util.ConcurrentModificationException} if the remapping
+ * function modified this map during computation.
+ *
+ * @throws ConcurrentModificationException if it is detected that the
+ * remapping function modified this map
+ */
@Override
public synchronized V computeIfPresent(K key, BiFunction<? super K, ? super V, ? extends V> remappingFunction) {
Objects.requireNonNull(remappingFunction);
@@ -1035,14 +1057,18 @@
Entry<K,V> e = (Entry<K,V>)tab[index];
for (Entry<K,V> prev = null; e != null; prev = e, e = e.next) {
if (e.hash == hash && e.key.equals(key)) {
+ int mc = modCount;
V newValue = remappingFunction.apply(key, e.value);
+ if (mc != modCount) {
+ throw new ConcurrentModificationException();
+ }
if (newValue == null) {
if (prev != null) {
prev.next = e.next;
} else {
tab[index] = e.next;
}
- modCount++;
+ modCount = mc + 1;
count--;
} else {
e.value = newValue;
@@ -1052,7 +1078,16 @@
}
return null;
}
-
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This method will, on a best-effort basis, throw a
+ * {@link java.util.ConcurrentModificationException} if the remapping
+ * function modified this map during computation.
+ *
+ * @throws ConcurrentModificationException if it is detected that the
+ * remapping function modified this map
+ */
@Override
public synchronized V compute(K key, BiFunction<? super K, ? super V, ? extends V> remappingFunction) {
Objects.requireNonNull(remappingFunction);
@@ -1064,14 +1099,18 @@
Entry<K,V> e = (Entry<K,V>)tab[index];
for (Entry<K,V> prev = null; e != null; prev = e, e = e.next) {
if (e.hash == hash && Objects.equals(e.key, key)) {
+ int mc = modCount;
V newValue = remappingFunction.apply(key, e.value);
+ if (mc != modCount) {
+ throw new ConcurrentModificationException();
+ }
if (newValue == null) {
if (prev != null) {
prev.next = e.next;
} else {
tab[index] = e.next;
}
- modCount++;
+ modCount = mc + 1;
count--;
} else {
e.value = newValue;
@@ -1080,7 +1119,9 @@
}
}
+ int mc = modCount;
V newValue = remappingFunction.apply(key, null);
+ if (mc != modCount) { throw new ConcurrentModificationException(); }
if (newValue != null) {
addEntry(hash, key, newValue, index);
}
@@ -1088,6 +1129,16 @@
return newValue;
}
+ /**
+ * {@inheritDoc}
+ *
+ * <p>This method will, on a best-effort basis, throw a
+ * {@link java.util.ConcurrentModificationException} if the remapping
+ * function modified this map during computation.
+ *
+ * @throws ConcurrentModificationException if it is detected that the
+ * remapping function modified this map
+ */
@Override
public synchronized V merge(K key, V value, BiFunction<? super V, ? super V, ? extends V> remappingFunction) {
Objects.requireNonNull(remappingFunction);
@@ -1099,14 +1150,18 @@
Entry<K,V> e = (Entry<K,V>)tab[index];
for (Entry<K,V> prev = null; e != null; prev = e, e = e.next) {
if (e.hash == hash && e.key.equals(key)) {
+ int mc = modCount;
V newValue = remappingFunction.apply(e.value, value);
+ if (mc != modCount) {
+ throw new ConcurrentModificationException();
+ }
if (newValue == null) {
if (prev != null) {
prev.next = e.next;
} else {
tab[index] = e.next;
}
- modCount++;
+ modCount = mc + 1;
count--;
} else {
e.value = newValue;
--- a/jdk/src/java.base/share/classes/java/util/Map.java Thu Apr 02 11:54:33 2015 -0700
+++ b/jdk/src/java.base/share/classes/java/util/Map.java Thu Apr 02 12:33:03 2015 -0700
@@ -894,8 +894,8 @@
* to {@code null}), attempts to compute its value using the given mapping
* function and enters it into this map unless {@code null}.
*
- * <p>If the function returns {@code null} no mapping is recorded. If
- * the function itself throws an (unchecked) exception, the
+ * <p>If the mapping function returns {@code null}, no mapping is recorded.
+ * If the mapping function itself throws an (unchecked) exception, the
* exception is rethrown, and no mapping is recorded. The most
* common usage is to construct a new object serving as an initial
* mapped value or memoized result, as in:
@@ -911,6 +911,7 @@
* map.computeIfAbsent(key, k -> new HashSet<V>()).add(v);
* }</pre>
*
+ * <p>The mapping function should not modify this map during computation.
*
* @implSpec
* The default implementation is equivalent to the following steps for this
@@ -925,16 +926,27 @@
* }
* }</pre>
*
+ * <p>The default implementation makes no guarantees about detecting if the
+ * mapping function modifies this map during computation and, if
+ * appropriate, reporting an error. Non-concurrent implementations should
+ * override this method and, on a best-effort basis, throw a
+ * {@code ConcurrentModificationException} if it is detected that the
+ * mapping function modifies this map during computation. Concurrent
+ * implementations should override this method and, on a best-effort basis,
+ * throw an {@code IllegalStateException} if it is detected that the
+ * mapping function modifies this map during computation and as a result
+ * computation would never complete.
+ *
* <p>The default implementation makes no guarantees about synchronization
* or atomicity properties of this method. Any implementation providing
* atomicity guarantees must override this method and document its
* concurrency properties. In particular, all implementations of
* subinterface {@link java.util.concurrent.ConcurrentMap} must document
- * whether the function is applied once atomically only if the value is not
- * present.
+ * whether the mapping function is applied once atomically only if the value
+ * is not present.
*
* @param key key with which the specified value is to be associated
- * @param mappingFunction the function to compute a value
+ * @param mappingFunction the mapping function to compute a value
* @return the current (existing or computed) value associated with
* the specified key, or null if the computed value is null
* @throws NullPointerException if the specified key is null and
@@ -967,10 +979,12 @@
* If the value for the specified key is present and non-null, attempts to
* compute a new mapping given the key and its current mapped value.
*
- * <p>If the function returns {@code null}, the mapping is removed. If the
- * function itself throws an (unchecked) exception, the exception is
- * rethrown, and the current mapping is left unchanged.
- *
+ * <p>If the remapping function returns {@code null}, the mapping is removed.
+ * If the remapping function itself throws an (unchecked) exception, the
+ * exception is rethrown, and the current mapping is left unchanged.
+ *
+ * <p>The remapping function should not modify this map during computation.
+ *
* @implSpec
* The default implementation is equivalent to performing the following
* steps for this {@code map}, then returning the current value or
@@ -987,16 +1001,27 @@
* }
* }</pre>
*
+ * <p>The default implementation makes no guarantees about detecting if the
+ * remapping function modifies this map during computation and, if
+ * appropriate, reporting an error. Non-concurrent implementations should
+ * override this method and, on a best-effort basis, throw a
+ * {@code ConcurrentModificationException} if it is detected that the
+ * remapping function modifies this map during computation. Concurrent
+ * implementations should override this method and, on a best-effort basis,
+ * throw an {@code IllegalStateException} if it is detected that the
+ * remapping function modifies this map during computation and as a result
+ * computation would never complete.
+ *
* <p>The default implementation makes no guarantees about synchronization
* or atomicity properties of this method. Any implementation providing
* atomicity guarantees must override this method and document its
* concurrency properties. In particular, all implementations of
* subinterface {@link java.util.concurrent.ConcurrentMap} must document
- * whether the function is applied once atomically only if the value is not
- * present.
+ * whether the remapping function is applied once atomically only if the
+ * value is not present.
*
* @param key key with which the specified value is to be associated
- * @param remappingFunction the function to compute a value
+ * @param remappingFunction the remapping function to compute a value
* @return the new value associated with the specified key, or null if none
* @throws NullPointerException if the specified key is null and
* this map does not support null keys, or the
@@ -1037,10 +1062,12 @@
* map.compute(key, (k, v) -> (v == null) ? msg : v.concat(msg))}</pre>
* (Method {@link #merge merge()} is often simpler to use for such purposes.)
*
- * <p>If the function returns {@code null}, the mapping is removed (or
- * remains absent if initially absent). If the function itself throws an
- * (unchecked) exception, the exception is rethrown, and the current mapping
- * is left unchanged.
+ * <p>If the remapping function returns {@code null}, the mapping is removed
+ * (or remains absent if initially absent). If the remapping function
+ * itself throws an (unchecked) exception, the exception is rethrown, and
+ * the current mapping is left unchanged.
+ *
+ * <p>The remapping function should not modify this map during computation.
*
* @implSpec
* The default implementation is equivalent to performing the following
@@ -1063,16 +1090,27 @@
* }
* }</pre>
*
+ * <p>The default implementation makes no guarantees about detecting if the
+ * remapping function modifies this map during computation and, if
+ * appropriate, reporting an error. Non-concurrent implementations should
+ * override this method and, on a best-effort basis, throw a
+ * {@code ConcurrentModificationException} if it is detected that the
+ * remapping function modifies this map during computation. Concurrent
+ * implementations should override this method and, on a best-effort basis,
+ * throw an {@code IllegalStateException} if it is detected that the
+ * remapping function modifies this map during computation and as a result
+ * computation would never complete.
+ *
* <p>The default implementation makes no guarantees about synchronization
* or atomicity properties of this method. Any implementation providing
* atomicity guarantees must override this method and document its
* concurrency properties. In particular, all implementations of
* subinterface {@link java.util.concurrent.ConcurrentMap} must document
- * whether the function is applied once atomically only if the value is not
- * present.
+ * whether the remapping function is applied once atomically only if the
+ * value is not present.
*
* @param key key with which the specified value is to be associated
- * @param remappingFunction the function to compute a value
+ * @param remappingFunction the remapping function to compute a value
* @return the new value associated with the specified key, or null if none
* @throws NullPointerException if the specified key is null and
* this map does not support null keys, or the
@@ -1121,9 +1159,11 @@
* map.merge(key, msg, String::concat)
* }</pre>
*
- * <p>If the function returns {@code null} the mapping is removed. If the
- * function itself throws an (unchecked) exception, the exception is
- * rethrown, and the current mapping is left unchanged.
+ * <p>If the remapping function returns {@code null}, the mapping is removed.
+ * If the remapping function itself throws an (unchecked) exception, the
+ * exception is rethrown, and the current mapping is left unchanged.
+ *
+ * <p>The remapping function should not modify this map during computation.
*
* @implSpec
* The default implementation is equivalent to performing the following
@@ -1140,19 +1180,31 @@
* map.put(key, newValue);
* }</pre>
*
+ * <p>The default implementation makes no guarantees about detecting if the
+ * remapping function modifies this map during computation and, if
+ * appropriate, reporting an error. Non-concurrent implementations should
+ * override this method and, on a best-effort basis, throw a
+ * {@code ConcurrentModificationException} if it is detected that the
+ * remapping function modifies this map during computation. Concurrent
+ * implementations should override this method and, on a best-effort basis,
+ * throw an {@code IllegalStateException} if it is detected that the
+ * remapping function modifies this map during computation and as a result
+ * computation would never complete.
+ *
* <p>The default implementation makes no guarantees about synchronization
* or atomicity properties of this method. Any implementation providing
* atomicity guarantees must override this method and document its
* concurrency properties. In particular, all implementations of
* subinterface {@link java.util.concurrent.ConcurrentMap} must document
- * whether the function is applied once atomically only if the value is not
- * present.
+ * whether the remapping function is applied once atomically only if the
+ * value is not present.
*
* @param key key with which the resulting value is to be associated
* @param value the non-null value to be merged with the existing value
* associated with the key or, if no existing value or a null value
* is associated with the key, to be associated with the key
- * @param remappingFunction the function to recompute a value if present
+ * @param remappingFunction the remapping function to recompute a value if
+ * present
* @return the new value associated with the specified key, or null if no
* value is associated with the key
* @throws UnsupportedOperationException if the {@code put} operation
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/jdk/test/java/util/Map/FunctionalCMEs.java Thu Apr 02 12:33:03 2015 -0700
@@ -0,0 +1,147 @@
+/*
+ * Copyright (c) 2015 Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation. Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+import java.util.Arrays;
+import java.util.ConcurrentModificationException;
+import java.util.HashMap;
+import java.util.Hashtable;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.function.BiFunction;
+
+import org.testng.annotations.Test;
+import org.testng.annotations.DataProvider;
+
+/**
+ * @test
+ * @bug 8071667
+ * @summary Ensure that ConcurrentModificationExceptions are thrown as specified from Map methods that accept Functions
+ * @author bchristi
+ * @build Defaults
+ * @run testng FunctionalCMEs
+ */
+public class FunctionalCMEs {
+ final static String KEY = "key";
+
+ @DataProvider(name = "Maps", parallel = true)
+ private static Iterator<Object[]> makeMaps() {
+ return Arrays.asList(
+ // Test maps that CME
+ new Object[]{new HashMap<>(), true},
+ new Object[]{new Hashtable<>(), true},
+ new Object[]{new LinkedHashMap<>(), true},
+ // Test default Map methods - no CME
+ new Object[]{new Defaults.ExtendsAbstractMap<>(), false}
+ ).iterator();
+ }
+
+ @Test(dataProvider = "Maps")
+ public void testComputeIfAbsent(Map<String,String> map, boolean expectCME) {
+ checkCME(() -> {
+ map.computeIfAbsent(KEY, k -> {
+ putToForceRehash(map);
+ return "computedValue";
+ });
+ }, expectCME);
+ }
+
+ @Test(dataProvider = "Maps")
+ public void testCompute(Map<String,String> map, boolean expectCME) {
+ checkCME(() -> {
+ map.compute(KEY, mkBiFunc(map));
+ }, expectCME);
+ }
+
+ @Test(dataProvider = "Maps")
+ public void testComputeWithKeyMapped(Map<String,String> map, boolean expectCME) {
+ map.put(KEY, "firstValue");
+ checkCME(() -> {
+ map.compute(KEY, mkBiFunc(map));
+ }, expectCME);
+ }
+
+ @Test(dataProvider = "Maps")
+ public void testComputeIfPresent(Map<String,String> map, boolean expectCME) {
+ map.put(KEY, "firstValue");
+ checkCME(() -> {
+ map.computeIfPresent(KEY, mkBiFunc(map));
+ }, expectCME);
+ }
+
+ @Test(dataProvider = "Maps")
+ public void testMerge(Map<String,String> map, boolean expectCME) {
+ map.put(KEY, "firstValue");
+ checkCME(() -> {
+ map.merge(KEY, "nextValue", mkBiFunc(map));
+ }, expectCME);
+ }
+
+ @Test(dataProvider = "Maps")
+ public void testForEach(Map<String,String> map, boolean ignored) {
+ checkCME(() -> {
+ map.put(KEY, "firstValue");
+ putToForceRehash(map);
+ map.forEach((k,v) -> {
+ map.remove(KEY);
+ });
+ }, true);
+ }
+
+ @Test(dataProvider = "Maps")
+ public void testReplaceAll(Map<String,String> map, boolean ignored) {
+ checkCME(() -> {
+ map.put(KEY, "firstValue");
+ putToForceRehash(map);
+ map.replaceAll((k,v) -> {
+ map.remove(KEY);
+ return "computedValue";
+ });
+ },true);
+ }
+
+ private static void checkCME(Runnable code, boolean expectCME) {
+ try {
+ code.run();
+ } catch (ConcurrentModificationException cme) {
+ if (expectCME) { return; } else { throw cme; }
+ }
+ if (expectCME) {
+ throw new RuntimeException("Expected CME, but wasn't thrown");
+ }
+ }
+
+ private static BiFunction<String,String,String> mkBiFunc(Map<String,String> map) {
+ return (k,v) -> {
+ putToForceRehash(map);
+ return "computedValue";
+ };
+ }
+
+ private static void putToForceRehash(Map<String,String> map) {
+ for (int i = 0; i < 64; ++i) {
+ map.put(i + "", "value");
+ }
+ }
+}