changeset 11714:0c3aa853064e

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 3b1241ecb05b
children 5a25329ab60b 194ce0d739b9 e57a739ba641
files src/java.base/share/classes/java/util/HashMap.java src/java.base/share/classes/java/util/Hashtable.java src/java.base/share/classes/java/util/Map.java test/java/util/Map/FunctionalCMEs.java
diffstat 4 files changed, 339 insertions(+), 33 deletions(-) [+]
line wrap: on
line diff
--- a/src/java.base/share/classes/java/util/HashMap.java	Thu Apr 02 11:54:33 2015 -0700
+++ b/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/src/java.base/share/classes/java/util/Hashtable.java	Thu Apr 02 11:54:33 2015 -0700
+++ b/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/src/java.base/share/classes/java/util/Map.java	Thu Apr 02 11:54:33 2015 -0700
+++ b/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/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");
+        }
+    }
+}