vlsi commented on PR #6721:
URL: https://github.com/apache/jmeter/pull/6721#issuecomment-5023240160
Thanks for looking into this — reusing the path `TestCompiler` already
computed is the right instinct, and the tree walk it removes is genuinely
redundant.
## The description does not match the diff
The description explains a `parentControllersCache` in `JMeterThread`, a
nested `CachedPathToRootTraverser`, and a `HashMap<Sampler, List<Controller>>`.
None of that is in the diff, which instead exposes the existing
`SamplePackage.controllers` through `TestCompiler`. The benchmark numbers
therefore measured different code than what is proposed here. Could you rewrite
the description around the actual change and re-run the measurement against it?
JMeter has a JMH module at `src/core/src/jmh` — a benchmark there would be more
convincing than a one-off harness.
## Correctness
I walked through the equivalence and it holds:
* `JMeterThread` builds `compiler = new TestCompiler(testTree)` and
traverses the same `testTree` in `initRun`, so both paths see one tree and one
traversal order.
* Order matches: `saveSamplerConfigs` iterates `for (int i = stack.size(); i
> 0; i--)`, sampler to root, and `getControllersToRoot()` returns leaf to root.
That matters — `breakOnCurrentLoop` stops at the first `IteratingController`.
* Identity matches: `samplerConfigMap` is an `IdentityHashMap`, and the
traverser compares `node == nodeToFind`.
* After `findRealSampler`, the sampler has always been through
`configureSampler` (which would have thrown NPE otherwise), so the map lookup
cannot miss on the live path.
One behavioral difference worth calling out in the description: if the same
`Sampler` instance appears twice in the tree, the traverser keeps the first
occurrence (`stopRecording`) while `samplerConfigMap.put` keeps the last.
Obscure, but it changes silently.
## The empty-list case deserves a log line
Returning `List.of()` on a map miss is the right call, but it should not be
silent.
A miss is reachable in a degraded state, and it is worth tracing where the
failure lands. `executeSamplePackage` calls
`compiler.configureSampler(current)`, which NPEs on a missing package. That NPE
is caught inside `processSampler` and logged as `Error while processing
sampler`, and the thread keeps going. Execution then reaches the
`onErrorStartNextLoop` branch in `run()` and calls
`triggerLoopLogicalActionOnParentControllers` — which sits outside that try
block. So an exception thrown from `getControllersForSampler` propagates to
`catch (Exception | JMeterError e)` in `run()` and kills the thread.
That is a worse outcome than today's behavior, where the loop action is
skipped but the thread survives, and the underlying error has already been
reported by the earlier catch. Turning a degraded loop into a dead
load-generator thread is a behavior change that does not belong in a
performance PR.
I would keep the lookup total and put the invariant check at the call site:
```java
List<Controller> controllers =
compiler.getControllersForSampler(realSampler);
if (controllers.isEmpty()) {
log.warn("No parent controllers found for sampler '{}', loop action is
skipped", realSampler.getName());
}
consumer.accept(controllers);
```
This keeps the new public API total — an unknown sampler maps to an empty
list, and callers pick their own failure policy — while making the anomaly
greppable. If maintainers prefer fail-fast, `IllegalStateException` matching
the `realSampler == null` check a few lines above is the right shape, but it
should land as a separate commit so the behavior change gets reviewed as one.
## API surface
`SamplePackage.getControllers()` returns the live mutable list. That is
consistent with `getConfigs()` and `getTimers()`, so it is not a blocker, but
this is a new public method on a public class and we are committing to it
permanently. `TestCompiler` lives in the same `org.apache.jmeter.threads`
package, so a package-private getter would do the job. If it stays public,
please add `@API(status = ..., since = ...)` — that is the convention here.
Also: `xdocs/changes.xml` needs an entry, and after this change
`FindTestElementsUpToRootTraverser` has no callers left in the codebase. Worth
noting in the description; deprecating it can be a separate discussion.
## Generics
`Consumer<List<Controller>>` drops the `? super` the previous signature had.
Since the method feeds the consumer, PECS asks for:
```java
Consumer<? super List<Controller>> consumer
```
At the three current call sites this makes no practical difference — they
are method references, and inference handles them either way. But it costs
nothing and restores the original variance, so I would rather keep it.
The return type of `getControllersForSampler` is correct as
`List<Controller>`; a wildcard there would only inconvenience callers.
## Tests
This is my main ask. `TestJMeterThread` has no coverage for break, continue,
or `onErrorStartNextLoop` today, so the change moves untested logic onto a new
data source and rests entirely on "both paths produce the same list." Let's pin
that claim down.
**1. Equivalence test — cheapest and highest value.** Build a tree, traverse
it with both `TestCompiler` and `FindTestElementsUpToRootTraverser`, and assert
`assertIterableEquals(traverser.getControllersToRoot(),
compiler.getControllersForSampler(sampler))`. Order, not set membership.
Parameterize over tree shapes: sampler directly under the thread group;
`LoopController` → `GenericController` → `IfController` nesting; sampler under
a `TransactionController`, including nested transactions; non-controllers
(config, timer, assertion) interleaved, to confirm they stay out of the list;
two samplers sharing a parent; a sampler absent from the tree, which should
yield an empty list rather than throw.
**2. Behavioral tests through `JMeterThread`.**
`testBug63490EndTestWhenDelayIsTooLongForScheduler` already gives you the
harness: build a tree, run `new TestCompiler` plus traverse, construct a
`JMeterThread`, call `run()`. On top of that, add a spy controller that records
`startNextLoop`, `breakLoop`, and `triggerEndOfLoop` calls, then drive a
failing sampler with `onErrorStartNextLoop` and assert the same controllers
fire in the same order. One test per `TestLogicalAction` value
(`BREAK_CURRENT_LOOP`, `START_NEXT_ITERATION_OF_THREAD`,
`START_NEXT_ITERATION_OF_CURRENT_LOOP`), set from a PostProcessor via
`threadContext.setTestLogicalAction(...)`. Plus a nested-loop case: a break in
the inner loop must leave the outer one alone.
**3. `TransactionController` (bug 52968).** This is the riskiest path,
because `findRealSampler` unwraps the `TransactionSampler` down to the child
sampler, whose package comes from `samplerConfigMap` rather than
`transactionControllerConfigMap`. `bin/testfiles/Bug52968.jmx` covers it at the
batch level and is wired into `src/dist-check/build.gradle.kts` — please
confirm it passes. A JUnit test is still more useful, since it shows which
controllers received `triggerEndOfLoop` instead of only that the end result
matched.
Item 1 is what guards the specific thing this PR could break. Items 2 and 3
close a coverage gap that predates the PR, so they are a fair ask but not
strictly yours to fix.
--
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]
