Avoid wrong results when Object.hashCode() collides, use IdentityHashMap instead of HashMap when key is TestElement

See #693

Author: vlsi

src/components/src/main/java/org/apache/jmeter/config/CSVDataSet.java

@@ -35,6 +35,7 @@
 import org.apache.jmeter.testbeans.gui.GenericTestBeanCustomizer;
 import org.apache.jmeter.testelement.property.JMeterProperty;
 import org.apache.jmeter.testelement.property.StringProperty;
+import org.apache.jmeter.threads.AbstractThreadGroup;
 import org.apache.jmeter.threads.JMeterContext;
 import org.apache.jmeter.threads.JMeterVariables;
 import org.apache.jmeter.util.JMeterUtils;

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