allthingssecurity opened a new pull request, #29525: URL: https://github.com/apache/camel/pull/29525
# Description [CAMEL-26741](https://issues.apache.org/jira/browse/CAMEL-26741) A plain `Processor` is called as an `AsyncProcessor` through an adapter: `DelegateSyncProcessor` for `process(...)`, the bridge of `AsyncProcessorConverterHelper` (for example a producer that is not an `AsyncProcessor`), and the Processor adapter of the bean component (`AbstractBeanProcessor.useCustomAdapter`, a bean that is a `Processor`). Each calls the callback in `finally`, but caught only exceptions (`DelegateSyncProcessor` also `LinkageError`, the bean adapter also `AssertionError`). Any other `Error`, such as an `AssertionError` in a test processor or a custom `Error`, was thrown on after the callback had been called: - the exchange had no exception, so the callback continued it and the next steps ran as if the processor had succeeded; - the error handler, `onException` and `doCatch` never saw the `Error`; - the `Error` left the reactive executor's loop, which catches only exceptions, so work already queued there was left behind: the exchange stayed inflight, the context stop waited its whole timeout, and the queued work ran later during another exchange's send. What main does, one context, routes with `.process(e -> { throw new Error("boom"); }).to("mock:result")` (sent in this order): | Route | Caller | `mock:result` / `mock:handled` | Inflight | |---|---|---|---| | plain | the `Error` is thrown to `template.send` | 0 / 0 | 1 | | with `onException(Error.class).handled(true).to("mock:handled")` | no exception on the exchange | 2 / 0 (this exchange, and the one left behind by the first send) | 0 | | `bean(...)` of a `Processor` that throws `new Error` | the `Error` is thrown | 2 / 0 | 1 | | inside `doTry ... doCatch(Error.class).to("mock:handled")` | the `Error` is thrown | 2 / 0 | 2 | Then `context.stop()` with a 5 s shutdown timeout took 5017 ms. With this change: each exchange fails with the `Error` (or is handled by `onException`/`doCatch`), `mock:result` gets nothing, inflight is 0 after each send, and the stop takes 1 ms. Change: the three adapters catch `Throwable` and set it on the exchange, as they already do for an exception. `Exchange.setException` wraps an `Error` in a `CamelExecutionException`, as it already does for the `LinkageError` that `DelegateSyncProcessor` catches (`DefaultErrorHandlerCatchThrowableTest` expects that for a `NoSuchMethodError` of a processor). Why catch `Throwable` here, given the history: - CAMEL-19748 (#11137) narrowed these catches from `Throwable` to `Exception` (keeping `LinkageError` in `DelegateSyncProcessor`), because catching JVM errors can be dangerous. In these adapters the `Error` was never passed on cleanly though: the callback had already continued the exchange, so it arrived after the fact, outside of any error handling. - CAMEL-16821 (2ed98ca, a direct commit) made the bean adapter fail the exchange on an `Error` such as an `AssertionError` (the narrowing then kept `AssertionError` there, c71e6a2), and CAMEL-19497 (#10512) moved its callback into `finally` so that "unhandled exceptions" do not block. - The engine already fails the exchange with any `Throwable` a step throws before its callback: `CamelInternalProcessor` and the error handlers catch `Throwable` there (CAMEL-26464). This change makes the adapters do the same for a processor that throws after Camel's adapter called the callback. The alternative, calling the callback only after a normal return or a caught exception and letting the `Error` through, would hand it to that engine code, which fails the exchange too. But it leaves callers outside the engine (and an `Error` thrown on by the caller of `process(exchange, callback)`) without a callback, and the adapter would still break the `AsyncProcessor` contract, now as "threw before calling the callback". Catching it in the adapter, as for an exception, keeps the plain `Processor` contract simple: what it throws fails the exchange. Processors of Camel that call the callback after a normal return, not in `finally` (such as `ConvertBodyProcessor`), let an `Error` through before the callback, and the engine code above already fails the exchange then. Four other processors catch only `Exception` and call the callback in `finally`, so an `Error` of user code they call has the same problem: `EvaluateExpressionProcessor` (an `Expression`), `DataTypeProcessor` and `DelegatingDataTypeProcessor` (a `Transformer`), and the synchronous branch of `MulticastProcessor.process`. They are left for a follow-up JIRA (I will file it), so this PR stays on the adapters of a plain `Processor` (the follow-up Claus asked for on #29409). The reactive executor's worker catches only `Exception` (`DefaultReactiveExecutor`), which is why the work queued behind such an `Error` is left behind; that is not changed here either. This changes behaviour for an `Error`, so the 4.24 upgrade guide has an entry. The main visible effect: the exchange keeps the `Error` wrapped in a `CamelExecutionException`, so `onException(Exception.class)`, `doCatch(Exception.class)` and a dead letter channel now also catch it (including an `OutOfMemoryError` or a `StackOverflowError`, as they already do for an `Error` thrown by a bean method or before the callback), and a test whose processor fails an assertion sees the exchange fail instead of getting the `AssertionError` thrown to it. To treat an `Error` differently, match it on its class, such as `onException(AssertionError.class)`. Relation to the exchange sanitizer (CAMEL-26714, #29409, merged): its rule R1 reported an `Error` of a plain `Processor` as "threw after the callback", with a special case that found Camel's adapter in the stack (`Sanitizer.Trace.SYNC_ADAPTERS`) and said that the adapter, not the processor, had called the callback. With this change the adapters no longer break the rule: they set the `Error` on the exchange and then call the callback, so the sanitizer sees a normal failed call and reports nothing. This PR therefore removes that special case from camel-sanitizer (the adapter names, the check of the stack, its own usual fix and the `record` overload that carried it), updates `SanitizerUserCodeTest` (the plain processor and the bean cases now expect the exchange to fail with the `Error` and no report; the bean case still checks that the sanitizer names the bean, not Camel's `BeanProcessor`, for that step (`Sanitizer.processorOf`), and the test of the adapter names is gone) and the par agraph of `sanitizer.adoc` and of its generated copy in camel-catalog (a plain `Processor` that throws breaks no rule). Tests: `ProcessorThrowsErrorTest` (camel-core), with a custom `Error`: - `.process(...)`: the exchange fails with the `Error` (wrapped), `mock:result` gets nothing, inflight is 0; - with `onException(ProcessingError.class).handled(true)`: `mock:handled` gets it, the caught exception is the `Error`, inflight 0; - in a `doTry` with `doCatch(ProcessingError.class)`: the `doCatch` gets it, `mock:result` gets nothing, inflight 0; - `bean(...)` of a `Processor`: same as the first; - `DelegateSyncProcessor` and the converted processor directly: `process` returns `true` without throwing, the callback is called once with `true`, and the exchange has the `Error`. On main all 6 fail (the `Error` thrown to the test 5 times, the `onException` one without an exception); with the fix 6/6. Sanitizer tests (`org.apache.camel.sanitizer` and `ProcessorThrowsErrorTest` in camel-core, and `ManagedSanitizerTest` in camel-management, run with `-Psanitizer`): 41 run, 0 failures; before the sanitizer change only the plain processor and bean cases of `SanitizerUserCodeTest` failed. Related tests (offline, `mvn test` of the core modules and the components they use, camel-core `org.apache.camel.processor`, `org.apache.camel.component.bean`, `org.apache.camel.sanitizer`, `*Error*Test`, `*Throwable*Test`, and `ManagedSanitizerTest`): 3681 run, 0 failures, 17 skipped (camel-core 3652). 20 runs in one JVM with the sanitizer on: `ProcessorThrowsErrorTest`, `SanitizerUserCodeTest` and `BeanThrowAssertionErrorTest` 380/380, `ManagedSanitizerTest` 20/20. Before the merge with the sanitizer, the full module tests of the core change were 10248 run, 0 failures, 52 ski pped. The formatter and import-sort plugins changed nothing. # Target - [x] I checked that the commit is targeting the correct branch (Camel 4 uses the `main` branch) # Tracking - [x] If this is a large change, bug fix, or code improvement, I checked there is a [JIRA issue](https://issues.apache.org/jira/browse/CAMEL) filed for the change (usually before you start working on it): [CAMEL-26741](https://issues.apache.org/jira/browse/CAMEL-26741). # Apache Camel coding standards and style - [x] I checked that each commit in the pull request has a meaningful subject line and body. - [ ] I have run `mvn clean install -DskipTests` locally from root folder and I have committed all auto-generated changes. (I built the core modules with the formatter and import-sort plugins and ran the tests listed above. The only generated file changed is the camel-catalog copy of `sanitizer.adoc`, kept identical to the component doc. I did not run the full root build.) # AI-assisted contributions - [x] If this PR includes AI-generated code, commits have proper co-authorship attribution (e.g., `Co-authored-by` trailers) and the PR description identifies the AI tool used. This PR was prepared with Claude Code (Claude Opus 5.5). The commit carries a `Co-Authored-By` trailer. _Claude Code on behalf of allthingssecurity_ 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- 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]
