Avoid wrong results when Object.hashCode() collides, 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;
 
@@ -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;
     }
 
     /**

src/jorphan/src/main/java/org/apache/jorphan/collections/ListedHashTree.java

@@ -175,7 +175,9 @@ public Collection<Object> 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;
     }