Croway commented on PR #29450:
URL: https://github.com/apache/camel/pull/29450#issuecomment-6765673404
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 `CxfClientCallback`. It comes
from CXF 4.2.4, so this PR does not fix it.**
I stress-tested the async wrong-server path: a fresh endpoint per call, the
same `wrongServerConfigurer`, 16 threads, and the POJO, PAYLOAD and RAW data
formats. With CXF 4.2.4 a few calls never complete, on the PR head and on the
merge-base alike. I added instrumentation, and every stuck call looks the same:
```
HANG, done never called; outMessage Exception
content=java.util.concurrent.CompletionException: java.net.ConnectException
| inMessage=null | ClientCallback still on cxf exchange (never
dispatched)=true
```
CXF never calls `handleResponse` or `handleException` for those exchanges.
The `ClientCallback` is still on the CXF exchange. A counting `CxfBinding` saw
one `populateExchangeFromCxfResponse` call per completed exchange, none for the
stuck ones, and no throws at all. No change in `CxfClientCallback` can complete
them.
I used the same harness and the same PR head, changing only `-Dcxf-version`:
| CXF | async wrong-server calls (16 threads) | stuck (> 8 s) |
|---|---|---|
| 4.2.3 | 4800 | 0 |
| 4.2.4 | 4800 | 6 (POJO 3, PAYLOAD 1, RAW 2) |
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 `future.exceptionally(...)` in `HttpClientHTTPConduit` now
calls `outMessage.setContent(Exception.class, ex)`. When that callback runs
before the caller thread's `close()`,
`WrappedOutputStream.handleResponseOnWorkqueue` sees `exceptionSet == true` and
schedules nothing, and nothing calls the fault observer. The raw
`CompletionException` on the stuck exchanges matches that path: completed calls
get a `Fault` caused by `ConnectException`.
What this means for this PR:
- `testInvokingAWrongServerAsync` brings back the CAMEL-26600 flake, now in
three classes, because `CxfProducerOperationTest` and `CxfProducerContextTest`
inherit it. It passes with or without the fix.
- The PR removes the comment above `getWrongEndpointUri()` that explains
`synchronous=true` but keeps the workaround. Please keep the comment.
- I don't think CAMEL-26696 should be resolved by this PR. A plain-CXF
reproducer (a JAX-WS client calling `client.invoke(ClientCallback, ...)`, no
Camel) gives the same result: 22 stuck calls out of 4800 on 4.2.4, 0 on 4.2.3,
and 42 on today's 4.2.5-SNAPSHOT. So CXF main still has it. I reported it to
CXF as [CXF-9272](https://issues.apache.org/jira/browse/CXF-9272).
**2. A `RuntimeException` from the cookie store now completes the exchange
twice, both times as a success.**
This is point 1 of the description. I used an async endpoint against a live
`HelloService`, with a `cookieHandler` whose `storeCookies` throws
`IllegalStateException`:
```
merge-base: completedWithin10s=false doneCalls=0 (hangs)
PR head: doneCalls=2
done#1(sync=false) exception=null body=[hello]
done#2(sync=false) exception=null body=[hello]
final exception=null
```
In a `direct -> cxf -> mock` route, the caller gets `exception=null` and
`body=[hello]`: the request comes back as the reply, and the cookie failure is
neither logged nor set on the exchange. This happens in two steps:
- In `handleResponse`, `storeCookies` throws before
`populateExchangeFromCxfResponse` runs. The `finally` calls `done(false)` with
no exception on the exchange, and then the exception escapes.
- CXF `ClientImpl.onMessage` catches it and calls `handleException(resCtx,
ex)` on the same callback. Since `ClientImpl` always sets a `ConduitSelector`,
`ex` is overwritten with `outMessage.getContent(Exception.class)` and then with
the in-message lookup, and both are null. Nothing gets set, the cookie store
throws again, and `done(false)` runs a second time.
Suggestions:
- Catch `RuntimeException` around `storeCookies` the same way as
`IOException`, or set it on the exchange.
- In `handleException`, keep the original `ex` when both content lookups
return null.
- Make sure `done` runs only once, for example with an `AtomicBoolean`.
**3. The new unit test does not guard the behaviour it claims to.**
- I removed both `camelExchange.setException(ex)` lines from
`handleException`, and all 7 tests in `CxfClientCallbackTest` still pass. The
"preserves root cause" tests stub `camelExchange.getException()` to return the
transport exception. A real `DefaultExchange`, or
`verify(camelExchange).setException(transportEx)`, would catch this.
- `cxfExchange.get(ConduitSelector.class)` is never stubbed, so the tests
only cover the `else` branch. With `ClientImpl` the selector is always set.
- The cookie path from point 1 is not tested, because `getCookieHandler()`
always returns null.
**4. Minor**
- `cur.addSuppressed(t)` throws `IllegalArgumentException: Self-suppression
not permitted` when a custom `CxfBinding` rethrows the exception that is
already on the exchange. I reproduced this: `done` is called once, then the IAE
escapes instead of the transport exception. Adding `if (cur != t)` avoids it.
`DefaultCxfBinding` never does this, so it only affects custom bindings.
- `org.mockito.Mockito.doAnswer(...)` is fully qualified three times; the
project rules ask for imports.
- The module's tests use JUnit assertions (52 files, versus this one using
AssertJ), and the project rules ask new tests to match the module's style.
I can share the reproducer test class if it helps.
_Claude Code on behalf of Croway_
--
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]
