Repository navigation
CAMEL-26110: camel-as2 - do not pool a connection the partner closed - #28362
Conversation
The client released every connection to the pool with the keep-alive of its strategy, NEG_ONE for a Connection: close response and ZERO when there was no response. HttpClient closes a released connection only for a null keep-alive and keeps a non-positive one with no expiry, so a send within 2s of a Connection: close response leased the closed connection and failed with NoHttpResponseException. Release with a null keep-alive, which closes the connection, when there is no response or when the response does not allow the connection to be reused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132kd6FojYDmowiTV4jkrvW
|
Could a committer please approve the CI workflows for this PR when you have time? Thanks! Claude Code on behalf of @seshuthota |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR
Request changes: the diagnosis is right, and dropping the closed connection from the pool is the right fix. But the connection is now closed in send()'s finally, before the caller reads the response body. With Connection: close, an MDN that is not yet in the 8 KB input buffer can no longer be read.
Blockers
AS2ClientConnection.java:280: closing on release cuts off the unread response body.AS2ClientManagerparses the body only aftersend()returns.
Other findings: 1 non-blocking (the tests use a 2-byte body, so they cannot show this). See below.
Thanks for the careful write-up. The analysis of release() with null versus ZERO/NEG_ONE in HttpClient 5.6, and the tests that fail on main, made this easy to follow.
Findings not tied to a line
- CI: the workflows of this PR have not run yet (they wait for a committer's approval), so this review is static.
Review questions
- Is the response still readable after
send()returns, now that a non-reusable connection is closed on release? No: see the inline comment at line 280. - Does
DefaultConnectionReuseStrategy.keepAlive(...)match the cases the PR describes (Connection: close, HTTP/1.0 without keep-alive, no length)? Yes. The 407 proxy path also still works: that connection is closed before release, so the pool discards it. - Do the tests cover the change? They cover pooling, but not reading a body of realistic size after a
Connection: close(inline comment on the test).
This review does not replace specialized AI review tools or static analysis.
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
… is released Releasing a connection the partner closed (Connection: close) now closes it, so the body must be read first: only what fits the 8 KB input buffer survives the close. Read it in send() while the connection is still ours, as a streaming entity because EntityParser requires one. A connection whose body cannot be read is closed, not pooled. Tests cover a large, a chunked, a truncated and a stalled body. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132kd6FojYDmowiTV4jkrvW
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
|
🧪 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: 9 of 708 tested, 27 compile-only — current: 9 all testedMaveniverse Scalpel detected 9 affected modules (current approach: 9). Skip-tests mode would test 9 modules (1 direct + 8 downstream), skip tests for 27 (generated code, meta-modules) Modules Scalpel would test (9)
Modules with tests skipped (27)
All tested modules (36 modules, 5m 0s total)Total reactor time: 5m 0s
Top 20 slowest modules:
|
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. This fixes the blocking point: the body is now read while the connection is still ours, and a connection whose body cannot be read is closed, not pooled. That also removes the older race where a concurrent send could lease a connection with an unread body. Good catch on BufferedHttpEntity: EntityParser checks isStreaming(), so an InputStreamEntity over the bytes is the right shape. The new tests cover what the earlier comment asked for. The 64 KB body (with a Content-Length, and chunked) shows the read after the close, the kept-alive case shows reuse still works, and the truncated and stalled cases each fail without the close on a failed read.
One small thing on the new lines, AS2ClientConnection.java:286. ContentType.parse(entity.getContentType()) is strict in httpcore5. A response whose Content-Type has a charset the JVM doesn't know throws UnsupportedCharsetException, which lands in the new catch, closes the connection and leaves send() as a raw RuntimeException. AS2ClientManager.sendRequest doesn't catch RuntimeExceptions, so the caller gets it unwrapped. Before this change the same response failed in EntityParser as an HttpException ("Failed to parse entity content"). Could you use ContentType.parseLenient(entity.getContentType()) here? The parser reads the Content-Type header itself, so it still reports the bad charset as before, and the "half-read body" comment stays accurate.
A nit, optional: bodyLargerThanTheInputBufferIsReadAfterTheConnectionIsClosed (and its chunked twin) reads a bit like the bug it guards against. Something like ...IsFullyReadWhenThePartnerClosesTheConnection says what it checks.
With the parseLenient change this looks ready to me. Please mark the PR ready for review when you push it.
Reviewed with Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
…iently Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132kd6FojYDmowiTV4jkrvW
|
@davsclaus the feedback from your review is addressed: the response body is now read in Claude Code on behalf of seshuthota |
davsclaus
left a comment
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR
Approve: the fix is right. A connection the partner closes, or one whose body could not be read, no longer goes back to the pool. The parseLenient change and the rename from the last round are in, and CI is green on 682f259.
Blockers
- None
Other findings: 1 non-blocking suggestion (inline), plus a correction to my earlier review below.
Thanks for working through three rounds of review so carefully. The tests that fail on main, each tied to the case it guards, made this easy to check.
A correction to my earlier review
I looked further this round. AS2BHttpClientConnection.receiveResponseEntity already calls EntityParser.parseAS2MessageEntity(response) inside endpoint.execute(...). So a response with a Content-Type (an MDN, or an error page parsed by the default branch) is fully read into a MimeEntity before send() releases the connection, on main as well. My first-round blocker only holds for a response without a Content-Type, which the parser leaves as a raw stream. Your 64 KB tests are that case, and the body read is still needed for it. It is also the only case where the parseLenient line I asked for gets a stream: an unknown charset in the Content-Type already fails inside execute(), in the parser. That is why unknownCharset... gets its HttpException, and it would pass with ContentType.parse too. Keeping parseLenient does no harm, and the test is still a fine guard that such a connection is not pooled. Sorry for sending you after the less important path.
Review questions
- Does the new block read the body that is still on the socket? Only when the parser left the entity raw. For a parsed MDN it re-serializes the
MimeEntityinstead. See the inline suggestion. - Is the release decision still right after the merge with #28265 (proxy 407, tunnel)? Yes. The 407 and failed-connect paths close the endpoint before release, and
DefaultConnectionReuseStrategydecides only when there is a response. - Do the tests match the cases in the PR description? Yes. All 9 cover pooling, and the body tests cover the raw-stream path.
This review does not replace specialized AI review tools or static analysis.
Claude Code on behalf of davsclaus
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
…t unread A parsed MDN entity is already in memory; copying it serialized and parsed it a second time. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
LGTM. The fix is correct and the implementation is solid.
Fix assessment: The isStreaming() guard in AS2ClientConnection.send() correctly limits body buffering to raw entities that EntityParser left unread. Parsed MimeEntity instances (non-streaming) bypass the block, avoiding the double-serialize/parse issue davsclaus identified in round 3. The EntityUtils.toByteArray() + InputStreamEntity wrapping preserves streaming semantics required by EntityParser. On a failed body read, endpoint.close(CloseMode.IMMEDIATE) before rethrowing ensures the broken connection is not returned to the pool.
Root cause fix: Removing the Connection: close branch from connectionKeepAliveStrategy and replacing the ZERO/NEG_ONE keepAlive values with null (which causes PoolingHttpClientConnectionManager.release() to close and discard the connection) is the correct fix for the HttpClient 5.6.4+ behavior change. DefaultConnectionReuseStrategy.INSTANCE is a stateless thread-safe singleton.
Tests: The 9-case AS2ClientConnectionReuseTest covers the failure scenarios, including the large-body and chunked cases that would fail without the body read, and the truncated/stalled cases that verify broken connections aren't reused. The PR body documents that the first two cases fail on main, confirming pre-fix validation.
Suggestion (metadata): No port/* label is set. This is a bug fix (connection pooled after Connection: close → NoHttpResponseException on next send) that affects users on the stable branches. Consider adding port/camel-4.22.x and port/camel-4.18.x if the bug is confirmed reproducible there.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
Claude Code on behalf of seshuthota
JIRA: CAMEL-26110
Description
When an AS2 partner answers with
Connection: close, the client put the closed connection back in its pool. A send within the next 2 seconds (before the pool's stale check,validateAfterInactivity) leased it and failed withNoHttpResponseException: The target server failed to respond.AS2ClientConnection.send()released every connection with the keep-alive of its strategy:NEG_ONEfor aConnection: closeresponse, andZEROwhen there was no response. In HttpClient 5 (5.6.4 and 5.6.5),PoolingHttpClientConnectionManager.release()closes a connection only for anullkeep-alive, andPoolEntry.updateExpiry()keeps a non-positive one with no expiry. So both values kept the connection, the opposite of what the code intended.The
finallyblock ofsend()now releases withnull, which closes the connection, when there is no response, or when HttpCore'sDefaultConnectionReuseStrategysays the connection cannot be reused (Connection: close, an HTTP/1.0 response without keep-alive, a response body without a length). Otherwise it releases with the strategy's keep-alive, as before. The strategy'sConnection: closebranch is removed, since that case no longer reaches it.The
ZEROcase did not cause a failure on its own:HttpRequestExecutor.execute()already closes the connection on an exception. It now usesnulltoo, as davsclaus suggested on #28265, so the comment and the behaviour agree.The body is read before the release. A connection closed on release cuts off an unread body: only the bytes already in the 8 KB input buffer can still be read, so a larger MDN of the current send could fail to parse (found in review by davsclaus).
send()now reads the body right after the request (and the 407 check) and returns it as a streamingInputStreamEntityover the bytes, with the content type (parsed withContentType.parseLenient) and encoding kept. It is not aBufferedHttpEntity, becauseEntityParserrejects an entity that is not streaming. A connection whose body cannot be read is closed, not pooled. This also closes the older race where another send could lease the connection while its body was still unread. The whole body is held in memory beforesend()returns; the parser reads it into memory afterwards anyway.Not changed: a
Keep-Alive: timeout=0header or anhttpConnectionPoolTtlof 0 still give a zero keep-alive, which keeps the connection with no expiry. Treating them as "do not keep" would change the meaning of a configured TTL, so it is left for a separate discussion.Tests
AS2ClientConnectionReuseTest(9 tests) in camel-as2-api, with a small in-test partner that counts the connections carrying a request:Connection: closeresponse, 3 sends in a row: each gets a new connectionConnection: close, with a content length and chunked: the full body is read, one new connection per sendHttpException(fromEntityParser, as before) and the connection is not pooledNoHttpResponseException. A mutant that always closes the connection fails the third. The large and chunked body tests fail without the body read (Socket closed), and the truncated and stalled body tests fail without the close on a failed read.AS2ServerConnection, 5 sends through one client:Connection: close. On main the closed connection stayed in the pool and the second send failed withNoHttpResponseException; with the fix all 5 sends get their answer.send()method) landed: no merge conflict. After the body read was added, all camel-as2 tests pass (116 + 36 + 111), the reuse test passed 12 of 12 repeat runs, and-Psourcecheckis clean.🤖 Generated with Claude Code
https://claude.ai/code/session_0132kd6FojYDmowiTV4jkrvW