Avoid wrong results when Object.hashCode() collides, use IdentityHash…

…Map instead of HashMap when key is TestElement

See #693

src/components/src/main/java/org/apache/jmeter/timers/ConstantThroughputTimer.java

@@ -37,6 +37,7 @@
 import org.apache.jmeter.threads.AbstractThreadGroup;
 import org.apache.jmeter.threads.JMeterContextService;
 import org.apache.jmeter.util.JMeterUtils;
+import org.apache.jorphan.collections.IdentityKey;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -103,10 +104,10 @@ 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<AbstractThreadGroup, ThroughputInfo> threadGroupsInfoMap =
+    //TestElements can't be used as keys in HashMap.
+    private static final ConcurrentMap<IdentityKey<AbstractThreadGroup>, ThroughputInfo> threadGroupsInfoMap =
             new ConcurrentHashMap<>();
 
-
     /**
      * Constructor for a non-configured ConstantThroughputTimer.
      */
@@ -198,13 +199,10 @@ private long calculateDelay() {
         case AllActiveThreadsInCurrentThreadGroup_Shared: //All threads in this group - alternate calculation
             final org.apache.jmeter.threads.AbstractThreadGroup group =
                 JMeterContextService.getContext().getThreadGroup();
-            ThroughputInfo groupInfo = threadGroupsInfoMap.get(group);
+            IdentityKey<AbstractThreadGroup> key = new IdentityKey<>(group);
+            ThroughputInfo groupInfo = threadGroupsInfoMap.get(key);
             if (groupInfo == null) {
-                groupInfo = new ThroughputInfo();
-                ThroughputInfo previous = threadGroupsInfoMap.putIfAbsent(group, groupInfo);
-                if (previous != null) { // We did not replace the entry
-                    groupInfo = previous; // so use the existing one
-                }
+                groupInfo = threadGroupsInfoMap.computeIfAbsent(key, (k) -> new ThroughputInfo());
             }
             delay = calculateSharedDelay(groupInfo,Math.round(msPerRequest));
             break;

src/components/src/main/java/org/apache/jmeter/timers/poissonarrivals/PreciseThroughputTimer.java

@@ -28,6 +28,7 @@
 import org.apache.jmeter.testelement.TestStateListener;
 import org.apache.jmeter.threads.AbstractThreadGroup;
 import org.apache.jmeter.timers.Timer;
+import org.apache.jorphan.collections.IdentityKey;
 import org.apache.jorphan.util.JMeterStopThreadException;
 import org.apiguardian.api.API;
 import org.slf4j.Logger;
@@ -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<AbstractThreadGroup, EventProducer> groupEvents = new ConcurrentHashMap<>();
+
+    // TestElements can't be used as keys in a HashMap, so we use IdentityHashMap
+    private static final ConcurrentMap<IdentityKey<AbstractThreadGroup>, EventProducer> groupEvents =
+            new ConcurrentHashMap<>();
 
     /**
      * Desired throughput configured as {@code throughput/throughputPeriod} per second.
@@ -134,9 +138,14 @@ public long delay() {
 
     private EventProducer getEventProducer() {
         AbstractThreadGroup tg = getThreadContext().getThreadGroup();
+        IdentityKey<AbstractThreadGroup> key = new IdentityKey<>(tg);
+        EventProducer eventProducer = groupEvents.get(key);
+        if (eventProducer != null) {
+            return eventProducer;
+        }
         Long seed = randomSeed == null || randomSeed == 0 ? null : randomSeed;
         return
-                groupEvents.computeIfAbsent(tg, x -> new ConstantPoissonProcessGenerator(
+                groupEvents.computeIfAbsent(key, x -> new ConstantPoissonProcessGenerator(
                         () -> PreciseThroughputTimer.this.getThroughput() / throughputPeriod,
                         batchSize, batchThreadDelay, this, seed, true));
     }

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<LoopIterationListener> iterationListeners = new ArrayDeque<>();
 
     // Only create the map if it is required
-    private transient ConcurrentMap<TestElement, Object> children = new ConcurrentHashMap<>();
+    private transient IdentityHashMap<TestElement, Object> 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;

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.
      * <p>This enables to associate {@link UnsharedComponent} UIs with their {@link TestElement}.</p>
      */
-    private final Map<TestElement, JMeterGUIComponent> nodesToGui = new HashMap<>();
+    private final IdentityHashMap<TestElement, JMeterGUIComponent> nodesToGui = new IdentityHashMap<>();
 
     /**
      * Map from Class to JMeterGUIComponent, mapping the Class of a GUI

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<TestElement, Customizer> 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();
 

src/core/src/main/java/org/apache/jmeter/testelement/AbstractTestElement.java

@@ -122,14 +122,12 @@ public boolean equals(Object o) {
         }
     }
 
-    // TODO temporary hack to avoid unnecessary bug reports for subclasses
-
     /**
      * {@inheritDoc}
      */
     @Override
-    public int hashCode(){
-        return System.identityHashCode(this);
+    public int hashCode() {
+        return propMap.hashCode();
     }
 
     /*

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<TestElement, Object> children = new ConcurrentHashMap<>();
+    private final transient IdentityHashMap<TestElement, Object> 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;
     }

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<TestElement> stack = new LinkedList<>();
 
-    private final Map<Sampler, SamplePackage> samplerConfigMap = new HashMap<>();
+    private final IdentityHashMap<Sampler, SamplePackage> samplerConfigMap = new IdentityHashMap<>();
 
-    private final Map<TransactionController, SamplePackage> transactionControllerConfigMap =
-            new HashMap<>();
+    private final IdentityHashMap<TransactionController, SamplePackage> transactionControllerConfigMap =
+            new IdentityHashMap<>();
 
     private final HashTree testTree;
 

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"
+        }
+    }
+}

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<Object, HashTree>, Cloneable
     private static final String FOUND = "found"; // $NON-NLS-1$
 
     // N.B. The keys can be either JMeterTreeNode or TestElement
-    protected final Map<Object, HashTree> data;
+    protected final IdentityHashMap<Object, HashTree> data;
 
     /**
      * Creates an empty new HashTree.
@@ -78,7 +79,7 @@ protected HashTree(Map<Object, HashTree> _map) {
      *            name of the new top-level node
      */
     public HashTree(Object key) {
-        this(new HashMap<Object, HashTree>(), key);
+        this(new IdentityHashMap<>(), key);
     }
 
     /**
@@ -94,9 +95,17 @@ public HashTree(Object key) {
      */
     private HashTree(Map<Object, HashTree> _map, Object key) {
         if(_map != null) {
-            data = _map;
+            if (_map instanceof IdentityHashMap) {
+                data = (IdentityHashMap<Object, HashTree>) _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<Object, HashTree> 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<Object, HashTree> 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,14 +913,24 @@ public int hashCode() {
      */
     @Override
     public boolean equals(Object o) {
+        if (o == this) {
+            return true;
+        }
         if (!(o instanceof HashTree)) {
             return false;
         }
         HashTree oo = (HashTree) o;
         if (oo.size() != this.size()) {
             return false;
         }
-        return data.equals(oo.data);
+        // data.equals(oo.data); uses reference identity for keys
+        for (Entry<Object, HashTree> entry : data.entrySet()) {
+            HashTree otherValue = oo.data.get(entry.getKey());
+            if (!entry.getValue().equals(otherValue)) {
+                return false;
+            }
+        }
+        return true;
     }
 
     /**