Repository navigation
[client-v2, jdbc-v2] Do not fail closing a partly read response that the server ended early - #3217
Open
polyglotAI-bot wants to merge 2 commits into
Open
polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
…the server ended early
When a query fails after the HTTP response started, the server ends the
chunked body without the final chunk. Closing a response that was not read
to the end makes HttpClient drain the rest of the body, and the drain threw
ConnectionClosedException ("Premature end of chunk coded message body"),
which ResultSet#close(), QueryResponse#close() and reader close passed on.
HttpAPIClientHelper now logs this exception at DEBUG instead of throwing it
when the caller opened the result stream. Read errors are unchanged, and a
response closed without reading (Client#execute, Statement#executeUpdate)
still reports the failure.
Fixes: #3203
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on all servers
On ClickHouse 25.8, a JSONEachRow query that fails mid-stream sends the
error as the last row ({"exception": ...}) and ends the chunked body
normally, because http_write_exception_in_output_format is 1 by default.
The body did not end early, so the test did not reproduce the bug, and
reading to the end did not throw. Set
http_write_exception_in_output_format = 0 so every server ends the body
early. Verified on 25.8 and 26.9: the test fails without the fix (both
formats, on close) and passes with it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
|
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
Contributor
|
@polyglotAI-bot |
Collaborator
Author
|
@chernser I investigated the Evidence
Root cause: stale reads across Cloud replicas
Proposed fix (separate PR) When
Local and Docker runs do not change because the setting applies only to Cloud. The Cloud CI job verifies the fix. This is not related to #3203, so I want to keep it out of this PR. Do you want me to open a separate PR with this change? If you prefer a different approach, for example a user-level setting on the Cloud test service, please tell me. |
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
2 participants
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
Fixes #3203.
When a query fails after the HTTP response started, the server writes the error into the body and ends the body without the final zero-length chunk. If the caller closes the response before it reads it to the end, Apache HttpClient reads and discards the rest of the body so that it can reuse the connection (
EofSensorInputStream.close->ResponseEntityProxy.streamClosed->ChunkedInputStream.close). That read gets to the missing final chunk and throwsConnectionClosedException: Premature end of chunk coded message body: closing chunk expected. The exception goes up throughAbstractBinaryFormatReader.close()/ the JSON parser close /QueryResponse.close(), andResultSetImpl.close()throws it as aSQLException. The caller stopped reading the result, so it cannot do anything about the exception.The fix is in the transport layer (
HttpAPIClientHelper), which is the only place that knows the HTTP client classes. The exception is detected by its class, not by its message. If the caller opened the result stream, aConnectionClosedExceptionfrom closing the body is logged atDEBUGwith the query id and is not thrown. HttpClient already discards the connection when the drain fails, so the pool does not reuse it.These cases do not change:
Client#execute/CommandResponse, JDBCexecuteUpdate). For these, the close-time failure is the only report that the query failed, so it is still thrown.Changes
client-v2HttpAPIClientHelper.TransportResponseImpl:createDataInputStream()wraps the body stream in a smallFilterInputStream(ResponseBodyInputStream). Itsclose()logs and ignoresConnectionClosedException.read()andread(byte[], int, int)call the wrapped stream directly. All readers use this stream (binary readers,JSONEachRowFormatReader,TableSchemaParser), and so do callers ofQueryResponse#getInputStream(). Because of this, the readers stay independent of the HTTP client.close()ignores the same exception only ifcreateDataInputStream()was called. This coversQueryResponse#close()/Records#close()when the caller closes the response but not the reader.history/latest/3203.md: changelog entry.Test
HttpTransportTests#testClosingNotFullyReadResponse(integration,@DataProviderfor uncompressed, ClickHouse LZ4, and HTTP-compressed responses). Each step uses a query that fails after the first rows are sent, on a client with one pooled connection:ClientException.client.execute(query): still fails, withConnectionClosedExceptionin the cause chain.SELECT 1succeeds, so the connection was released at each step.ResultSetImplTest#testCloseNotFullyReadResultSet(integration,@DataProviderforRowBinaryWithNamesAndTypesandJSONEachRow, one pooled connection):ResultSet#close()after one row does not throw. Reading to the end still throwsSQLException.executeUpdate(query)still throwsSQLException. The next query succeeds.ConnectionClosedException/SQLException: Premature end of chunk coded message body.createDataInputStream()guard inclose(), theexecute/executeUpdateassertions fail. So the guard is pinned.testChunkedResponsePrematureEndIsReported,testTailCorruptedStreamFailsDecoding,testClosingLZ4InputStreamReleasesConnectionandtestErrorWithSuccessfulResponsepass.HttpTransportTestsandQueryTests(client-v2 integration).ResultSetImplTest,StatementTest,ConnectionTestandPreparedStatementTest(jdbc-v2 integration).testServerErrorHandling(expects error 636, the server returns 400),testPasswordAuthenticationandtestAuthHeaderIsKeptFromUser: these fail the same way on unpatchedmain.Compatibility (
docs/changes_checklist.md)ConnectionClosedExceptionfrom closing a body whose stream the caller opened is no longer thrown. All other close failures, close failures of responses that were never read, and all read failures still propagate. The connection is discarded, not reused.DEBUGmessage per close of an incomplete body (not in a row loop), with the throwable attached and the query id. No SQL or data is logged.QueryResponse#getInputStream()still returns anInputStream; the new wrapper is private.readcall on the result stream.docs/features.md: no change. "Manages result-set lifecycle correctly" already describes the intended behavior.Limits (not changed by this PR)
ConnectionClosedExceptionis ignored, as suggested in the review of jdbc-v2: swallow chunked-stream drain errors on ResultSet.close (fixes #2361) #2857. If the connection drops inside a chunk, HttpClient throwsTruncatedChunkException(aMalformedChunkCodingException), and that still propagates from close. I could not produce it against a real server. It can be added to the same catch if wanted.Pre-PR validation gate
AGENTS.md/docs/ai-review.md/docs/changes_checklist.mdhistory/latest/3203.md)Client.query/Client.executeand JDBCStatement)Related
HttpAPIClientHelper, and a test inHttpTransportTests.TransportResponseImpl. That is a different case: this PR changes only what happens when a partly read response is closed. If both are merged, the conflict is small.🤖 Generated with Claude Code