From 837330c6079702d20fe9f008dd87dd0535d48b4d Mon Sep 17 00:00:00 2001 From: rpatil Date: Fri, 15 Jun 2018 12:00:45 +0530 Subject: [PATCH] 8186171: HashMap: Entry.setValue may not work after Iterator.remove() called for previous entries Reviewed-by: martin Contributed-by: deepak.kejriwal@oracle.com --- src/share/classes/java/util/HashMap.java | 27 ++-- test/java/util/HashMap/Bug8186171Test.java | 157 +++++++++++++++++++++ 2 files changed, 173 insertions(+), 11 deletions(-) create mode 100644 test/java/util/HashMap/Bug8186171Test.java diff --git a/src/share/classes/java/util/HashMap.java b/src/share/classes/java/util/HashMap.java index d74cfc075..d73a6012d 100644 --- a/src/share/classes/java/util/HashMap.java +++ b/src/share/classes/java/util/HashMap.java @@ -491,7 +491,7 @@ public class HashMap extends AbstractMap } /** - * Implements Map.putAll and Map constructor + * Implements Map.putAll and Map constructor. * * @param m the map * @param evict false when initially constructing this map, else @@ -558,7 +558,7 @@ public class HashMap extends AbstractMap } /** - * Implements Map.get and related methods + * Implements Map.get and related methods. * * @param hash hash for key * @param key the key @@ -613,7 +613,7 @@ public class HashMap extends AbstractMap } /** - * Implements Map.put and related methods + * Implements Map.put and related methods. * * @param hash hash for key * @param key the key @@ -701,7 +701,7 @@ public class HashMap extends AbstractMap } threshold = newThr; @SuppressWarnings({"rawtypes","unchecked"}) - Node[] newTab = (Node[])new Node[newCap]; + Node[] newTab = (Node[])new Node[newCap]; table = newTab; if (oldTab != null) { for (int j = 0; j < oldCap; ++j) { @@ -801,7 +801,7 @@ public class HashMap extends AbstractMap } /** - * Implements Map.remove and related methods + * Implements Map.remove and related methods. * * @param hash hash for key * @param key the key @@ -1364,8 +1364,11 @@ public class HashMap extends AbstractMap } /** - * Reconstitute the {@code HashMap} instance from a stream (i.e., - * deserialize it). + * Reconstitutes this map from a stream (that is, deserializes it). + * @param s the stream + * @throws ClassNotFoundException if the class of a serialized object + * could not be found + * @throws IOException if an I/O error occurs */ private void readObject(java.io.ObjectInputStream s) throws IOException, ClassNotFoundException { @@ -1905,7 +1908,6 @@ public class HashMap extends AbstractMap /** * Forms tree of the nodes linked from this node. - * @return root of tree */ final void treeify(Node[] tab) { TreeNode root = null; @@ -2043,8 +2045,11 @@ public class HashMap extends AbstractMap return; if (root.parent != null) root = root.root(); - if (root == null || root.right == null || - (rl = root.left) == null || rl.left == null) { + if (root == null + || (movable + && (root.right == null + || (rl = root.left) == null + || rl.left == null))) { tab[index] = first.untreeify(map); // too small return; } @@ -2273,7 +2278,7 @@ public class HashMap extends AbstractMap static TreeNode balanceDeletion(TreeNode root, TreeNode x) { - for (TreeNode xp, xpl, xpr;;) { + for (TreeNode xp, xpl, xpr;;) { if (x == null || x == root) return root; else if ((xp = x.parent) == null) { diff --git a/test/java/util/HashMap/Bug8186171Test.java b/test/java/util/HashMap/Bug8186171Test.java new file mode 100644 index 000000000..75aa2816a --- /dev/null +++ b/test/java/util/HashMap/Bug8186171Test.java @@ -0,0 +1,157 @@ +/* + * 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. + * + * 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.List; +import java.util.Map; +import java.util.HashMap; +import java.util.Iterator; +import java.util.ArrayList; +import java.util.concurrent.ThreadLocalRandom; + +import org.testng.annotations.Test; +import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; +import static org.testng.Assert.assertTrue; + +/* + * @test + * @bug 8186171 + * @run testng Bug8186171Test + * @summary Verify the fix for scenario reported in JDK-8186171 + * @author deepak.kejriwal@oracle.com + */ +@Test +public class Bug8186171Test{ + + /** + * Tests and extends the scenario reported in + * https://bugs.openjdk.java.net/browse/JDK-8186171 + * HashMap: Entry.setValue may not work after Iterator.remove() called for previous entries + * Runs 1000 times as it is based on randomization. + */ + @Test(invocationCount = 1000) + static void testBug8186171NonDeterministic() + { + final ThreadLocalRandom rnd = ThreadLocalRandom.current(); + + final Object v1 = rnd.nextBoolean() ? null : 1; + final Object v2 = (rnd.nextBoolean() && v1 != null) ? null : 2; + + /** If true, always lands in first bucket in hash tables. */ + final boolean poorHash = rnd.nextBoolean(); + + class Key implements Comparable { + final int i; + Key(int i) { this.i = i; } + public int hashCode() { return poorHash ? 0 : super.hashCode(); } + public int compareTo(Key x) { + return Integer.compare(this.i, x.i); + } + } + + // HashMap and ConcurrentHashMap have: + // TREEIFY_THRESHOLD = 8; UNTREEIFY_THRESHOLD = 6; + final int size = rnd.nextInt(1, 25); + + List keys = new ArrayList<>(); + for (int i = size; i-->0; ) keys.add(new Key(i)); + Key keyToFrob = keys.get(rnd.nextInt(keys.size())); + + Map m = new HashMap(); + + for (Key key : keys) m.put(key, v1); + + for (Iterator> it = m.entrySet().iterator(); + it.hasNext(); ) { + Map.Entry entry = it.next(); + if (entry.getKey() == keyToFrob) + entry.setValue(v2); // does this have the expected effect? + else + it.remove(); + } + + assertFalse(m.containsValue(v1)); + assertTrue(m.containsValue(v2)); + assertTrue(m.containsKey(keyToFrob)); + assertEquals(1, m.size()); + } + + + /** + * Tests and extends the scenario reported in + * https://bugs.openjdk.java.net/browse/JDK-8186171 + * HashMap: Entry.setValue may not work after Iterator.remove() called for previous entries + * Runs single time by reproducing exact scenario for issue mentioned in 8186171 + */ + @Test() + static void testBug8186171Deterministic(){ + class Key implements Comparable + { + final int i; + Key(int i) { this.i = i; } + + @Override + public int hashCode() { return 0; } //Returning same hashcode so that all keys landup to same bucket + + @Override + public int compareTo(Key x){ + if(this.i == x.i){ + return 0; + } + else { + return Integer.compare(this.i, x.i); + } + } + @Override + public String toString() + { + return "Key_" + i; + } + } + + // HashMap have TREEIFY_THRESHOLD = 8; UNTREEIFY_THRESHOLD = 6; + final int size = 11; + List keys = new ArrayList<>(); + + for (int i = 0; i < size; i++){ + keys.add(new Key(i)); + } + + Key keyToFrob = keys.get(9); + Map m = new HashMap(); + for (Key key : keys) m.put(key, null); + + for (Iterator> it = m.entrySet().iterator(); it.hasNext(); ){ + Map.Entry entry = it.next(); + if (entry.getKey() == keyToFrob){ + entry.setValue(2); + } + else{ + it.remove(); + } + } + + assertFalse(m.containsValue(null)); + assertTrue(m.containsValue(2)); + assertTrue(m.containsKey(keyToFrob)); + assertEquals(1, m.size()); + } +} -- GitLab