allthingssecurity opened a new pull request, #28756: URL: https://github.com/apache/camel/pull/28756
# Description [CAMEL-26333](https://issues.apache.org/jira/browse/CAMEL-26333) When a `doTry` calls a route whose error handler handles a failure (a dead letter channel, or `onException(...).handled(true)`), the `doFinally` block runs only its first step and silently skips the rest. ```java errorHandler(deadLetterChannel("mock:dead")); from("direct:a") .doTry() .to("direct:b") .doFinally() .to("mock:f1").to("mock:f2").setHeader("f3", constant("yes")).to("mock:f4") .end() .to("mock:end"); from("direct:b").setBody(constant("in-b")).throwException(new IllegalArgumentException("x")); ``` When `direct:b` fails, `mock:f1` gets the message, but `mock:f2` and `mock:f4` do not, and the header `f3` is not set. When it does not fail, all of them run. The `doFinally` EIP is described (model and catalog) as steps "to always execute after a doTry block completes, regardless of whether an exception was thrown or caught". Main, 4.23.0, 4.22.x and 4.18.x all behave this way (the same `FinallyProcessor` and `PipelineHelper` code). Cause: the called route's error handler marks the exchange as handled by the error handler (`errorHandlerHandled`) and as failure handled (`failureHandled`). `FinallyProcessor` clears the exception before it runs the block, but not those marks. The `doFinally` block is a `Pipeline`, which always runs its first step and then stops before each later step when the exchange is handled by the error handler (`PipelineHelper.continueProcessing`, "Handled by the error handler"). And an error handler does not handle a new failure of an exchange that is failure handled (`RedeliveryErrorHandler.isDone`), so a route called from such a `doFinally` could not handle or continue its own failure with its error handler. A second consequence is fixed by the same change. A recipient list is a pipeline of two processors: the first evaluates the recipients into the exchange property `CamelEvaluateExpressionResult`, and the second sends and removes it. As the first step of such a `doFinally`, the recipient list only evaluated, and the value stayed on the exchange. A routing slip or dynamic router that ran later on the same exchange took that value as its slip (`RoutingSlip.process`), so it sent to the recipient list's endpoints instead of its own. When the value was not a valid list for the routing slip's delimiter, the routing slip sent to one `direct:` endpoint without a consumer and blocked for the direct timeout (30 seconds). Changes: - `FinallyProcessor` clears both marks while the `doFinally` block runs, as `OnCompletionProcessor` does for an onCompletion, and sets them again when the block is done (the handled mark only if the block did not set it itself). Every step of the block runs, a route that the block calls handles or continues its own failure with its error handler as it does when the `doTry` did not fail (a handled failure there stops the rest of the block, as in any route), and the route still stops after the `doTry` as before. CAMEL-19441 decided that a caller does not continue routing after a called route handled the failure, and this stays true (the new tests check it: without setting the mark again, `mock:end` would get the message). - The recipient list in a `doFinally` now sends to its recipients and removes its property, so nothing stale is left for a routing slip. - No upgrade guide entry, as running all the steps of the `doFinally` is the fix. Routes that relied on the old behaviour now see the second and later steps of such a `doFinally` run (and a recipient list, routing slip or `to` there send their messages), once per exchange. A route called from such a `doFinally` that fails is now handled (or continued) by its own error handler; before, its error handler left the failure alone because the exchange was still marked as failure handled, and the exchange failed with that exception. Nothing else changes for them: the outcome of the `doTry` body, the dead letter queue and the stop after the `doTry` are the same. - Not changed: `RoutingSlip.process` still reads `CamelEvaluateExpressionResult`, because camel-bean's `@RoutingSlip` and `@DynamicRouter` annotations hand their result to the routing slip in that property (`MethodInfo`; `RoutingSlipPOJOTest` passes). A dedicated key for that hand-off (the "TODO: Maybe use a new constant than EVALUATE_EXPRESSION_RESULT" from CAMEL-13449) would stop a routing slip from reading another EIP's value at all. It touches camel-bean, so I would file it as a separate minor issue if you want it. Tests: - New `DoFinallyAfterHandledCalledRouteTest` (camel-core, 6 tests, every send bounded, no sleeps): a dead letter channel, the same with asynchronous delayed redelivery (the `doFinally` block then runs on another thread, which the test checks), and a handled `onException`. Each checks every `doFinally` step, the dead letter queue, that the `doCatch` does not run, and that the route does not continue after the `doTry` (CAMEL-19441). Two more tests call a failing route from the `doFinally`: its dead letter channel handles that failure too (which stops the rest of the block), and its `onException(...).continued(true)` continues it (the block then runs to the end). These 5 fail on main. A control where the called route does not fail passes on main as well. - New `RoutingSlipInDoFinallyAfterRecipientListTest` (camel-core, 5 tests): a routing slip after the recipient list with a dead letter channel and with a handled `onException`, a dynamic router, and a routing slip in a `doFinally` inside a failover after a redelivery, with a recipient list that uses `;` as the delimiter (on main it blocks, and the test's 10 second bound fails it). These 4 fail on main. A control where the called route does not fail passes on main as well. - I also ran the new tests with `FinallyProcessor` not setting the handled mark again after the block: the 3 bug tests of `DoFinallyAfterHandledCalledRouteTest` then fail on `mock:end`. With a `FinallyProcessor` that clears only the handled mark, the two tests with a failing route called from the `doFinally` fail (the exchange fails with that route's exception). - The new tests together with CAMEL-26327 (routing slip redelivery hang, `RoutingSlipNestedSlipRedeliveryTest`) and the fix for a `doCatch` after a continued failure in a called route (another branch of this batch) applied on the same tree: the routing slip, dynamic router, recipient list, doTry/doCatch/doFinally, error handler and onCompletion tests pass (674 tests). - camel-core: the doTry/doCatch/doFinally, onException, redelivery, dead letter channel, error handler, routing slip, recipient list, dynamic router and pipeline and onCompletion tests (`*Try*`, `*Catch*`, `*Finally*`, `*OnException*`, `*Redelivery*`, `*DeadLetter*`, `*ErrorHandler*`, `*Continue*`, `*RoutingSlip*`, `*RecipientList*`, `*DynamicRouter*`, `*Pipeline*`, `*OnCompletion*`): 984 tests, 0 failures, 10 skipped, including `RoutingSlipPOJOTest`. - The whole camel-core suite (one run, with the core modules it depends on, camel-core test log at INFO): the machine's resource guard stopped the run (load from another job) after 2951 of the about 3030 camel-core test classes (7982 tests). 12 failed, none of them a doTry, error handler or EIP test; all are known failures of my local offline build, also on main: `SimpleSyntaxHintsTest` (5), two OGNL hint tests of `SimpleTest`, `FileProducerChecksumOfWrittenFileTest` (3), `FilePollEnrichDynamicFileNameTest` and `ValidatorExternalResourceTest` (it downloads a schema). The core modules before camel-core passed, except `ModelWriterTest` (4) and `YamlModelWriterTest` (1) in camel-xml-io and camel-yaml-io, which fail because my offline build regenerates the model writers differently (the regenerated files are not part of the commit). How it was found: a Rocq specification of Camel's error handling, written from the documentation, was compared with real Camel on generated routes. The cases were then reduced and reproduced on HotSpot. Backport candidates, for the maintainers to label if wanted (contributors cannot set `port/*` labels): camel-4.22.x and 4.18.x have the same code (`FinallyProcessor`, `Pipeline`, `PipelineHelper`). Routes see the second and later `doFinally` steps run where they did not before, so I leave the backport to the reviewers. # 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-26333) filed for the change (usually before you start working on it): [CAMEL-26333](https://issues.apache.org/jira/browse/CAMEL-26333). # 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]
