Repository navigation
CAMEL-26422: camel-resilience4j, camel-microprofile-fault-tolerance - a circuit breaker no longer ends the enclosing doTry - #28780
Conversation
… a circuit breaker no longer ends the enclosing doTry A circuit breaker runs its steps as if in a doTry: it sets the exchange property TryRouteBlock while they run. When it was done it removed the property, also when the circuit breaker itself was in the steps of a doTry, so a later routing slip, dynamic router, multicast or recipient list of that doTry used the error handler of the route (redelivery, dead letter channel) instead of the doCatch. Also, a fallback that recovered from a failure sets the property CamelExceptionHandled (as a doCatch does), and it stayed set, so no doCatch of the enclosing doTry caught a later failure in the same doTry: CatchProcessor skips its clause when that property is set. The failure was thrown from the doTry. Both circuit breakers now keep the values of TryRouteBlock and CamelExceptionHandled of the enclosing scope and set them again when they are done, as a nested doTry does (CAMEL-25114, which listed the circuit breakers as not changed), also when Resilience4j completes asynchronously. Co-Authored-By: GPT-6 <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 42 of 713 tested, 26 compile-only — current: 42 all testedMaveniverse Scalpel detected 42 affected modules (current approach: 42). Skip-tests mode would test 42 modules (3 direct + 40 downstream), skip tests for 26 (generated code, meta-modules) Modules Scalpel would test (42)
Modules with tests skipped (26)
Build reactor — dependencies compiled but only changed modules were tested (3 modules, 1m 33s total)Total reactor time: 1m 33s
Top 20 slowest modules:
|
… review: more tests, remove the property when the scope had none - A fallback that fails inside a doTry is now caught by the doCatch: a test in both test classes, and a sentence in the upgrade guide. - The doCatch control test puts a failing step after the circuit breaker inside the doCatch, so it fails when the restore of CamelExceptionHandled is replaced by a remove (the earlier version passed either way). - An asynchronous variant of the recovered fallback test (Resilience4j), which covers the restore in the asynchronous completion. - When the enclosing scope had no value, the property is removed instead of set to null, as TryProcessor does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR: The fix is correct, and I'd approve it after two small additions. Thanks @allthingssecurity: saving and restoring TryRouteBlock and CamelExceptionHandled the way TryProcessor does for a nested doTry is the right model. It also fixes one more case that is not tested or documented yet (finding 1), and the doCatch control test passes whether or not the restore is there (finding 2).
What I checked:
- The bug is real:
TryProcessorsetsTryRouteBlockonce for the try pipeline (TryProcessor.java:128), and the fallback'sCamelExceptionHandled=truemadeCatchProcessor.java:135skip every doCatch. CAMEL-25114 only deferred this, so nothing deliberate is reverted. - Resilience4j:
restoreScope(ResilienceProcessor.java:569-576) runs exactly once on the sync, async and setup-failure paths; MicroProfile FT is sync with one exit. Circuit breaker state, counters and the fallback are unchanged. - I ran scratch tests in camel-resilience4j: the PR's 6 tests pass, and the cases in findings 1-3 fail on main and pass with the PR.
- No shared code with #28779, #28785 or #28786.
Findings:
-
A failing fallback inside a doTry is now caught by the doCatch. This change has no test and no upgrade-guide entry. On main, the fallback sets
CamelExceptionHandled=truebefore it runs (ResilienceProcessor.java:1082,FaultToleranceProcessor.java:623). When the fallback then fails, the doCatch is skipped and the exception leaves the doTry. With this PR, the restore clears that flag, so the doCatch catches the failure. That is the right behaviour, and it matches a circuit breaker without a fallback. I confirmed it with this route (it fails on main and passes here):from("direct:failingFallback").doTry() .circuitBreaker().throwException(new IllegalArgumentException("Recover me")) .onFallback().throwException(new IllegalStateException("fallback fails")).end() .endDoTry() .doCatch(IllegalStateException.class).to("mock:caught").end();
Suggestion: add this case to both test classes, and one sentence to
camel-4x-upgrade-guide-4_24.adoc:343-351, for example: "A failure of the fallback itself is now also caught by thedoCatch(before, it was thrown from thedoTry)." -
circuitBreakerInDoCatchKeepsTheFailureHandledpasses even without the restore. SeeResilienceOuterTryScopeTest.java:57and:114, andFaultToleranceOuterTryScopeTest.java:52and:104. After the first doCatch the exchange has no exception, so the second doCatch skips it anyway:catches(exchange, null)returns null. I changed the restore atResilienceProcessor.java:574toremoveProperty(...), and all 6 tests still passed. A version that does fail with that change puts a failing step after the circuit breaker, inside the doCatch:from("direct:inCatch").doTry() .throwException(new IllegalStateException("First failure")) .doCatch(IllegalStateException.class).to("mock:first") .circuitBreaker().to("mock:inside").end() .throwException(new IllegalArgumentException("in catch")).endDoTry() .doCatch(Exception.class).to("mock:second").end(); // mock:second expects 0, and out.getException() is the IllegalArgumentException
-
(minor) The asynchronous test only covers the success path.
direct:async(ResilienceOuterTryScopeTest.java:99) never runs the fallback, but the async fallback path is whereCamelExceptionHandledis restored fromwhenComplete. An async variant ofdirect:afterFallback(resilience4jConfiguration().asynchronous(true).end()) fails on main and passes with the PR, so it would cover that path. -
(minor, optional) When the enclosing scope had no value, both processors call
setProperty(key, null)(ResilienceProcessor.java:573-574,FaultToleranceProcessor.java:347-348), whereTryProcessor.java:141-145removes the property. It works the same, becausegetInternalProperties()skips null values. MatchingTryProcessorwould make it easier for a reader to compare the two.
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
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR: Approve. Thanks @allthingssecurity, 4048257 addresses all four points: the failing-fallback case is tested in both modules and has its upgrade-guide sentence, circuitBreakerInDoCatchKeepsTheFailureHandled now throws after the circuit breaker inside the doCatch so it fails without the restore, the asynchronous fallback path is covered, and restoreScopeProperty removes the property when the enclosing scope had none, as TryProcessor does. CI is green.
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.
|
Thanks for the review. All four points are in 4048257.
From your review of #28779, finding 2: the routing slip test for a route called from Tests: both classes 20 times in one JVM (280 tests); camel-resilience4j (93) and camel-microprofile-fault-tolerance (43), all tests, 0 failures. The description is updated. Claude Code on behalf of allthingssecurity |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. The fix correctly models TryProcessor's scope-save/restore semantics for both circuit breaker implementations.
Verified:
restoreScopePropertycallsremoveProperty(key)when the enclosing scope had no value (null) andsetProperty(key, value)otherwise — identical behavior toTryProcessorforTRY_ROUTE_BLOCK; and functionally equivalent toTryProcessor's unconditionalsetProperty(key, null)forEXCEPTION_HANDLED, since all consumers (CatchProcessor:135,CatchDefinition:125) testgetProperty() != null, which returns false in both cases.ResilienceProcessor: therestoreScopewrapper callback is passed to bothprocessAsyncandprocessSync, so all exit paths (sync success, asyncwhenComplete, setup-failure fast-path) restore the properties exactly once. No staleremoveProperty(TRY_ROUTE_BLOCK)calls remain.FaultToleranceProcessor: sync-only; restore runs at line 347-348, beforecallback.done(true), covering all exit paths within the try/finally block.- Async lambda captures are effectively final; the exchange handoff model guarantees single-thread access at each point, so no race.
davsclaus's four findings (failing-fallback test, doCatch control test, async fallback path test,setProperty(null)vsremoveProperty) are all already addressed or non-issues in the current version of the PR.- Pre-fix test validation: 10 of the 14 tests fail on
mainper the PR description; the two control tests (outsideTryUsesGlobalErrorHandler,circuitBreakerInDoCatchKeepsTheFailureHandled) pass onmain. Static trace confirms each failing test exercises the exact properties restored by this fix.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Description
CAMEL-26422
A circuit breaker (camel-resilience4j and camel-microprofile-fault-tolerance) inside a
doTryends thatdoTryfor the steps after it:doCatchof thedoTrycatches a later failure in the samedoTry, whatever the failing step is. The failure is thrown from thedoTry;doTryuses the route's error handler (redelivery and the dead letter channel) instead of thedoCatch.Cause: the circuit breaker runs its steps as if in a doTry, so it sets the exchange property
TryRouteBlockwhile they run, and it removed the property when it was done, also when it was itself in adoTry. CAMEL-25114 lists this under "Not changed". The fallback setsCamelExceptionHandled(as adoCatchdoes) and leaves it set;CatchProcessorskips its clause when that property is set. The PR of CAMEL-26332 (#28755) names this as a follow-up.Changes:
TryRouteBlockandCamelExceptionHandledof the enclosing scope when they start and set them again when they are done, asTryProcessordoes for a nested doTry; when the enclosing scope had no value, the property is removed. Resilience4j does it in the callback, so also when it completes asynchronously; the callback is still called once on every path (the pooled tasks do not use it).doTry,CamelExceptionHandledis no longer left set on the exchange. Only the doCatch (and its debugger hook) read it; it is not documented.doTryis now caught by thedoCatch, as a circuit breaker without a fallback is. Before, the fallback had setCamelExceptionHandledbefore it ran, so thedoCatchskipped its failure and it was thrown from thedoTry.doCatch, also a failure of the fallback itself; to have those steps use the route's error handler, move them out of thedoTry.Tests:
ResilienceOuterTryScopeTest(8 tests) andFaultToleranceOuterTryScopeTest(6 tests): a recipient list after a circuit breaker (also asynchronous for Resilience4j), a recipient list and a plain failing step after a recovered fallback (also asynchronous for Resilience4j), a fallback that fails, and two controls: a circuit breaker outside adoTry(the dead letter channel is used), and a circuit breaker inside adoCatchfollowed by a failing step in thatdoCatch(a laterdoCatchof the samedoTrydoes not catch it). 10 fail with the processors of main (6 + 4); the two controls pass there. ThedoCatchcontrol fails when the restore ofCamelExceptionHandledis replaced by a remove (both processors).org.apache.camel.processortests of camel-core (2791 tests) and the tests of these two modules pass (one local run of all the branches together).circuitBreaker()whose routing slip redelivers only the failing step (asked in the review of CAMEL-26421: camel-core - a route called from a doTry uses the error handler of its routing slip, multicast, recipient list and dynamic router #28779) is in CAMEL-26421: camel-core - a route called from a doTry uses the error handler of its routing slip, multicast, recipient list and dynamic router #28779 (ResilienceCalledRouteRoutingSlipTest), not here: it depends on the change of CAMEL-26421: camel-core - a route called from a doTry uses the error handler of its routing slip, multicast, recipient list and dynamic router #28779 and fails without it (the whole slip is redelivered, the first step gets 3 messages).Backport: no, failures now go to the
doCatchinstead of the route's error handler. 4.22 has the same behaviour.Target
mainbranch)Tracking
Apache Camel coding standards and style
mvn clean install -DskipTestslocally from root folder and I have committed all auto-generated changes.Claude Code on behalf of allthingssecurity
🤖 Generated with Claude Code