OpenJDK / jdk / hs
changeset 29743:981893a47bec
8071667: HashMap.computeIfAbsent() adds entry that HashMap.get() does not find.
Summary: Throw ConcurrentModificationException from computeIfAbsent() & friends
Reviewed-by: chegar, psandoz
| author | bchristi |
|---|---|
| date | Thu, 02 Apr 2015 12:33:03 -0700 |
| parents | b73f38796859 |
| children | 03803e001170 b7b1fab1fb2f ebd762f09224 |
| files | jdk/src/java.base/share/classes/java/util/HashMap.java jdk/src/java.base/share/classes/java/util/Hashtable.java jdk/src/java.base/share/classes/java/util/Map.java jdk/test/java/util/Map/FunctionalCMEs.java |
| diffstat | 4 files changed, 339 insertions(+), 33 deletions(-) [+] |
line wrap: on
line diff
--- 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"); + } + } +}