Repository navigation
CAMEL-26696: camel-cxf: async producer may never complete the exchange when the connection fails - #29450
CAMEL-26696: camel-cxf: async producer may never complete the exchange when the connection fails#29450gnodet wants to merge 7 commits into
Conversation
…e when the connection fails In CxfClientCallback.handleResponse() and handleException(), camelAsyncCallback.done(false) was called after populateExchangeFromCxfResponse() with no guard. If populateExchangeFromCxfResponse() threw a runtime exception, the Camel async callback was never signalled, causing the calling thread to hang indefinitely in DefaultAsyncProcessorAwaitManager.await(). Fix: wrap populateExchangeFromCxfResponse() in its own inner try/finally in both methods so that camelAsyncCallback.done(false) is always called, even if the response-binding throws. Add testInvokingAWrongServerAsync() to CxfProducerTest to cover the async (synchronous=false) connection-failure path that CAMEL-26600 had to work around with synchronous=true. Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
🔄 Backport BotThis bugfix targets
Add a Backport PRs will be created automatically on merge (once a port label is added). Comment ℹ️ If you push additional commits after |
|
🧪 CI tested the following changed modules:
🔁 1 test passed only after a retry on JDK 21 (1 retried attempt) Recovered flaky tests on JDK 21 (1)
🔬 Scalpel shadow comparison — Scalpel: 11 of 718 tested, 27 compile-only — current: 11 all testedMaveniverse Scalpel detected 11 affected modules (current approach: 11). Skip-tests mode would test 11 modules (1 direct + 10 downstream), skip tests for 27 (generated code, meta-modules) Modules Scalpel would test (11)
Modules with tests skipped (27)
All tested modules (38 modules, 6m 43s total)Total reactor time: 6m 43s
Top 20 slowest modules:
|
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
The fix is correct — wrapping populateExchangeFromCxfResponse in try/finally in both handleResponse and handleException guarantees camelAsyncCallback.done(false) is always signaled, even if the response-binding throws. This directly prevents the DefaultAsyncProcessorAwaitManager hang described in CAMEL-26696.
Pre-fix test validation: Static trace confirms the test correctly exercises the fix: without it, handleException exits without calling done(false) → DefaultAsyncProcessorAwaitManager.await() blocks indefinitely → class-level @Timeout(30s) triggers failure. With the fix, done(false) is always called → sendSimpleMessage returns with an IOException → assertions pass.
One process note: this is a correctness bug (exchanges silently hang forever under connection failures in async mode) that should reach LTS users. The camel-4.22.x maintenance branch is missing a backport label.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — one edge case was not addressed in my first pass.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
… in handleResponse
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. The try/finally pattern correctly guarantees camelAsyncCallback.done(false) is always signalled — even when populateExchangeFromCxfResponse throws — preventing the async exchange from hanging indefinitely in DefaultAsyncProcessorAwaitManager. Both handleResponse and handleException are fixed symmetrically. The new test covers the previously-broken async wrong-server path and the class-level @Timeout(30s) guards against regressions.
Backport note: this is a critical async hang fix — suggest adding a port/camel-4.22.x label.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for picking this up, Guillaume. Making the callback signalling robust is worthwhile, and having done(false) in a finally is good hardening. Some concerns before this can be called the fix for CAMEL-26696:
The test does not exercise the fix. In the wrong-server case populateExchangeFromCxfResponse returns early: there is no CXF in-message (DefaultCxfBinding.java:191-195). On a connection failure CXF 4.2 goes through HTTPConduit.handleResponseOnWorkqueue, then the fault observer, then ClientOutFaultObserver, then handleException, and that path never creates an in-message. So testInvokingAWrongServerAsync (CxfProducerTest.java:211-220) passes with or without the change, and its comment about the binding throwing does not apply. The CAMEL-26600 hang must come from somewhere else, possibly the CXF conduit never invoking the callback, so re-enabling the async wrong-server path may bring the 30s-timeout flake back, in three classes since two subclasses inherit the test. The hang was also seen with dataFormat=PAYLOAD, which the async test does not cover. Could we instead unit-test CxfClientCallback directly? Mockito is already a test dependency: use a binding that throws and a counting AsyncCallback, and assert done runs exactly once and the exchange has the exception, for both handleResponse and handleException.
handleException loses the root cause (CxfClientCallback.java:126-129). The exchange already holds the transport/fault exception from lines 105/108, and the new catch replaces it with the binding error. Suggest keeping the original:
} catch (Throwable t) {
Exception cur = camelExchange.getException();
if (cur != null) {
cur.addSuppressed(t);
} else {
camelExchange.setException(t);
}
}Catch Throwable, not Exception (lines 76 and 128). If an Error is thrown in handleResponse, done(false) runs with no exception set (the exchange continues as a success). The Error then reaches CXF ClientImpl.onMessage, which calls handleException on the same callback, and done(false) runs a second time. Core moved to catch (Throwable) in these spots in CAMEL-26741.
done is still skipped if the cookie store throws (lines 60-68 and 112-120 catch only IOException). The JIRA names storeCookies as a candidate. Making done(false) the last statement of an outer try/finally around both the cookie block and the binding would make the "always signal" comment true:
} finally {
try {
// cookies + populateExchangeFromCxfResponse, each failure recorded on the exchange
} finally {
if (!boi.getOperationInfo().isOneWay()) {
camelAsyncCallback.done(false);
}
}
}Minor: the PR description's "Fix" snippet predates the catch added in the second commit, and the root-cause section should say it is hardening rather than the confirmed cause. CAMEL-26696 should probably stay open, or a follow-up be filed, until the real hang is found. That matters for the port/camel-4.22.x label too.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…ter finally for done(false), unit test - Change catch(Exception) to catch(Throwable) in both handleResponse and handleException so that Errors also set the exception on the exchange and don't trigger a spurious second done(false) call via handleException. - Restructure try/finally so that camelAsyncCallback.done(false) is guarded by an outer finally block that wraps both the cookie-store and the binding calls. Previously done(false) was inside the inner binding try/finally, so a RuntimeException from storeCookies (beyond IOException) would skip it. - In handleException: when populateExchangeFromCxfResponse throws, preserve the original transport/fault exception already set on the exchange and add the binding failure as a suppressed throwable. Only set the binding exception directly when no prior exception exists. - Add CxfClientCallbackTest (Mockito unit tests) that directly verify the callback semantics: done(false) called exactly once, exception preserved or set correctly, Error handled, suppressed pattern correct. These tests cover scenarios that the integration test cannot (binding throws in various paths). - Update the testInvokingAWrongServerAsync comment to accurately reflect what the integration test does and does not verify.
|
Thanks for hardening this. Before writing this comment I ran reproducers against the PR head (f928913) and the merge-base (52680d1). 1. The CAMEL-26696 hang does not come from I stress-tested the async wrong-server path: a fresh endpoint per call, the same CXF never calls I used the same harness and the same PR head, changing only
Sequential runs (3300 calls) never got stuck locally, so it needs some load, as on CI. The likely cause is CXF-9250, which shipped in 4.2.4 (Camel bumped to it on 2026-10-09). Its What this means for this PR:
2. A This is point 1 of the description. I used an async endpoint against a live In a
Suggestions:
3. The new unit test does not guard the behaviour it claims to.
4. Minor
I can share the reproducer test class if it helps. Claude Code on behalf of Croway |
…ess/hang If CookieHandler.storeCookies() throws an unexpected RuntimeException (beyond the declared IOException), the previous code let the exception escape the inner catch block, preventing populateExchangeFromCxfResponse from running. done(false) was still called via the outer finally, but no exception was recorded on camelExchange, making the exchange appear as a success. Fix: catch Throwable from storeCookies in both handleResponse and handleException, record the error on the exchange (setException in the response path; addSuppressed on the root transport exception in the exception path), and always continue to run populateExchangeFromCxfResponse. Also remove the unused java.io.IOException import. Add CxfClientCallbackTest cases covering cookie-store failure in both paths.
|
Thanks @davsclaus, all points addressed in the latest commit (
The Hermes Agent (Claude Sonnet 4.6) on behalf of Guillaume Nodet |
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Reviewing CAMEL-26696 / camel-cxf async producer fix. The core fix (always calling done(false) via a finally block) is correct and well-motivated. A few observations on the implementation details.
A — handleResponse binding catch overwrites the cookie exception silently (medium)
In handleResponse, if the cookie store throws at line 74 (camelExchange.setException(e)) and then populateExchangeFromCxfResponse also throws at line 80, the binding catch at line 81 unconditionally overwrites the cookie exception with setException(t). The cookie error is silently lost.
Compare with handleException where the same scenario correctly uses addSuppressed to preserve both errors. The asymmetry is worth fixing for consistency and debuggability.
B — Self-suppression risk in handleException binding catch (low)
At line 146, cur.addSuppressed(t) has no guard against cur == t. If populateExchangeFromCxfResponse somehow rethrows the same exception object that is already on the exchange, Throwable.addSuppressed(this) throws IllegalArgumentException: Self-suppression not permitted. Defensive guard: if (cur != null && cur != t).
C — Nit: doAnswer used fully-qualified in test (nit)
doThrow, mock, and when have static imports (lines 37–39) but doAnswer is used fully-qualified (org.mockito.Mockito.doAnswer) at four call sites. Add a static import for consistency.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…sponse, guard against self-suppression, fix doAnswer static import - In handleResponse binding catch: use addSuppressed (with cur != t guard) instead of unconditionally calling setException, to preserve any exception already set by the cookie store (mirrors the pattern already used in handleException) - In handleException binding catch: add cur != t self-suppression guard to prevent IllegalArgumentException when the same Throwable object is re-thrown - Add static import for doAnswer in CxfClientCallbackTest (was used fully-qualified at 4 sites) - Add test: handleResponse_cookieAndBindingThrow_bindingErrorSuppressedOnCookieError covering the double-failure case in the success path
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
✅ Well-structured fix. The try/catch/finally refactor in both handleResponse and handleException correctly ensures camelAsyncCallback.done(false) always fires, preventing caller-thread hangs in DefaultAsyncProcessorAwaitManager.
Nit: In handleException's cookie-store catch block (~line 139), the guard before root.addSuppressed(e) is if (root != null) but is missing the root != e self-suppression guard that the adjacent binding catch block has. In practice root (the transport exception) and e (the cookie-store exception) originate from different call sites and cannot be the same object, so the risk is negligible — but for consistency with the defensive pattern used in the binding catch blocks, consider:
if (root != null && root != e) {
Otherwise LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
…Selector null-ex fallback, JUnit assertions, restore synchronous=true comment
|
Thanks @Croway — all points addressed in commit 1. CAMEL-26696 hang is in CXF 4.2.4 (CXF-9272), not in 2. 3. 4. Unit test gaps — addressed:
5. Missing 13 unit tests all pass locally. Hermes Agent (Claude Sonnet 4.6) on behalf of Guillaume Nodet |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks Guillaume. The callback is much more robust now. Both handlers catch Throwable, the root cause is kept via addSuppressed with the self-suppression guards (including the cookie path at CxfClientCallback.java:142), and done runs once from an outer finally. CxfClientCallbackTest now covers the binding, cookie and ConduitSelector branches in the module's JUnit style. The description now explains the hang through CXF-9272.
One concern in the latest commit (4c874bc):
The ConduitSelector fallback looks like it breaks async failover (CxfClientCallback.java:111-119). For FailoverTargetSelector, conduitSelector.complete() runs the retry itself. performFailover clears the out-message Exception content, calls exchange.clear(), and calls Retryable.invoke synchronously on the same CXF exchange. It puts the old fault back only if the retry throws. So "both lookups return null" means the failover succeeded, and that is why the old code skipped setException (see the comment at line 107). With setException(resolved != null ? resolved : ex), a successful failover keeps the original ConnectException on the exchange. FailOverFeatureTest uses an async producer (no synchronous=true), so it should catch this once CI runs. Since handleResponse no longer lets anything escape before done, the re-dispatch case this was meant for is gone. I'd revert to if (resolved != null), or fall back to ex only when cxfExchange.getInMessage() == null. Then adjust handleException_withConduitSelector_fallsBackToOriginalExWhenMessageContentIsNull (CxfClientCallbackTest.java:178).
Also:
testInvokingAWrongServerAsync(CxfProducerTest.java:211-220) passes with or without the change, and CXF-9272 makes it hang occasionally under load. CxfProducerOperationTest and CxfProducerContextTest inherit it, so it runs in three classes. Could we drop it, or@Disabledit with a reference to CXF-9272? The restored comment at line 253 also says the async error path is not covered here, right below a test that covers it.- Minor: when
signalDone()suppresses a second call, handleException has already changed an exchange Camel considers done. An early return whendoneFiredis already set would avoid that. - CAMEL-26696: since the hang is CXF-9272, I'd keep the JIRA open (or file a follow-up) and link CXF-9272. Then reconsider whether this hardening needs
port/camel-4.22.x.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — the 1 remaining open issue (missing root != e self-suppression guard in handleException's cookie-store catch) has been addressed. All previous findings resolved. LGTM.
The self-suppression guard at line 136 now correctly reads if (root != null && root != e), closing the residual IllegalArgumentException risk. The 13 unit tests cover the critical paths: normal/error for both handlers, cookie-store failures, binding failures, ConduitSelector resolution, Error propagation, and double-call suppression.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
- ConduitSelector fallback: when conduitSelector.complete() returns with no exception on either message, the failover succeeded — do not set the original transport exception on the Camel exchange (would break FailoverTargetSelector which re-dispatches on success) - Early-return in handleException when doneFired is already set, to avoid mutating an already-done exchange in the duplicate-call path - Disable testInvokingAWrongServerAsync: passes with or without the fix (CXF takes the handleException path on connection failure, never reaches populateExchangeFromCxfResponse), and CXF-9272 makes it hang under load - Update CxfClientCallbackTest: rename + flip assertion for the ConduitSelector no-exception case; add static import for never()
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all concerns from both @davsclaus and my previous review are addressed in the latest commit (d19bf54).
ConduitSelector failover fix ✅ — The conduitSelector branch now uses if (resolved != null) { camelExchange.setException(resolved); } without falling back to the original ex when both message lookups return null. This correctly identifies "both null = failover succeeded" and leaves the exchange clean, as @davsclaus requested. The companion test handleException_withConduitSelector_noExceptionOnMessages_doesNotSetException asserts never().setException(any()) for this case.
Early return in handleException when doneFired ✅ — Added at the top of handleException. When handleResponse has already fired and signalled done, a re-dispatch from ClientImpl.onMessage now returns immediately without mutating the already-committed exchange.
testInvokingAWrongServerAsync disabled ✅ — @Disabled("CXF-9272: may hang under load; callback invariants covered by CxfClientCallbackTest") with a restored comment referencing CAMEL-26696. The 13 unit tests in CxfClientCallbackTest provide direct coverage of the callback invariants across all error paths.
The hardening is solid: done(false) fires exactly once regardless of CXF calling both handlers, cookie-store failures, binding errors, Error propagation, and ConduitSelector failover paths. LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Summary
Hardens the camel-cxf async producer to ensure
camelAsyncCallback.done(false)is always signalled exactly once — even when the cookie store or response-binding throws — preventing the calling thread from hanging indefinitely inDefaultAsyncProcessorAwaitManager.Root Cause
This is a defensive hardening fix, not a confirmed root cause. The hang in CAMEL-26696 has been traced to CXF 4.2.4 by @Croway (see CXF-9272):
HttpClientHTTPConduitcan complete theCompletableFuturewith an exception before the caller thread'sclose(), causingWrappedOutputStream.handleResponseOnWorkqueueto skip scheduling the fault observer, so theClientCallbackis never dispatched. This PR cannot fix that CXF bug, but makes the callback-signalling inCxfClientCallbackunconditionally robust.In
CxfClientCallback.handleResponse()andhandleException(),camelAsyncCallback.done(false)was placed after the cookie-store block with only anIOExceptioncatch. An unexpectedRuntimeExceptionfromstoreCookiesor aThrowablefrompopulateExchangeFromCxfResponsecould leave the exchange uncompleted.Fix
AtomicBoolean done guard:
signalDone()ensuresdone(false)fires exactly once even if CXF calls bothhandleResponseandhandleExceptionon the same instance. WhendoneFiredis already set,handleExceptionreturns immediately to avoid mutating an already-done exchange.catch (Throwable) from cookie store (both paths): catches any Throwable from
storeCookies, records it on the exchange, and continues to the binding call.catch (Throwable) from
populateExchangeFromCxfResponse(both paths): catchesErrorin addition toException.Preserve root cause in
handleException: binding failures are added as suppressed throwables on the root transport/fault exception; only when no prior exception exists is the binding exception set directly. Guard against self-suppression (cur != t).ConduitSelector failover safety: when
conduitSelector.complete()returns with no exception on either message, the failover succeeded — the exchange is left clean (no exception set) so the re-dispatched request can proceed. Only a non-null resolved exception is propagated. Previously, both-null lookups fell back to the original transport exception, breakingFailoverTargetSelectorfailover paths.done(false)in outer finally: wraps both the cookie-store and binding calls viasignalDone().Test
CxfClientCallbackTest(Mockito unit test, 13 tests): normal path, binding throwsException/Error, suppressed-exception preservation, cookie-store failure in both paths,ConduitSelectorbranch with both resolved and null-message outcomes, and double-dispatch guard.CxfProducerTest.testInvokingAWrongServerAsync: disabled with@Disabled("CXF-9272: ..."). The test passes with or without the fix (CXF takes thehandleExceptionpath on connection failure, never reachingpopulateExchangeFromCxfResponse), and CXF-9272 makes it hang occasionally under load. The Mockito unit tests cover the callback invariants directly.Notes
The CXF-level hang (CXF-9272) requires a CXF fix. CAMEL-26696 remains open pending a CXF 4.x release with the fix.
Related
Hermes Agent (Claude Sonnet 4.6) on behalf of Guillaume Nodet