Avoid wrong results when Object.hashCode()happen to collide

Use IdentityHashMap instead of HashMap when key is TestElement

See apache#693

Author: vlsi

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<AbstractThreadGroup, ThroughputInfo> threadGroupsInfoMap =
-            new ConcurrentHashMap<>();
+    //TestElements can't be used as keys in HashMap.
+    private static final Map<AbstractThreadGroup, ThroughputInfo> threadGroupsInfoMap =
+            Collections.synchronizedMap(new IdentityHashMap<>());
 
 
     /**

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<AbstractThreadGroup, EventProducer> groupEvents = new ConcurrentHashMap<>();
+
+    // TestElements can't be used as keys in a HashMap, so we use IdentityHashMap
+    private static final Map<AbstractThreadGroup, EventProducer> groupEvents =
+            Collections.synchronizedMap(new IdentityHashMap<>());
 
     /**
      * Desired throughput configured as {@code throughput/throughputPeriod} per second.

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 Map<TestElement, JMeterGUIComponent> nodesToGui = new HashMap<>();
+    private 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

@@ -37,7 +37,6 @@
 
 import javax.swing.JPopupMenu;
 
-import org.apache.commons.collections4.map.LRUMap;
 import org.apache.jmeter.assertions.Assertion;
 import org.apache.jmeter.assertions.gui.AbstractAssertionGui;
 import org.apache.jmeter.config.ConfigElement;
@@ -69,6 +68,9 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+
 /**
  * JMeter GUI element editing for TestBean elements.
  * <p>
@@ -109,7 +111,11 @@ public class TestBeanGUI extends AbstractJMeterGuiComponent implements JMeterGUI
      * needs to be limited, though, to avoid memory issues when editing very
      * large test plans.
      */
-    private final Map<TestElement, Customizer> customizers = new LRUMap<>(20);
+    private final Cache<TestElement, Customizer> customizers =
+            Caffeine.newBuilder()
+                    .weakKeys() // So test elements are compared using identity == rather than .equals
+                    .maximumSize(20)
+                    .build();
 
     /** Index of the customizer in the JPanel's child component list: */
     private int customizerIndexInPanel;
@@ -325,7 +331,7 @@ private void setValues(TestElement element) {
             if (initialized){
                 remove(customizerIndexInPanel);
             }
-            Customizer c = customizers.computeIfAbsent(element, e -> {
+            Customizer c = customizers.get(element, e -> {
                 Customizer result = createCustomizer();
                 result.setObject(propertyMap);
                 return result;

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;
@@ -50,7 +49,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();
 
@@ -135,9 +134,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/main/java/org/apache/jmeter/util/JSR223TestElement.java

@@ -58,8 +58,10 @@ public abstract class JSR223TestElement extends ScriptingTestElement
     /**
      * Cache of compiled scripts
      */
+    @SuppressWarnings("RedundantTypeArguments")
     private static final Map<String, CompiledScript> compiledScriptsCache =
-            Collections.synchronizedMap(
+            // Type arguments are needed to workaround https://bugs.openjdk.org/browse/JDK-8288590
+            Collections.<String, CompiledScript>synchronizedMap(
                     new LRUMap<>(JMeterUtils.getPropDefault("jsr223.compiled_scripts_cache_size", 100)));
 
     /** If not empty then script in ScriptText will be compiled and cached */

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;
 
@@ -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);
     }
 
     /**
@@ -96,7 +97,7 @@ private HashTree(Map<Object, HashTree> _map, Object key) {
         if(_map != null) {
             data = _map;
         } else {
-            data = new HashMap<>();
+            data = new IdentityHashMap<>();
         }
         if(key != null) {
             data.put(key, new HashTree());
@@ -213,7 +214,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 +228,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());
         }

xdocs/changes.xml

@@ -153,6 +153,7 @@ Summary
 <h3>General</h3>
 <ul>
   <li><bug>66157</bug><pr>719</pr>Correct theme for darklaf on rsyntaxtextarea</li>
+  <li><pr>693</pr>Avoid wrong results when <code>Object.hashCode()</code> happen to collide. Use <code>IdentityHashMap</code> instead of <code>HashMap</code> when key is <code>TestElement</code></li>
 </ul>
 
  <!--  =================== Thanks =================== -->