fix(http): keep response header count and line length unlimited after httpclient5 5.6.4 - #3081
Closed
polyglotAI-bot wants to merge 1 commit into
Closed
fix(http): keep response header count and line length unlimited after httpclient5 5.6.4#3081polyglotAI-bot wants to merge 1 commit into
polyglotAI-bot wants to merge 1 commit into
Conversation
httpcore5 5.4.x limits an incoming HTTP/1 message to 100 headers and to a
line length of 8192 bytes by default. A ClickHouse response of a long query
with send_progress_in_http_headers carries one X-ClickHouse-Progress header
per progress interval, so it goes above the header limit and the response
fails to parse ("Maximum header count exceeded").
Both HTTP transports now disable the two limits explicitly, which keeps the
behavior of the previous HTTP core version. The response parser factory keeps
its own configuration, so the configuration is also given to the factory.
Contributor
|
Not requested. Will be fixed when progress headers are supported. |
Client V2 CoverageCoverage Report
Class Coverage
|
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
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
Follow-up of #3079 (issue #3078), which upgraded
httpclient55.4.4->5.6.4.That upgrade also moves
httpcore5from5.3.4to5.4.3. In5.4.3the defaults ofHttp1Configchanged from unlimited (-1) tomaxHeaderCount = 100andmaxLineLength = 8192. A ClickHouse response of a query that runs withsend_progress_in_http_headerscarries oneX-ClickHouse-Progressheader per progress interval (default 100 ms), so a long query sends far more than 100 response headers. The client then fails with:This is a regression of the merged upgrade, reported by the review of #3079.
Two related points made the fix necessary in two steps:
Http1Configwith a buffer size only, so the new limits applied.DefaultBHttpClientConnectioncreates the parser withresponseParserFactory.create(), so the parser uses the configuration of the factory, not the one given to the connection factory.DefaultHttpResponseParserFactory.INSTANCEholdsHttp1Config.DEFAULT, so setting the limits on the connection configuration alone has no effect. The configuration is now also given to the parser factory.Changes
client-v2HttpAPIClientHelper: onecreateConnectionFactory(int)builds theHttp1ConfigwithmaxHeaderCount = -1andmaxLineLength = -1and passes it tonew DefaultHttpResponseParserFactory(...). It is used by the pooled connection manager and by the non-pooled one (enableConnectionPool(false)), which previously used the library defaults and also ignored the network buffer size.clickhouse-http-clientApacheHttpConnectionImpl.HttpConnectionManager: same configuration through a newnewConnectionFactory(int).-1disables both limits (the parser checksmaxHeaderCount > 0/maxLineLen > 0), which restores the behavior ofhttpcore55.3.4. A finite limit is not usable here, because the number of progress headers grows with the duration of the query.Test
Two integration tests, one per transport, run a query that produces 150 progress headers (
send_progress_in_http_headers = 1,http_headers_progress_interval_ms = 10,max_block_size = 1):client-v2HttpTransportTests.testResponseWithManyProgressHeaders, for a pooled and for a non-pooled connection manager.clickhouse-http-clientApacheHttpConnectionImplTest.testResponseWithManyProgressHeaders.Verified against a local ClickHouse server:
MessageConstraintException: Maximum header count exceeded.clickhouse-data,clickhouse-client,clickhouse-http-clientandclient-v2: green (2449 tests).HttpTransportTestsfull class: same 27 pre-existing environment failures as before the change, the new tests pass.Checklist
Related: #3079, #3078
Addresses the parse-failure part of #3080. The other part of #3080 - reading the progress headers and reporting the progress to the application - is not in this PR.