ruthst00 commented on code in PR #6735:
URL: https://github.com/apache/jmeter/pull/6735#discussion_r4066315928
##########
src/core/src/main/java/org/apache/jmeter/threads/JMeterThread.java:
##########
@@ -959,18 +959,29 @@ private static void processAssertion(SampleResult result,
Assertion assertion) {
private static void runPostProcessors(List<? extends PostProcessor>
extractors) {
for (PostProcessor ex : extractors) {
- TestBeanHelper.prepare((TestElement) ex);
- ex.process();
+ try {
+ if (log.isDebugEnabled()) {
+ log.debug("Running postprocessor: {}",
((AbstractTestElement) ex).getName());
+ }
+ TestBeanHelper.prepare((TestElement) ex);
+ ex.process();
+ } catch (Exception | JMeterError e) {
Review Comment:
`JMeterStopTestException`, `JMeterStopTestNowException`, and
`JMeterStopThreadException` all extend `RuntimeException`. The new `catch
(Exception | JMeterError e)` block will silently swallow these exceptions,
completely breaking the "Stop Test", "Stop Test Now", and "Stop Thread"
control-flow mechanisms when they are thrown from inside a pre- or
post-processor.
The existing code in `processSampler()` and `run()` carefully catches these
three exception types *before* the generic `Exception` catch, precisely to
handle them correctly. The new code does the opposite — it catches `Exception`
first and logs it as an error, discarding the intent entirely.
__Fix required:__ Re-throw these control-flow exceptions before logging the
generic error:
```java
} catch (JMeterStopTestException | JMeterStopTestNowException |
JMeterStopThreadException e) {
throw e; // preserve control-flow semantics
} catch (Exception | JMeterError e) {
log.error("Error processing PostProcessor: {}", ((AbstractTestElement)
ex).getName(), e);
}
```
##########
src/core/src/test/java/org/apache/jmeter/threads/TestJMeterThread.java:
##########
@@ -168,6 +172,114 @@ void testBug63490EndTestWhenDelayIsTooLongForScheduler() {
assertTrue(duration <= maxDuration, "Test plan should not run for
longer than duration");
}
+ @Test
+ void testPostProcessorExceptionHandling() {
+ JMeterContextService.getContext().setVariables(new JMeterVariables());
+
+ HashTree testTree = new HashTree();
+ LoopController samplerController = new LoopController();
+ samplerController.setLoops(1);
+ samplerController.setContinueForever(false);
+ samplerController.setEnabled(true);
+
+ DummySampler dummySampler = createSampler();
+ DummyPostProcessor failingPostProcessor = new DummyPostProcessor(true);
+ DummyPostProcessor secondPostProcessor = new DummyPostProcessor(false);
+
+ HashTree samplerControllerTree = testTree.add(samplerController);
+ HashTree samplerTree = samplerControllerTree.add(dummySampler);
+ samplerTree.add(failingPostProcessor);
+ samplerTree.add(secondPostProcessor);
+
+ ThreadGroup threadGroup = new ThreadGroup();
+ threadGroup.setNumThreads(1);
+
+ JMeterThread jMeterThread = new JMeterThread(testTree, threadGroup,
null);
+ jMeterThread.setThreadGroup(threadGroup);
+ jMeterThread.run();
+
+ assertTrue(dummySampler.isCalled(), "Sampler should be executed");
+ assertTrue(failingPostProcessor.isCalled(), "Failing post processor
should be executed");
+ assertTrue(secondPostProcessor.isCalled(), "Second post processor
should still be executed after exception in first");
+ }
+
+ @Test
+ void testPreProcessorExceptionHandling() {
+ JMeterContextService.getContext().setVariables(new JMeterVariables());
+
+ HashTree testTree = new HashTree();
+ LoopController samplerController = new LoopController();
+ samplerController.setLoops(1);
+ samplerController.setContinueForever(false);
+ samplerController.setEnabled(true);
+
+ DummySampler dummySampler = createSampler();
+ DummyPreProcessor failingPreProcessor = new DummyPreProcessor(true);
+ DummyPreProcessor secondPreProcessor = new DummyPreProcessor(false);
+
+ HashTree samplerControllerTree2 = testTree.add(samplerController);
+ HashTree samplerTree2 = samplerControllerTree2.add(dummySampler);
+ samplerTree2.add(failingPreProcessor);
+ samplerTree2.add(secondPreProcessor);
+
+ ThreadGroup threadGroup = new ThreadGroup();
+ threadGroup.setNumThreads(1);
+
+ JMeterThread jMeterThread = new JMeterThread(testTree, threadGroup,
null);
+ jMeterThread.setThreadGroup(threadGroup);
+ jMeterThread.run();
+
+ assertTrue(failingPreProcessor.isCalled(), "Failing pre processor
should be executed");
+ assertTrue(secondPreProcessor.isCalled(), "Second pre processor should
still be executed after exception in first");
+ assertTrue(dummySampler.isCalled(), "Sampler should still be executed
after preprocessor exception");
+ }
+
+ private static class DummyPostProcessor extends AbstractTestElement
implements PostProcessor {
+ private static final long serialVersionUID = 1L;
+ private final boolean shouldThrow;
+ private boolean called = false;
+
+ public DummyPostProcessor(boolean shouldThrow) {
+ this.shouldThrow = shouldThrow;
+ setEnabled(true);
+ }
+
+ @Override
+ public void process() {
+ called = true;
+ if (shouldThrow) {
+ throw new RuntimeException("PostProcessor test exception");
+ }
+ }
+
+ public boolean isCalled() {
+ return called;
+ }
+ }
+
+ private static class DummyPreProcessor extends AbstractTestElement
implements PreProcessor {
+ private static final long serialVersionUID = 1L;
+ private final boolean shouldThrow;
+ private boolean called = false;
+
+ public DummyPreProcessor(boolean shouldThrow) {
+ this.shouldThrow = shouldThrow;
+ setEnabled(true);
+ }
+
+ @Override
+ public void process() {
+ called = true;
+ if (shouldThrow) {
+ throw new RuntimeException("PreProcessor test exception");
+ }
+ }
+
+ public boolean isCalled() {
+ return called;
+ }
+ }
+
Review Comment:
### Test Issues
1. __`DummySampler.sample()` change is a side-effect:__ The PR changes the
existing `DummySampler.sample()` to return a non-null `SampleResult`
(previously returned `null`). This is needed for the new tests to work (so
post-processors are actually invoked), but it silently changes the behaviour of
the existing `testBug63490EndTestWhenDelayIsTooLongForScheduler` test. That
test asserts `assertFalse(dummySampler.isCalled())` — the sampler is never
reached due to the timer, so the change doesn't break it, but it's a fragile
coupling. The new `DummySampler` variant used in the new tests should be a
separate inner class (or the existing one should be left unchanged and a new
subclass created for the new tests).
2. __Variable naming:__ `samplerControllerTree2` in
`testPreProcessorExceptionHandling()` uses a `2` suffix that is a copy-paste
artifact from the post-processor test. Since it's a local variable in its own
method, it should just be named `samplerControllerTree`.
3. __Missing import ordering:__ The two new imports (`PostProcessor`,
`PreProcessor`) are inserted between `ThreadListener` and `Timer`, breaking the
alphabetical ordering that the rest of the import block follows. They should be
placed before `ThreadListener`.
4. __No assertion on `LAST_SAMPLE_OK`:__ The tests verify that processors
and samplers are called, but don't verify that `JMeterThread.LAST_SAMPLE_OK` is
set correctly after a processor failure. Given that the change affects error
handling in the sampling pipeline, this would strengthen the test coverage.
##########
src/core/src/main/java/org/apache/jmeter/threads/JMeterThread.java:
##########
@@ -959,18 +959,29 @@ private static void processAssertion(SampleResult result,
Assertion assertion) {
private static void runPostProcessors(List<? extends PostProcessor>
extractors) {
for (PostProcessor ex : extractors) {
- TestBeanHelper.prepare((TestElement) ex);
- ex.process();
+ try {
+ if (log.isDebugEnabled()) {
+ log.debug("Running postprocessor: {}",
((AbstractTestElement) ex).getName());
+ }
+ TestBeanHelper.prepare((TestElement) ex);
+ ex.process();
+ } catch (Exception | JMeterError e) {
+ log.error("Error processing PostProcessor: {}",
((AbstractTestElement) ex).getName(), e);
+ }
}
}
private static void runPreProcessors(List<? extends PreProcessor>
preProcessors) {
for (PreProcessor ex : preProcessors) {
- if (log.isDebugEnabled()) {
- log.debug("Running preprocessor: {}", ((AbstractTestElement)
ex).getName());
+ try {
+ if (log.isDebugEnabled()) {
+ log.debug("Running preprocessor: {}",
((AbstractTestElement) ex).getName());
+ }
+ TestBeanHelper.prepare((TestElement) ex);
+ ex.process();
+ } catch (Exception | JMeterError e) {
Review Comment:
Same comment as above.
--
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]
