milamberspace commented on code in PR #6721:
URL: https://github.com/apache/jmeter/pull/6721#discussion_r3598339852
##########
src/core/src/main/java/org/apache/jmeter/threads/JMeterThread.java:
##########
@@ -92,6 +94,8 @@ public class JMeterThread implements Runnable, Interruptible {
private final HashTree testTree;
+ private final Map<Sampler, List<Controller>> parentControllersCache = new
HashMap<>();
Review Comment:
**Correctness (blocking): keying this cache on `Sampler` changes the lookup
semantics.**
The original code locates the sampler in the tree by **reference identity**:
```java
// FindTestElementsUpToRootTraverser.addNode
if (node == nodeToFind) { this.stopRecording = true; } // ==, identity
```
But a `HashMap` keys on `equals`/`hashCode`, and `AbstractTestElement`
overrides both to be **value-based**:
```java
// AbstractTestElement.java
public boolean equals(Object o) { ... return other.propMap.equals(propMap); }
public int hashCode() { return propMap.hashCode(); }
```
Two real consequences:
1. **Collision → wrong controllers.** Two *distinct* samplers with identical
properties (copy-pasted requests, two default-named Debug Samplers, …) are
`.equals()` and share a bucket. On a cache hit the second sampler gets the
**first sampler's** path-to-root, so `breakOnCurrentLoop` /
`continueOnThreadLoop` / `startNextLoop` act on the wrong parent controllers on
error. The identity-based traversal never had this bug.
2. **Mutable key.** A sampler's `propMap` mutates during execution (variable
substitution, `runningVersion`, config merge), so its `hashCode` changes *while
it is a key* — violating the `Map` contract.
**Suggested fix:** use an identity-keyed map, which matches the traverser's
`==` semantics and is immune to property mutation.
```suggestion
private final Map<Sampler, List<Controller>> parentControllersCache =
new IdentityHashMap<>();
```
(Remember to `import java.util.IdentityHashMap;` and drop the now-unused
`HashMap` import.)
##########
src/core/src/main/java/org/apache/jmeter/threads/JMeterThread.java:
##########
@@ -343,6 +347,20 @@ public void run() {
}
}
+ private static class CachedPathToRootTraverser extends
FindTestElementsUpToRootTraverser {
Review Comment:
**Minor (design):** subclassing the traverser via `super(null)` just to
override `getControllersToRoot()` works only because every current consumer
calls that one method. A future consumer that touches the recorded stack would
get an empty deque from this cached instance. Since the cache is consumed
purely as a `List<Controller>`, consider passing the list directly to the
consumers instead of faking a traverser — or at minimum add a comment
documenting that only `getControllersToRoot()` is valid on this subclass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
