From 90cde1cc5212d682620058a76784bbd79b50fb4f Mon Sep 17 00:00:00 2001 From: Vladimir Sitnikov Date: Sun, 30 Apr 2023 09:24:30 +0300 Subject: [PATCH] Avoid wrong results when Object.hashCode() collides, use IdentityHashMap instead of HashMap when key is TestElement See https://github.com/apache/jmeter/pull/693 --- .../timers/ConstantThroughputTimer.java | 10 ++-- .../PreciseThroughputTimer.java | 10 ++-- .../jmeter/control/GenericController.java | 16 +++--- .../org/apache/jmeter/gui/GuiPackage.java | 3 +- .../jmeter/testbeans/gui/TestBeanGUI.java | 3 +- .../jmeter/threads/AbstractThreadGroup.java | 13 ++--- .../apache/jmeter/threads/TestCompiler.java | 9 ++-- .../jorphan/collections/SearchByClassTest.kt | 41 +++++++++++++++ .../apache/jorphan/collections/HashTree.java | 52 ++++++++++++++++--- .../jorphan/collections/ListedHashTree.java | 18 +++++-- .../jorphan/collections/SearchByClass.java | 5 +- .../http/util/accesslog/SessionFilter.java | 4 +- .../util/accesslog/TestSessionFilter.java | 4 +- .../protocol/java/sampler/JavaSampler.java | 8 ++- xdocs/changes.xml | 1 + 15 files changed, 150 insertions(+), 47 deletions(-) create mode 100644 src/core/src/test/kotlin/org/apache/jorphan/collections/SearchByClassTest.kt diff --git a/src/components/src/main/java/org/apache/jmeter/timers/ConstantThroughputTimer.java b/src/components/src/main/java/org/apache/jmeter/timers/ConstantThroughputTimer.java index 945f22f5619..7f02f0b1145 100644 --- a/src/components/src/main/java/org/apache/jmeter/timers/ConstantThroughputTimer.java +++ b/src/components/src/main/java/org/apache/jmeter/timers/ConstantThroughputTimer.java @@ -20,9 +20,10 @@ import java.beans.BeanInfo; import java.beans.IntrospectionException; import java.beans.Introspector; +import java.util.Collections; +import java.util.IdentityHashMap; +import java.util.Map; import java.util.ResourceBundle; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; import org.apache.jmeter.gui.GUIMenuSortOrder; import org.apache.jmeter.gui.TestElementMetadata; @@ -103,8 +104,9 @@ public String toString() { private static final ThroughputInfo allThreadsInfo = new ThroughputInfo(); //For holding the ThroughputInfo objects for all ThreadGroups. Keyed by AbstractThreadGroup objects - private static final ConcurrentMap threadGroupsInfoMap = - new ConcurrentHashMap<>(); + //TestElements can't be used as keys in HashMap. + private static final Map threadGroupsInfoMap = + Collections.synchronizedMap(new IdentityHashMap<>()); /** diff --git a/src/components/src/main/java/org/apache/jmeter/timers/poissonarrivals/PreciseThroughputTimer.java b/src/components/src/main/java/org/apache/jmeter/timers/poissonarrivals/PreciseThroughputTimer.java index 48a6a75233d..d3261ffacb3 100644 --- a/src/components/src/main/java/org/apache/jmeter/timers/poissonarrivals/PreciseThroughputTimer.java +++ b/src/components/src/main/java/org/apache/jmeter/timers/poissonarrivals/PreciseThroughputTimer.java @@ -17,8 +17,9 @@ package org.apache.jmeter.timers.poissonarrivals; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; +import java.util.Collections; +import java.util.IdentityHashMap; +import java.util.Map; import java.util.concurrent.TimeUnit; import org.apache.jmeter.gui.GUIMenuSortOrder; @@ -44,7 +45,10 @@ public class PreciseThroughputTimer extends AbstractTestElement implements Clone private static final Logger log = LoggerFactory.getLogger(PreciseThroughputTimer.class); private static final long serialVersionUID = 4; - private static final ConcurrentMap groupEvents = new ConcurrentHashMap<>(); + + // TestElements can't be used as keys in a HashMap, so we use IdentityHashMap + private static final Map groupEvents = + Collections.synchronizedMap(new IdentityHashMap<>()); /** * Desired throughput configured as {@code throughput/throughputPeriod} per second. diff --git a/src/core/src/main/java/org/apache/jmeter/control/GenericController.java b/src/core/src/main/java/org/apache/jmeter/control/GenericController.java index 4d9d721395d..2fff0cf5f61 100644 --- a/src/core/src/main/java/org/apache/jmeter/control/GenericController.java +++ b/src/core/src/main/java/org/apache/jmeter/control/GenericController.java @@ -21,10 +21,9 @@ import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Deque; +import java.util.IdentityHashMap; import java.util.Iterator; import java.util.List; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; import org.apache.jmeter.engine.event.LoopIterationEvent; import org.apache.jmeter.engine.event.LoopIterationListener; @@ -58,7 +57,7 @@ public class GenericController extends AbstractTestElement implements Controller private transient Deque iterationListeners = new ArrayDeque<>(); // Only create the map if it is required - private transient ConcurrentMap children = new ConcurrentHashMap<>(); + private transient IdentityHashMap children = new IdentityHashMap<>(); private static final Object DUMMY = new Object(); @@ -353,10 +352,13 @@ public void addTestElement(TestElement child) { * {@inheritDoc} */ @Override + @SuppressWarnings("SynchronizeOnNonFinalField") public final boolean addTestElementOnce(TestElement child){ - if (children.putIfAbsent(child, DUMMY) == null) { - addTestElement(child); - return true; + synchronized (children) { + if (children.putIfAbsent(child, DUMMY) == null) { + addTestElement(child); + return true; + } } return false; } @@ -414,7 +416,7 @@ protected void resetIterCount() { protected Object readResolve(){ iterationListeners = new ArrayDeque<>(); - children = new ConcurrentHashMap<>(); + children = new IdentityHashMap<>(); subControllersAndSamplers = new ArrayList<>(); return this; diff --git a/src/core/src/main/java/org/apache/jmeter/gui/GuiPackage.java b/src/core/src/main/java/org/apache/jmeter/gui/GuiPackage.java index 2d3919cc88a..18e0a548e2b 100644 --- a/src/core/src/main/java/org/apache/jmeter/gui/GuiPackage.java +++ b/src/core/src/main/java/org/apache/jmeter/gui/GuiPackage.java @@ -24,6 +24,7 @@ import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; @@ -102,7 +103,7 @@ public final class GuiPackage implements LocaleChangeListener, HistoryListener { * to their corresponding GUI components. *

This enables to associate {@link UnsharedComponent} UIs with their {@link TestElement}.

*/ - private final Map nodesToGui = new HashMap<>(); + private final IdentityHashMap nodesToGui = new IdentityHashMap<>(); /** * Map from Class to JMeterGUIComponent, mapping the Class of a GUI diff --git a/src/core/src/main/java/org/apache/jmeter/testbeans/gui/TestBeanGUI.java b/src/core/src/main/java/org/apache/jmeter/testbeans/gui/TestBeanGUI.java index dfcce91dcb5..59c0fb4e9d9 100644 --- a/src/core/src/main/java/org/apache/jmeter/testbeans/gui/TestBeanGUI.java +++ b/src/core/src/main/java/org/apache/jmeter/testbeans/gui/TestBeanGUI.java @@ -112,7 +112,8 @@ public class TestBeanGUI extends AbstractJMeterGuiComponent implements JMeterGUI * large test plans. */ private final Cache customizers = - Caffeine.newBuilder() // TOOD: should this be made static? + Caffeine.newBuilder() // TODO: should this be made static? + .weakKeys() // So test elements are compared using identity == rather than .equals .maximumSize(20) .build(); diff --git a/src/core/src/main/java/org/apache/jmeter/threads/AbstractThreadGroup.java b/src/core/src/main/java/org/apache/jmeter/threads/AbstractThreadGroup.java index ea7c9d20923..16773bb50b0 100644 --- a/src/core/src/main/java/org/apache/jmeter/threads/AbstractThreadGroup.java +++ b/src/core/src/main/java/org/apache/jmeter/threads/AbstractThreadGroup.java @@ -19,8 +19,7 @@ import java.io.Serializable; import java.time.Duration; -import java.util.concurrent.ConcurrentHashMap; -import java.util.concurrent.ConcurrentMap; +import java.util.IdentityHashMap; import java.util.concurrent.atomic.AtomicInteger; import org.apache.jmeter.control.Controller; @@ -51,7 +50,7 @@ public abstract class AbstractThreadGroup extends AbstractTestElement private static final long serialVersionUID = 240L; // Only create the map if it is required - private final transient ConcurrentMap children = new ConcurrentHashMap<>(); + private final transient IdentityHashMap children = new IdentityHashMap<>(); private static final Object DUMMY = new Object(); @@ -136,9 +135,11 @@ public void addTestElement(TestElement child) { */ @Override public final boolean addTestElementOnce(TestElement child){ - if (children.putIfAbsent(child, DUMMY) == null) { - addTestElement(child); - return true; + synchronized (children) { + if (children.putIfAbsent(child, DUMMY) == null) { + addTestElement(child); + return true; + } } return false; } diff --git a/src/core/src/main/java/org/apache/jmeter/threads/TestCompiler.java b/src/core/src/main/java/org/apache/jmeter/threads/TestCompiler.java index e0c22641cb2..c3cc460c319 100644 --- a/src/core/src/main/java/org/apache/jmeter/threads/TestCompiler.java +++ b/src/core/src/main/java/org/apache/jmeter/threads/TestCompiler.java @@ -18,12 +18,11 @@ package org.apache.jmeter.threads; import java.util.ArrayList; -import java.util.HashMap; import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.LinkedList; import java.util.List; import java.util.ListIterator; -import java.util.Map; import java.util.Set; import org.apache.jmeter.assertions.Assertion; @@ -69,10 +68,10 @@ public class TestCompiler implements HashTreeTraverser { // TODO: replace with ArrayDequeue private final LinkedList stack = new LinkedList<>(); - private final Map samplerConfigMap = new HashMap<>(); + private final IdentityHashMap samplerConfigMap = new IdentityHashMap<>(); - private final Map transactionControllerConfigMap = - new HashMap<>(); + private final IdentityHashMap transactionControllerConfigMap = + new IdentityHashMap<>(); private final HashTree testTree; diff --git a/src/core/src/test/kotlin/org/apache/jorphan/collections/SearchByClassTest.kt b/src/core/src/test/kotlin/org/apache/jorphan/collections/SearchByClassTest.kt new file mode 100644 index 00000000000..655808fd0ea --- /dev/null +++ b/src/core/src/test/kotlin/org/apache/jorphan/collections/SearchByClassTest.kt @@ -0,0 +1,41 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.jorphan.collections + +import org.apache.jmeter.threads.AbstractThreadGroup +import org.apache.jmeter.threads.ThreadGroup +import org.junit.jupiter.api.Assertions +import org.junit.jupiter.api.Test + +class SearchByClassTest { + @Test + fun `search finds all matching elements`() { + val tree = ListedHashTree() + val count = 100000 + for (i in 0 until count) { + tree.add(ThreadGroup()) + } + val searcher = SearchByClass( + AbstractThreadGroup::class.java + ) + tree.traverse(searcher) + Assertions.assertEquals(count, searcher.searchResults.size) { + "Test plan included $count ThreadGroup elements, so SearchByClass should find all of them" + } + } +} diff --git a/src/jorphan/src/main/java/org/apache/jorphan/collections/HashTree.java b/src/jorphan/src/main/java/org/apache/jorphan/collections/HashTree.java index b3b56d56b6b..8d3879e7b0d 100644 --- a/src/jorphan/src/main/java/org/apache/jorphan/collections/HashTree.java +++ b/src/jorphan/src/main/java/org/apache/jorphan/collections/HashTree.java @@ -25,6 +25,7 @@ import java.util.Collection; import java.util.HashMap; import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.Map; import java.util.Set; @@ -54,7 +55,7 @@ public class HashTree implements Serializable, Map, Cloneable private static final String FOUND = "found"; // $NON-NLS-1$ // N.B. The keys can be either JMeterTreeNode or TestElement - protected final Map data; + protected final IdentityHashMap data; /** * Creates an empty new HashTree. @@ -78,7 +79,7 @@ protected HashTree(Map _map) { * name of the new top-level node */ public HashTree(Object key) { - this(new HashMap(), key); + this(new IdentityHashMap<>(), key); } /** @@ -94,9 +95,17 @@ public HashTree(Object key) { */ private HashTree(Map _map, Object key) { if(_map != null) { - data = _map; + if (_map instanceof IdentityHashMap) { + data = (IdentityHashMap) _map; + } else { + // Technically speaking, TestElements can't be placed in HashMapk keys, + // so we have to convert the map to an IdentityHashMap. + @SuppressWarnings("IdentityHashMapUsage") + IdentityHashMap identityMap = new IdentityHashMap<>(_map); + data = identityMap; + } } else { - data = new HashMap<>(); + data = new IdentityHashMap<>(); } if(key != null) { data.put(key, new HashTree()); @@ -213,7 +222,7 @@ public void add(HashTree newTree) { * a collection of objects to be added to the created HashTree. */ public HashTree(Collection keys) { - data = new HashMap<>(); + data = new IdentityHashMap<>(); for (Object o : keys) { data.put(o, new HashTree()); } @@ -227,7 +236,7 @@ public HashTree(Collection keys) { * array with names for the new top-level nodes */ public HashTree(Object[] keys) { - data = new HashMap<>(); + data = new IdentityHashMap<>(); for (Object key : keys) { data.put(key, new HashTree()); } @@ -873,7 +882,24 @@ protected HashTree getTreePath(Collection treePath) { */ @Override public int hashCode() { - return data.hashCode() * 7; + int xor = 0; + int sum = 0; + int mul = 0; + // Unordered hash function which uses reference equality for keys, and object equality for values + // We use several commutative functions to reduce the possibility of collisions. + // Note: IdentityHashMap.hashCode uses reference equality for both keys and values, so we can't use it. + for (Entry objectHashTreeEntry : data.entrySet()) { + int key = System.identityHashCode(objectHashTreeEntry.getKey()); + int value = objectHashTreeEntry.getValue().hashCode(); + xor = xor + key ^ value; + sum = sum + key + value; + mul = mul + (key|1) * (value|1); + } + int hash = 1; + hash = hash * 31 + xor; + hash = hash * 31 + sum; + hash = hash * 31 + mul; + return hash; } /** @@ -887,6 +913,9 @@ public int hashCode() { */ @Override public boolean equals(Object o) { + if (o == this) { + return true; + } if (!(o instanceof HashTree)) { return false; } @@ -894,7 +923,14 @@ public boolean equals(Object o) { if (oo.size() != this.size()) { return false; } - return data.equals(oo.data); + // data.equals(oo.data); uses reference identity for keys + for (Entry entry : data.entrySet()) { + HashTree otherValue = oo.data.get(entry.getKey()); + if (!entry.getValue().equals(otherValue)) { + return false; + } + } + return true; } /** diff --git a/src/jorphan/src/main/java/org/apache/jorphan/collections/ListedHashTree.java b/src/jorphan/src/main/java/org/apache/jorphan/collections/ListedHashTree.java index 1a7d58e34b9..8164b82b94e 100644 --- a/src/jorphan/src/main/java/org/apache/jorphan/collections/ListedHashTree.java +++ b/src/jorphan/src/main/java/org/apache/jorphan/collections/ListedHashTree.java @@ -175,7 +175,9 @@ public Collection list() { /** {@inheritDoc} */ @Override public HashTree remove(Object key) { - order.remove(key); + if (data.containsKey(key)) { + order.removeIf(x -> x == key); + } return data.remove(key); } @@ -190,7 +192,9 @@ public Object[] getArray() { @Override public int hashCode() { int hc = 17; - hc = hc * 37 + (order == null ? 0 : order.hashCode()); + for (Object o : order) { + hc = hc * 37 + System.identityHashCode(o); + } hc = hc * 37 + super.hashCode(); return hc; } @@ -202,7 +206,15 @@ public boolean equals(Object o) { return false; } ListedHashTree lht = (ListedHashTree) o; - return super.equals(lht) && order.equals(lht.order); + if (!super.equals(lht)) { + return false; + } + for (int i = 0; i < order.size(); i++) { + if (order.get(i) != lht.order.get(i)) { + return false; + } + } + return true; } diff --git a/src/jorphan/src/main/java/org/apache/jorphan/collections/SearchByClass.java b/src/jorphan/src/main/java/org/apache/jorphan/collections/SearchByClass.java index 22786324935..2d676239eb6 100644 --- a/src/jorphan/src/main/java/org/apache/jorphan/collections/SearchByClass.java +++ b/src/jorphan/src/main/java/org/apache/jorphan/collections/SearchByClass.java @@ -19,9 +19,8 @@ import java.util.ArrayList; import java.util.Collection; -import java.util.HashMap; +import java.util.IdentityHashMap; import java.util.List; -import java.util.Map; /** * Useful for finding all nodes in the tree that represent objects of a @@ -54,7 +53,7 @@ public class SearchByClass implements HashTreeTraverser { private final List objectsOfClass = new ArrayList<>(); - private final Map subTrees = new HashMap<>(); + private final IdentityHashMap subTrees = new IdentityHashMap<>(); private final Class searchClass; diff --git a/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/util/accesslog/SessionFilter.java b/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/util/accesslog/SessionFilter.java index 6ffef587c35..e7ed3b6d5ec 100644 --- a/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/util/accesslog/SessionFilter.java +++ b/src/protocol/http/src/main/java/org/apache/jmeter/protocol/http/util/accesslog/SessionFilter.java @@ -19,7 +19,7 @@ import java.io.Serializable; import java.util.Collections; -import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -69,7 +69,7 @@ public class SessionFilter implements Filter, Serializable, TestCloneable,Thread * Creates a new SessionFilter and initializes its fields to new collections */ public SessionFilter() { - this(new ConcurrentHashMap<>(), Collections.synchronizedSet(new HashSet<>())); + this(new ConcurrentHashMap<>(), Collections.synchronizedSet(Collections.newSetFromMap(new IdentityHashMap<>()))); } /** diff --git a/src/protocol/http/src/test/java/org/apache/jmeter/protocol/http/util/accesslog/TestSessionFilter.java b/src/protocol/http/src/test/java/org/apache/jmeter/protocol/http/util/accesslog/TestSessionFilter.java index 04b852ca4b4..48b3849bd5a 100644 --- a/src/protocol/http/src/test/java/org/apache/jmeter/protocol/http/util/accesslog/TestSessionFilter.java +++ b/src/protocol/http/src/test/java/org/apache/jmeter/protocol/http/util/accesslog/TestSessionFilter.java @@ -21,7 +21,7 @@ import static org.junit.Assert.assertTrue; import java.util.Collections; -import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -66,7 +66,7 @@ public void testGetCookieManagerLastUse() { public void testIsFiltered() throws Exception { Map cm = new ConcurrentHashMap<>(); Set inUse = Collections - .synchronizedSet(new HashSet()); + .synchronizedSet(Collections.newSetFromMap(new IdentityHashMap<>())); SessionFilter filter = new SessionFilter(cm, inUse); HTTPSampler sampler = new HTTPSampler(); filter.isFiltered("1.2.3.4 ...", sampler); diff --git a/src/protocol/java/src/main/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java b/src/protocol/java/src/main/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java index 0d8534bebb3..ec0393872a8 100644 --- a/src/protocol/java/src/main/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java +++ b/src/protocol/java/src/main/java/org/apache/jmeter/protocol/java/sampler/JavaSampler.java @@ -21,8 +21,8 @@ import java.util.Arrays; import java.util.Collections; import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import org.apache.jmeter.config.Arguments; import org.apache.jmeter.config.ConfigTestElement; @@ -59,7 +59,11 @@ public class JavaSampler extends AbstractSampler implements TestStateListener, I * This is used so that the JavaSamplerClient can be notified when the test ends. */ private static final Set TEAR_DOWN_SET = - Collections.newSetFromMap(new ConcurrentHashMap()); + Collections.synchronizedSet( + Collections.newSetFromMap( + new IdentityHashMap<>() + ) + ); /** * Property key representing the classname of the JavaSamplerClient to user. diff --git a/xdocs/changes.xml b/xdocs/changes.xml index 7d28fa3cb88..ce782ec80be 100644 --- a/xdocs/changes.xml +++ b/xdocs/changes.xml @@ -191,6 +191,7 @@ Summary
  • 66157719Correct theme for darklaf on rsyntaxtextarea
  • 58725874Trim name in Argument objects.
    • +
  • 693Avoid wrong results when Object.hashCode() happen to collide. Use IdentityHashMap instead of HashMap when key is TestElement