Repository navigation
CAMEL-26327: camel-core - Routing Slip / Dynamic Router: a redelivered step no longer hangs after a nested routing slip - #28750
Merged
davsclaus merged 2 commits intoOct 31, 2026
Conversation
…d step no longer hangs after a nested routing slip The error handler of a routing slip or dynamic router step redelivers through RoutingSlipProcessor, which reads the step's producer from the exchange property CamelSlipProducer. A routing slip or dynamic router in the called route runs on the same exchange: it overwrote that property and removed it when its step completed. The redelivery then threw a NullPointerException inside the reactive executor, which logs and ignores it, so the callback was never called and the caller waited forever. - RoutingSlip.processExchange keeps the producer that was in the property and puts it back when the step completes; it removes the property only when there was none. - RoutingSlipProcessor fails the exchange with an IllegalStateException and calls the callback when the property is missing, instead of throwing. - New RoutingSlipNestedSlipRedeliveryTest (15 tests, every send bounded), also with an inner slip step that its own error handler redelivers before the outer step is redelivered. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gnodet-bot
reviewed
Oct 31, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Good fix for a real deadlock bug in nested RoutingSlip/DynamicRouter scenarios. The root cause is clear: when processExchange is called recursively (nested slips), the inner call overwrites SLIP_PRODUCER on the shared Exchange, leaving the outer call's producer reference as null on redelivery.
The save/restore pattern is correct — Exchange is single-threaded by design, so there is no concurrent access risk. The null guard (callback.done(true); return true) follows the standard Camel synchronous failure pattern. SLIP_ENDPOINT does not need saving because it is unconditionally set at the start of each step regardless.
One minor observation on test robustness — see inline comment.
Missing backport labels: this fix applies to active maintenance branches (camel-4.22.x, camel-4.18.x).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Contributor
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
Contributor
|
🧪 CI tested the following changed modules:
🔁 6 tests passed only after a retry on JDK 21 (6 retried attempts) Recovered flaky tests on JDK 21 (6)
🔬 Scalpel shadow comparison — Scalpel: 572 of 713 tested, 27 compile-only — current: 573 all testedMaveniverse Scalpel detected 572 affected modules (current approach: 573). Skip-tests mode would test 572 modules (2 direct + 570 downstream), skip tests for 27 (generated code, meta-modules) Modules only in current approach (1)
Modules Scalpel would test (572)
Modules with tests skipped (27)
Build reactor — dependencies compiled but only changed modules were tested (2 modules, 9.0s total)Total reactor time: 9.0s
Top 20 slowest modules:
|
davsclaus
approved these changes
Oct 31, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
Nice find, and the root cause is right: the first slip step runs on the incoming exchange itself, so a routing slip in a route called through direct overwrites CamelSlipProducer and then removes it, and the outer step's error handler (one RoutingSlipProcessor per slip, the CAMEL-12120 design) has nothing to redeliver to. Restoring the previous producer in the step callback fixes the cause rather than hiding the symptom; fixing only the redelivery path would turn the hang into an NPE and the outer step still could not be redelivered. The guard in RoutingSlipProcessor is a good extra safety net. I traced the async inner step and the copies made for later inner steps, and both carry the restored producer. The tests are bounded and deterministic, with useful controls.
A couple of points:
SLIP_ENDPOINT: since thedone()callback inRoutingSlip.processExchangeis being rewritten anyway, consider also settingExchangePropertyKey.SLIP_ENDPOINTback toendpoint.getEndpointUri()there. The routingSlip and dynamicRouter EIP pages document it as the current slip endpoint, and with a nested slip an outer dynamic router expression currently sees the inner slip's last endpoint after the step. It is one line on code this PR already changes.- Please file the follow-up JIRA for
RedeliveryErrorHandler.RedeliveryTask.redeliver()missing the try/catch thatrun()has (thereactiveExecutor.schedule(this::redeliver)paths), and link it from CAMEL-26327. It is the general reason a throwing async processor hangs on redelivery (CAMEL-26301 had the same symptom). - Nit, optional:
template.asyncSend(...).get(5, TimeUnit.SECONDS)would bound the send without a preemptive thread switch, butassertTimeoutPreemptivelyis fine as a safety net.
This looks like a good candidate for camel-4.18.x once merged.
Claude Code on behalf of Claus Ibsen. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
Contributor
Author
|
@davsclaus thanks for the review. On point 2: the follow-up JIRA for Claude Code on behalf of allthingssecurity |
… endpoint back to the step after a nested slip A routing slip or dynamic router in the called route changed CamelSlipEndpoint, so an outer dynamic router expression saw the inner slip's last endpoint after the step, or no endpoint at all after an empty inner slip (and took its first step again). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
@davsclaus thanks, point 1 is in f243730: the step callback in Claude Code on behalf of allthingssecurity |
gnodet-bot
reviewed
Oct 31, 2026
gnodet-bot
left a comment
gnodet-bot
left a comment
Contributor
There was a problem hiding this comment.
Following up on my earlier review — previous findings are addressed. The assertTimeoutPreemptively concern was a minor observation and the author's defense is valid (it's a safety net for a hang regression, and ContextTestSupport handles cleanup). The new code in f243730 is correct: the unconditional SLIP_ENDPOINT restore in the done() callback (responding to @davsclaus's point 1) fixes dynamic router expressions seeing a stale inner endpoint after a step that called a route with its own routing slip. The two new tests precisely validate this for both the non-empty and empty inner slip cases.
No new issues found. LGTM.
One standing point: backport labels are still missing. This deadlock fix applies to active maintenance branches (camel-4.22.x, camel-4.18.x).
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Contributor
Author
|
@davsclaus the follow-up is filed as CAMEL-26430 (RedeliveryTask.redeliver has no try/catch), linked to CAMEL-26327. Claude Code on behalf of allthingssecurity |
davsclaus
approved these changes
Oct 31, 2026
davsclaus
left a comment
davsclaus
left a comment
Contributor
There was a problem hiding this comment.
TL;DR: Approve. Since my last review the only change is the CamelSlipEndpoint restore in the step callback, with two tests that fail without it. No blockers.
Thanks for picking up point 1 and filing CAMEL-26430. The restore runs before the slip continues (sync and async), so the next dynamic router evaluation sees the step's endpoint, and without a nested slip nothing changes. The empty nested slip case (the router looping on its first step) is a good catch.
- The
assertTimeoutPreemptivelythread is answered; it can be resolved. - Optional: a one-line upgrade-guide note that
CamelSlipEndpointafter a step that ran a nested slip is now the step's endpoint (it matches the EIP docs, so this is a fix).
Claude Code on behalf of Claus Ibsen. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
davsclaus
added
port/camel-4.22.x
Contributor
🔄 Backport BotPort PRs created:
ℹ️ If you push additional commits after |
This was referenced Nov 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
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:
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
doTrywithout a matchingdoCatch, a route withnoErrorHandler(), or a rethrow. With a dead letter channel the message never reaches the dead letter endpoint. ArecipientListin the called route, or no redelivery, returns as expected.The cause:
RoutingSlip.processExchangestores the producer of the step in the exchange propertyCamelSlipProducer. The error handler that wraps the step callsRoutingSlipProcessor, 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).DynamicRouterextendsRoutingSlipand shares this code.RoutingSlipProcessor.processthrewNullPointerException: "producer" is null. The redelivery runs as reactive work:DefaultReactiveExecutorlogs a WARN that ends with "This exception is ignored", and the callback is never called.Changes:
RoutingSlip.processExchangekeeps 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.RoutingSlipProcessorfails the exchange with anIllegalStateExceptionand 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).CamelSlipProducer(git grep). The callback is still called exactly once on both the synchronous and the asynchronous path.CamelSlipEndpointback to the step's endpoint. The nested slip changed it too: an outer dynamic router whose expression reads it after such a step saw the inner slip's last endpoint (mock://inner), notdirect://sub. After an empty inner slip it saw no endpoint at all, because a slip removes the property when it starts, so a router written as "first step whenCamelSlipEndpointis null" took its first step again, and again. Without a nested slip the property already had that value, so nothing changes there.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 thatRedeliveryTask.run()has for the first attempt. So any asynchronous processor that throws fromprocess(exchange, callback)on a redelivery loses the callback (DefaultReactiveExecutorlogs 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:
RoutingSlipNestedSlipRedeliveryTest(camel-core, 17 tests, no sleeps). Every send is bounded withassertTimeoutPreemptively(5 s), so a lost callback fails the test instead of blocking the build. The tests cover:doTryat the top of the called route (default error handler, one redelivery) or withnoErrorHandler()and a dead letter channel (two redeliveries). The mock in the called route counts the attempts;onExceptionwith redelivery;threads());doTry/doFinally,onExceptionwith redelivery);IllegalStateException;CamelSlipProducerleft on the exchange after nested slips;CamelSlipEndpointon each evaluation, with a routing slip tomock:inneror an empty routing slip in the called route: it seesnone, thendirect://sub(bounded to five steps, so the empty case cannot loop forever).CamelSlipEndpointtests fail without the new line in the callback ([none, mock://inner], and[none, none, none, none, none, none]for the empty inner slip) and pass with it.*RoutingSlip*Test,*DynamicRouter*Test,*Redeliver*Test,*ErrorHandler*Test,*DeadLetter*Test): 388 tests, 0 failures, 2 skipped. After theCamelSlipEndpointchange:*RoutingSlip*Testand*DynamicRouter*Test, 91 tests, 0 failures, 2 skipped.TokenizerProcessorchanged in core): 8332 tests, 45 skipped. 12 of them failed:SimpleSyntaxHintsTest(5), two OGNL hint tests ofSimpleTest,FileProducerChecksumOfWrittenFileTest(3),FilePollEnrichDynamicFileNameTest, andValidatorExternalResourceTest, 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, theapiDocsdefault 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. Apart fromCamelSlipEndpointafter a step that ran a nested slip (above), the fix changes nothing that routes see, so it is a backport candidate.Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally 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
Co-authored-bytrailers) 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-Bytrailer.Claude Code on behalf of allthingssecurity
🤖 Generated with Claude Code