allthingssecurity opened a new pull request, #28786: URL: https://github.com/apache/camel/pull/28786
# Description [CAMEL-26434](https://issues.apache.org/jira/browse/CAMEL-26434) From the side question in the review of #28756 (CAMEL-26333). When the `doTry` block calls a route (such as with `direct`) that fails *unhandled* (default error handler, no `onException`), a route called from the `doCatch` cannot handle its own failure with its error handler. Its dead letter channel or handled `onException` does not run, and the exchange fails with that route's exception. When the `doTry` fails in the same route, the same `doCatch` works: ```java from("direct:viaCallee") .doTry().to("direct:failing") .doCatch(IllegalArgumentException.class).to("mock:catch").to("direct:dlcRoute") .end() .to("mock:after"); from("direct:failing").throwException(new IllegalArgumentException("callee")); from("direct:dlcRoute").errorHandler(deadLetterChannel("mock:dead").redeliveryDelay(0)) .throwException(new IOException("in catch")); ``` On main, `direct:viaCallee` fails with `IOException: in catch` and `mock:dead` gets nothing. With `.throwException(new IllegalArgumentException(...))` in the `doTry` instead of `.to("direct:failing")`, `mock:dead` gets the message and the exchange does not fail. The same holds for a handled `onException` in the called route. With this PR the `doCatch` behaves as when the `doTry` failed in the same route. Two points differ from what the JIRA's proposal might suggest, on purpose: - when the route called from the `doCatch` *handles* its failure, the rest of the `doCatch` block and the steps after the `doTry` do not run, as for any failure that a called route handles (CAMEL-19441) and as in the in-route case. The caller continues after the `doTry` as before when the `doCatch` block does not call a failing route (or the called route continues its failure); - the "not handled" mark of the caught failure is not set again after the block (failure handled is). The `doCatch` handled that failure; setting "not handled" again made a `doTry`/`doCatch` inside a handled `onException` fail the exchange with the caught exception (see below). Cause: the callee's error handler marks the exchange as failure handled (`ExchangeExtension.isFailureHandled() == true`) and as not handled by the error handler (`errorHandlerHandled=false`) before the exception goes back to the caller. `CatchProcessor` clears the exception but not these marks. In the route called from the `doCatch`, `RedeliveryErrorHandler.isDone` treats a failure handled exchange as done, so the error handler gives the failure straight back (no redelivery, no dead letter channel, no `onException`), and `prepareExchangeAfterFailure` keeps a "not handled" that is already set. It is the same kind of cause as CAMEL-26333 (#28756) for the `doFinally` block, with the marks of an *unhandled* callee failure instead of a handled one. ## Changes - `CatchProcessor` clears both marks when the `doCatch` takes over the exception (only when one of them is set), as the JIRA proposes. When the block is done: - failure handled is set again, one way (never cleared), as #28756 does; - a handled mark is set again only when it was set and true, and the block left it unset: the same "set and true" snapshot as `FinallyProcessor` in #28756. When a route called from the block handled its failure, its `true` stays, so the rest of the block and the caller after the `doTry` stop, as for any failure that a called route handles (CAMEL-19441) and as when the `doTry` failed in the same route; - the "not handled" mark of the caught failure is not set again: the `doCatch` handled that failure. This is a difference from main, on purpose: with it set again, a `doTry`/`doCatch` inside an `onException(...).handled(true)`, whose `doTry` calls a failing route, made the exchange fail with the exception the `doCatch` caught (the outer error handler saw "not handled" and put `CamelExceptionCaught` back). The new test `doTryInHandledOnExceptionCallsRouteThatFails` covers it. - When neither mark is set when the `doCatch` takes over, nothing changes. This is the case when the `doTry` failed in the same route, unless an earlier `doCatch` of the route caught a callee failure: failure handled is set again after that one, so a later `doTry` in the same route had the same bug on main, and is fixed too (test `secondDoTryAfterCaughtCalleeFailure`). - What else changes for a `doCatch` after an unhandled callee failure, all in the direction of the in-route case: - the error handler of a route called from the block now runs in full, also when it does not handle the failure: it redelivers, runs a non-handled `onException` or a context-scoped one, logs the exhausted failure, and `CamelFailureRouteId` names that route (test `redeliveryOfRouteCalledFromDoCatchAfterUnhandledCallee`: 3 attempts with `maximumRedeliveries(2)`, 1 on main); - for a `doCatch` that only logs, the error registry now records the caught callee failure as handled (`CatchProcessor` fires the failure handled event before the marks are set again), and the JMX `failuresHandled` counters of the steps inside the block no longer count it; as for an in-route failure; - copies of the exchange made later in the route (`split`, `multicast`, `recipientList`, `enrich`, `wireTap`) take over the handled mark but not failure handled. They used to inherit "not handled", so a route called from them handled its failure with its dead letter channel but the exception came back; now it stays handled, as in the in-route case (test `splitAfterCaughtCalleeFailure`); - with `bridgeErrorHandler=true` on the consumer, a failure of such an exchange that is not handled (a `doCatch` that fails, or a later failure in the route) now goes to the route's error handler, as an exchange with the body `Error processing exchange`, instead of only being logged (`BridgeExceptionHandlerToErrorHandler` only logs when the mark is "not handled"); - after the block, the exchange keeps failure handled as before, so the idempotent consumer, `ResumableCompletion` and the route's `failuresHandled` counter see the same as before. - Not changed: CAMEL-2577 and CAMEL-7275 (no nested error handlers for the children of `doTry`). The routes called from the `doCatch` have error handlers of their own; the `doCatch` children are still not wrapped in one. - Upgrade guide (4.24), in the camel-core section: the error handler of the called route now runs in full, a handled failure there stops the rest of the block and the caller, a later `doTry`, a `doTry` in an `onCompletion` and the copies made by `split` and the like, the `onException` case, `bridgeErrorHandler`, the error registry and the JMX counters, and the way back: `noErrorHandler()` on the called route (which applies to every caller of that route). ## Merge order with #28756 Please merge this after #28756 (CAMEL-26333). With this PR alone, a `doCatch` whose called route handles its failure, followed by a `doFinally` with two steps, runs only the first `doFinally` step: the called route's handled mark now reaches the `doFinally`, which is the CAMEL-26333 bug (on main the exchange failed instead, and both steps ran). The same `doTry` failing in the same route has this bug on main today. With both PRs, both steps run. ## Tests New `DoCatchCalledRouteAfterUnhandledCalleeTest` (camel-core, 21 tests, JUnit assertions as the other doTry tests of the package, no sleeps). Most cases through a failing callee have a control where the `doTry` fails in the same route, with the same expected outcome: - the four cases of the JIRA: a dead letter channel and a handled `onException` in the route called from the `doCatch`, each with its in-route control. The handled failure stops the rest of the `doCatch` block and the caller, in both; - a continued `onException` in the route called from the `doCatch` (the rest of the block and the caller then run), with its control; - a route called from the `doCatch` whose error handler redelivers and does not handle: 3 attempts, the exchange fails with that route's exception, with its control; - a route called from the `doCatch` that fails asynchronously (a delay with `asyncDelayed`; the test checks it ran on the delayer's thread), with its control; - a `doTry`/`doCatch` inside a handled `onException`, whose `doTry` calls the failing route: the `onException` handles the failure, with its control; - a second `doTry` in the route after a `doCatch` caught the callee failure: the route called from the second `doCatch` handles its failure, with its control; - a `split` after a `doCatch` that caught the callee failure, calling the route with the dead letter channel: the failure stays handled and the route continues after the split, with its control; - an asynchronous callee: the callee fails on the delayer's thread, so the `doCatch` runs on another thread than the one that started the route (checked); - nested `doTry` (no in-route control): the inner `doCatch` calls the route with the dead letter channel (the outer `doCatch` does not run); and the inner `doCatch` fails, so the outer `doCatch` takes over and calls that route (the outer `doCatch` clears the marks again); - the caller continues after the `doTry` when the `doCatch` does not call a failing route, and a `doCatch` that throws fails the exchange with its exception: failure handled as before, no "not handled" mark (these two pass on main except for that mark). Without the fix, 13 of the 21 tests fail; the 8 in-route controls pass on main as well. Mutants of the fix, each failing at least one test: no restore after the block, clearing only one of the two marks, a restore that always sets the earlier handled mark back (over what the block set), restoring failure handled only after a handled failure, setting "not handled" back. The restore of a `true` handled mark is defensive (a failure that an error handler handled has no exception left to catch), so no test reaches it, and removing it fails no test. - New class 20 times in one JVM: 420 tests, 0 failures. - camel-core `org.apache.camel.processor.**` (with `errorhandler`, `onexception`, `exceptionpolicy` and the other subpackages) plus the doTry/doCatch/doFinally, redelivery, error handler and exception tests of `org.apache.camel.issues`: 1441 classes, 2951 tests, 0 failures, 24 skipped. Compared per class with the same run on main 0fd7d9abf9c2 (1440 classes, 2930 tests, 0 failures): identical except the new class. - Formatter and import-sort: no changes. ## Follow-up I will file The failure handled mark of the unhandled callee failure still stays on the exchange after the `doTry` (as on main, and on purpose in this PR). Two consequences remain, reproduced on main and with this PR (scratch test, not in the PR): - a later failure in the calling route, after the `doTry`, is not handled by the calling route's `onException` (the exchange fails with it), while it is when the `doTry` failed in the same route (with `bridgeErrorHandler=true` the bridge now gives it to the error handler, see above). Likewise a route called later with a plain `.to("direct:x")` still cannot handle its own failure, while the same route called through `enrich` or a `split` now can, as the copy does not carry failure handled; - a route called from a `doFinally` after such a `doCatch` (or after the unhandled callee without a matching `doCatch`) cannot handle its own failure either. #28756 clears the marks around the `doFinally` only after a *handled* callee failure. Clearing failure handled for good when a `doCatch` catches the exception would fix both, but it changes what the readers of that mark see at the end of the exchange: the idempotent consumer (`IdempotentOnCompletion`) then confirms the message instead of removing it, `ResumableCompletion` records the offset, and the JMX and Micrometer `failuresHandled` counters no longer count it. That needs its own JIRA and upgrade guide entry, so I kept this PR to the `doCatch` block. Together with #28756 (and with CAMEL-26430 and CAMEL-26327 of this batch) on one tree: a `doCatch` whose called route handles its failure, followed by a two-step `doFinally`, runs both steps, as the same `doTry` failing in the same route does; the follow-up cases above are unchanged; the tests of the four branches and the doTry/doCatch/doFinally, routing slip, recipient list, dynamic router, redelivery, dead letter channel, error handler, onException and onCompletion tests of camel-core pass (1042 tests, 0 failures, 10 skipped). Once #28756 is merged I will add a test for the `doCatch` + two-step `doFinally` case here. # 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-26434) filed for the change (usually before you start working on it): [CAMEL-26434](https://issues.apache.org/jira/browse/CAMEL-26434). # 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 and ran the tests above, including the formatter and import-sort plugins. No generated file changes. 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]
