Added try catch block for Pre, Post processors. - #6735
rajat315315 wants to merge 1 commit into
Conversation
ruthst00
left a comment
There was a problem hiding this comment.
Design Concern: Silently Hiding Failures
The current behaviour — letting an exception propagate up to processSampler() — means the sampler result is still recorded and the error is visible in the test results. With this change, a broken post-processor silently does nothing and the test continues as if everything is fine. This could mask real configuration or scripting errors from users.
Consider whether this should instead:
- Mark the sample result as failed/errored when a post-processor throws, or
- At minimum, be a configurable/opt-in behaviour rather than always-on.
The existing processAssertion() method handles this well as a reference: it catches exceptions but still records an assertion failure on the result. A similar approach for post-processors would be more transparent.
| } | ||
| TestBeanHelper.prepare((TestElement) ex); | ||
| ex.process(); | ||
| } catch (Exception | JMeterError e) { |
There was a problem hiding this 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:
} catch (JMeterStopTestException | JMeterStopTestNowException | JMeterStopThreadException e) {
throw e; // preserve control-flow semantics
} catch (Exception | JMeterError e) {
log.error("Error processing PostProcessor: {}", ((AbstractTestElement) ex).getName(), e);
}| } | ||
| TestBeanHelper.prepare((TestElement) ex); | ||
| ex.process(); | ||
| } catch (Exception | JMeterError e) { |
| return called; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Test Issues
-
DummySampler.sample()change is a side-effect: The PR changes the existingDummySampler.sample()to return a non-nullSampleResult(previously returnednull). This is needed for the new tests to work (so post-processors are actually invoked), but it silently changes the behaviour of the existingtestBug63490EndTestWhenDelayIsTooLongForSchedulertest. That test assertsassertFalse(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 newDummySamplervariant 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). -
Variable naming:
samplerControllerTree2intestPreProcessorExceptionHandling()uses a2suffix 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 namedsamplerControllerTree. -
Missing import ordering: The two new imports (
PostProcessor,PreProcessor) are inserted betweenThreadListenerandTimer, breaking the alphabetical ordering that the rest of the import block follows. They should be placed beforeThreadListener. -
No assertion on
LAST_SAMPLE_OK: The tests verify that processors and samplers are called, but don't verify thatJMeterThread.LAST_SAMPLE_OKis set correctly after a processor failure. Given that the change affects error handling in the sampling pipeline, this would strengthen the test coverage.
Fixes: #6734
Description
runPostProcessors(...)inorg.apache.jmeter.threads.JMeterThread:ex.process()calls intry-catch (Exception | JMeterError e)blocks.runPreProcessors(...)inorg.apache.jmeter.threads.JMeterThread:ex.process()calls intry-catch (Exception | JMeterError e)blocks to ensure pre-processor failures do not interrupt subsequent pre-processors or sample sampling.org.apache.jmeter.threads.TestJMeterThread:testPostProcessorExceptionHandling(): Verifies that when a PostProcessor throws an exception, remaining PostProcessors and sample pipeline steps continue executing.testPreProcessorExceptionHandling(): Verifies that when a PreProcessor throws an exception, remaining PreProcessors and the main sampler execute cleanly.Motivation and Context
Resolves an issue where an uncaught
RuntimeExceptionin aPreProcessororPostProcessorwould abort the execution loop, preventing subsequent processors from running and skipping sample assertions, listener notifications, and compiler state cleanup.How Has This Been Tested?
Ran Gradle test suite for core module:
./gradlew :src:core:test --tests "org.apache.jmeter.threads.TestJMeterThread"Types of changes
Checklist: