allthingssecurity opened a new pull request, #28750: URL: https://github.com/apache/camel/pull/28750
# Description [CAMEL-26327](https://issues.apache.org/jira/browse/CAMEL-26327) A routing slip or dynamic router step that the error handler redelivers never completes when the route it called ran a routing slip or dynamic router on the same exchange and then failed. The caller waits forever and the exchange stays in flight: ```java errorHandler(defaultErrorHandler().maximumRedeliveries(1).redeliveryDelay(0)); from("direct:start").routingSlip(constant("direct:sub")); from("direct:sub") .doTry() .routingSlip(constant("mock:inner")) .throwException(new IllegalStateException("boom")) .endDoTry() .doFinally() .end(); template.send("direct:start", e -> e.getMessage().setBody("in")); // never returns ``` Three things are needed: a routing slip or dynamic router step with redelivery, a called route (direct passes the same exchange) that runs a routing slip or dynamic router, and an exception that leaves the called route. It can leave through a `doTry` without a matching `doCatch`, a route with `noErrorHandler()`, or a rethrow. With a dead letter channel the message never reaches the dead letter endpoint. A `recipientList` in the called route, or no redelivery, returns as expected. The cause: - `RoutingSlip.processExchange` stores the producer of the step in the exchange property `CamelSlipProducer`. The error handler that wraps the step calls `RoutingSlipProcessor`, which reads the producer back from that property on every attempt (the design of CAMEL-12120, which replaced the cache of error handlers of CAMEL-8393 / CAMEL-10048). `DynamicRouter` extends `RoutingSlip` and shares this code. - When a step completes, its callback removed the property. A routing slip in the called route runs on the same exchange, so it overwrote the outer step's producer and then removed it. - The outer step's error handler then redelivers, and `RoutingSlipProcessor.process` threw `NullPointerException: "producer" is null`. The redelivery runs as reactive work: `DefaultReactiveExecutor` logs a WARN that ends with "This exception is ignored", and the callback is never called. Changes: - `RoutingSlip.processExchange` keeps the producer that was in the property before it sets its own, and the step's callback puts it back. It removes the property only when there was none, so nothing is left on the exchange after the outermost slip, as CAMEL-12120 intended when it added the property and its cleanup. - `RoutingSlipProcessor` fails the exchange with an `IllegalStateException` and calls the callback when the property is missing, instead of throwing. A missing producer can no longer turn into a hang. The error handler handles that exception like any other (redelivery, then the dead letter channel or the caller). - No other production code sets or reads `CamelSlipProducer` (`git grep`). The callback is still called exactly once on both the synchronous and the asynchronous path. Not changed: the nested slip also overwrites the `CamelSlipEndpoint` property of the outer slip. An outer dynamic router whose expression reads it after such a step sees the inner slip's last endpoint (`mock://inner`), not `direct://sub`. It is not a hang and changing it is visible to routes, so I left it out of this PR. I can open a separate JIRA for it if you want. Also not changed: the reason an exception here becomes a hang is in the error handler. `RedeliveryErrorHandler.RedeliveryTask.redeliver()` calls the output without the try/catch that `RedeliveryTask.run()` has for the first attempt. So any asynchronous processor that throws from `process(exchange, callback)` on a redelivery loses the callback (`DefaultReactiveExecutor` logs and ignores the exception), while the same throw on the first attempt fails the exchange. That affects more than this EIP, so I would rather fix it under its own JIRA. Tests: - New `RoutingSlipNestedSlipRedeliveryTest` (camel-core, 15 tests, no sleeps). Every send is bounded with `assertTimeoutPreemptively` (5 s), so a lost callback fails the test instead of blocking the build. The tests cover: - a routing slip or dynamic router calling a route with a routing slip or dynamic router, with a `doTry` at the top of the called route (default error handler, one redelivery) or with `noErrorHandler()` and a dead letter channel (two redeliveries). The mock in the called route counts the attempts; - a handled `onException` with redelivery; - an inner step that completes asynchronously (`threads()`); - an inner slip step that the called route's error handler redelivers, before the outer step is redelivered (both producers in use on the same exchange); - the route the comparison generated (nested `doTry`/`doFinally`, `onException` with redelivery); - the property missing on redelivery: the exchange fails with the new `IllegalStateException`; - three controls that also pass on main: a recipient list in the called route, no redelivery, and no `CamelSlipProducer` left on the exchange after nested slips. - The 12 other tests time out on main ("the send did not complete") and pass with the change. - camel-core: the RoutingSlip, DynamicRouter, redelivery, error handler and dead letter tests (`*RoutingSlip*Test`, `*DynamicRouter*Test`, `*Redeliver*Test`, `*ErrorHandler*Test`, `*DeadLetter*Test`): 388 tests, 0 failures, 2 skipped. - The whole camel-core suite, run on the previous base ffab48ea514b with the first version of the test class (14 tests; since then only `TokenizerProcessor` changed in core): 8332 tests, 45 skipped. 12 of them failed: `SimpleSyntaxHintsTest` (5), two OGNL hint tests of `SimpleTest`, `FileProducerChecksumOfWrittenFileTest` (3), `FilePollEnrichDynamicFileNameTest`, and `ValidatorExternalResourceTest`, which downloads an XSD from GitHub and timed out in my offline build. They fail the same way without this change, and none of them uses a routing slip or dynamic router. The other core modules: 1117 tests; the 5 failures (`ModelWriterTest`, `YamlModelWriterTest`, the `apiDocs` default of a rest verb) come from generated writers that my offline build regenerated with an older plugin, and are not in this change. The formatter and import-sort plugins ran and changed nothing. How it was found: a Rocq specification of Camel's EIPs, written from the documentation, was compared with real Camel on generated routes. One generated route hung. It was minimised, and the lost callback was traced on HotSpot. Backport candidates, for the maintainers to label if wanted (contributors cannot set `port/*` labels): camel-4.22.x and camel-4.18.x have the same code, and the hang is reproduced on 4.22.1 and 4.23.0. The fix changes nothing else that routes see, so it is a backport candidate. # 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-26327) filed for the change (usually before you start working on it): [CAMEL-26327](https://issues.apache.org/jira/browse/CAMEL-26327). # 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 files change. 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]
