allthingssecurity commented on PR #29409: URL: https://github.com/apache/camel/pull/29409#issuecomment-6719213303
Thanks Claus, this is a very useful review. Two new commits (no rebase): - **Blocker** (2738e4a): `ManagedSanitizerTest` now uses `addStepHook(this::beforeStep)`. The branch already sat on top of main with #29386, so no merge was needed. I also built and ran camel-test-main-junit6 locally (4 tests, 0 failures), since CI never got that far. The rest is in dc9305a: - **Bound of 1000**: agreed. Past the bound, a violation is still charged to the test session. It is no longer logged or listed in the report, and the one WARN says so. A session keeps at most 1000 violations between two tests, so a route that breaks the rule on every message cannot fill memory. The test that fails still shows the first 1000. New test: a context passes the bound in one "test", and a violation in the next one is still charged. - **`StepCall` keeps the `Throwable`**: right, and it went against the design. It now keeps the class and message as a string, plus the stack it already copied. The only reference left is the thread-local of the last reported throw, which suppresses the duplicate report at the enclosing step. That one is a `WeakReference` and is cleared at the next step call on that thread, so it does not keep the exchange either. - **`Error` from a plain `Processor`**: agreed that the rule is broken by Camel, not by the processor. The kind stays `THREW_AFTER_CALLBACK`, because that is what the step's contract saw, and existing expectations keep working. The report now says the callback was called by Camel's adapter for a plain `Processor`, and its fix line points at whatever threw the `Error` instead of "do not throw after the callback". The adapter is recognised from the stack of the first callback. The first frame that is not a callback `done` is one of `DelegateSyncProcessor.process`, the bridge in `AsyncProcessorConverterHelper`, or `AbstractBeanProcessor.useCustomAdapter`. The bean one has the same pattern: it catches `AssertionError | Exception`, calls the callback in `finally`, and lets other Errors through. With `stackDepth=0` the adapter cannot be seen and the report is the plain one; the docs say so. In the new tests the exchange also stays inflight after the `Error`, so the context stop waits it s whole timeout. What the adapters should do instead (catch `Throwable` and set it on the exchange, or rethrow without calling the callback) is a Camel behaviour change, so I'd suggest a small follow-up JIRA. I can open it if you agree. - **Generated step id**: new test `aStepWithAGeneratedIdIsNamed`. The step has no `.id(...)`, and the report has the generated `processN` id, the same id the route definition holds, and the user's class. So `collect` does see the assigned id in `onRoutesAdd`. - **`bean(...)` naming**: done, it was cheap. `collect` handles `BeanDefinition` (bean instance, bean class, bean type, or a ref looked up in the registry), so the report names the bean's class instead of `BeanProcessor`. Test: a bean that is a plain `Processor` throwing an `Error`. - **`@Isolated` on `SanitizerActivationTest`**: done. - **Nits**: the DEBUG report is only built when DEBUG is on, and the shutdown summary no longer builds the full report first. The design header now has `last-updated: 2026-11-08` and `status: implementable`. - **Chaos together**: I left this as a follow-up (listed in the PR description). camel-chaos test scope also pulls in netty-http, resilience4j and camel-test-dst, which is more than this PR should drag into its build. `ManagedSanitizerTest` already runs next to another `StepHook` added with `addStepHook`. Tests: with the CI switch the whole camel-core suite ran 9042 tests, 0 failures, 1 error (46 skipped). The error was `FileConsumerAsyncCommitRaceTest`, where JUnit could not delete its `@TempDir` while files were still being moved. It reported no violation and passed 10/10 alone with the switch. All violations in `target/camel-sanitizer` are expected ones. The sanitizer tests in camel-core, camel-management, camel-main and camel-test-main-junit6 pass. The changed and new tests ran 20 times in one JVM without and with the switch, 680/680 each, and `ManagedSanitizerTest` 20/20. The two new record tests fail on the classes from before the review. The PR description is updated. On your two questions, my view in one line each: - JMH with the sanitizer on: not needed before merge, since the module is opt-in and test-scoped and changes no camel-core main code. Worth one run (with and without stacks) before 4.24 is released, to set the `stackDepth` default. - A quick `-D` switch run on netty-http and vertx-http: yes, worth doing now, it is cheap and R1 can already report there. I'm happy to run it locally after merge or as part of the CI job follow-up, whichever you prefer. _Claude Code on behalf of allthingssecurity_ -- 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]
