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

Use IdentityHashMap instead of HashMap when key is TestElement

Author: vlsi

src/core/src/main/java/org/apache/jmeter/control/GenericController.java

@@ -21,6 +21,7 @@
 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;
@@ -58,7 +59,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();
 
@@ -354,9 +355,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;
     }
@@ -414,7 +417,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,6 +37,9 @@
 
 import javax.swing.JPopupMenu;
 
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
+
 import org.apache.commons.collections4.map.LRUMap;
 import org.apache.jmeter.assertions.Assertion;
 import org.apache.jmeter.assertions.gui.AbstractAssertionGui;
@@ -109,7 +112,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 +332,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,6 +19,7 @@
 
 import java.io.Serializable;
 import java.time.Duration;
+import java.util.IdentityHashMap;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -50,7 +51,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 +136,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

@@ -20,6 +20,7 @@
 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;
@@ -69,10 +70,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;
 

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>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 =================== -->