diff --git a/CHANGELOG.md b/CHANGELOG.md index 90cfbda77..aeb167a57 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -306,6 +306,9 @@ - **[repo]** Upgraded `org.apache.httpcomponents.client5:httpclient5` from `5.4.4` to `5.6.4` in `client-v2` and `clickhouse-http-client` to pick up the fixes of the newer 5.x releases, including known vulnerabilities. + The new HTTP core version limits an incoming message to 100 headers and to a line length of 8192 bytes by default, + which a response of a long query with `send_progress_in_http_headers` exceeds, so both HTTP transports now disable + these two limits explicitly and keep the behavior of the previous version. (https://github.com/ClickHouse/clickhouse-java/issues/3078) ### Docs & Examples diff --git a/clickhouse-http-client/src/main/java/com/clickhouse/client/http/ApacheHttpConnectionImpl.java b/clickhouse-http-client/src/main/java/com/clickhouse/client/http/ApacheHttpConnectionImpl.java index 0c00f7015..8e85d00fd 100644 --- a/clickhouse-http-client/src/main/java/com/clickhouse/client/http/ApacheHttpConnectionImpl.java +++ b/clickhouse-http-client/src/main/java/com/clickhouse/client/http/ApacheHttpConnectionImpl.java @@ -465,14 +465,25 @@ static class HttpConnectionManager extends PoolingHttpClientConnectionManager { } } + private static ManagedHttpClientConnectionFactory newConnectionFactory(int networkBufferSize) { + // Server may send an unbounded number of X-ClickHouse-Progress headers (one per + // progress interval) and headers with long values, so response header count and + // line length must stay unlimited (negative value disables the limit). + Http1Config http1Config = Http1Config.custom() + .setBufferSize(Math.min(1_000_000, networkBufferSize)) + .setMaxHeaderCount(-1) + .setMaxLineLength(-1) + .build(); + // Parser factory keeps its own config, so the same config is passed to it explicitly. + return new ManagedHttpClientConnectionFactory(http1Config, CharCodingConfig.DEFAULT, + new DefaultHttpResponseParserFactory(http1Config)); + } + public HttpConnectionManager(Registry socketFactory, ClickHouseConfig config, PoolConcurrencyPolicy poolConcurrentcyPolicy, PoolReusePolicy poolReusePolicy, TimeValue ttl, int networkBufferSize) { super(socketFactory, poolConcurrentcyPolicy, poolReusePolicy, ttl, - new ManagedHttpClientConnectionFactory(Http1Config.custom() - .setBufferSize(Math.min(1_000_000, networkBufferSize)).build(), - CharCodingConfig.DEFAULT, - DefaultHttpResponseParserFactory.INSTANCE)); + newConnectionFactory(networkBufferSize)); ConnectionConfig connConfig = ConnectionConfig.custom() .setConnectTimeout(Timeout.of(config.getConnectionTimeout(), TimeUnit.MILLISECONDS)) .setValidateAfterInactivity(config.getLongOption(ClickHouseHttpOption.AHC_VALIDATE_AFTER_INACTIVITY), TimeUnit.MILLISECONDS) diff --git a/clickhouse-http-client/src/test/java/com/clickhouse/client/http/ApacheHttpConnectionImplTest.java b/clickhouse-http-client/src/test/java/com/clickhouse/client/http/ApacheHttpConnectionImplTest.java index 9774c745d..d5837fe09 100644 --- a/clickhouse-http-client/src/test/java/com/clickhouse/client/http/ApacheHttpConnectionImplTest.java +++ b/clickhouse-http-client/src/test/java/com/clickhouse/client/http/ApacheHttpConnectionImplTest.java @@ -148,6 +148,29 @@ public void testConnection() throws Exception { } } + @Test(groups = { "integration" }) + public void testResponseWithManyProgressHeaders() throws Exception { + ClickHouseNode server = getServer(ClickHouseProtocol.HTTP); + + try (ClickHouseClient client = ClickHouseClient.newInstance()) { + // The server sends one X-ClickHouse-Progress header per progress interval, so a + // slow query produces many more response headers than the HTTP/1 parser default. + ClickHouseRequest req = newRequest(client, server) + .set("send_progress_in_http_headers", 1) + .set("http_headers_progress_interval_ms", 10) + .set("max_block_size", 1) + .set("wait_end_of_query", 1); + try (ClickHouseResponse resp = req.query("select number, sleep(0.05) from numbers(150)") + .executeAndWait()) { + int count = 0; + for (Object ignored : resp.records()) { + count++; + } + Assert.assertEquals(count, 150); + } + } + } + @Test(groups = { "integration" }) @Ignore("Need to disable the option to provide custom socket factory. note: need to remove it") public void testCustomOptions() throws Exception { diff --git a/client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java b/client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java index 54d53eec9..d4f3e2698 100644 --- a/client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java +++ b/client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java @@ -233,14 +233,29 @@ private ConnectionConfig createConnectionConfig(Map configuratio return connConfig.build(); } + private static MeteredManagedHttpClientConnectionFactory createConnectionFactory(int networkBufferSize) { + // Server may send an unbounded number of X-ClickHouse-Progress headers (one per + // progress interval) and headers with long values, so response header count and + // line length must stay unlimited (negative value disables the limit). + Http1Config http1Config = Http1Config.custom() + .setBufferSize(networkBufferSize) + .setMaxHeaderCount(-1) + .setMaxLineLength(-1) + .build(); + // Parser factory keeps its own config, so the same config is passed to it explicitly. + return new MeteredManagedHttpClientConnectionFactory(http1Config, CharCodingConfig.DEFAULT, + new DefaultHttpResponseParserFactory(http1Config)); + } + private HttpClientConnectionManager basicConnectionManager(LayeredConnectionSocketFactory sslConnectionSocketFactory, SocketConfig socketConfig, Map configuration) { Lookup tlsSocketStrategyLookup = RegistryBuilder.create() .register(URIScheme.HTTPS.id, (socket, target, port, attachment, context) -> (SSLSocket) sslConnectionSocketFactory.createLayeredSocket(socket, target, port, context)) .build(); + int networkBufferSize = ClientConfigProperties.CLIENT_NETWORK_BUFFER_SIZE.getOrDefault(configuration); BasicHttpClientConnectionManager connManager = BasicHttpClientConnectionManager.create( - null, null, tlsSocketStrategyLookup, null); + null, null, tlsSocketStrategyLookup, createConnectionFactory(networkBufferSize)); connManager.setConnectionConfig(createConnectionConfig(configuration)); connManager.setSocketConfig(socketConfig); @@ -269,12 +284,7 @@ private HttpClientConnectionManager poolConnectionManager(LayeredConnectionSocke ClientConfigProperties.HTTP_MAX_OPEN_CONNECTIONS.applyIfSet(configuration, connMgrBuilder::setMaxConnPerRoute); int networkBufferSize = ClientConfigProperties.CLIENT_NETWORK_BUFFER_SIZE.getOrDefault(configuration); - MeteredManagedHttpClientConnectionFactory connectionFactory = new MeteredManagedHttpClientConnectionFactory( - Http1Config.custom() - .setBufferSize(networkBufferSize) - .build(), - CharCodingConfig.DEFAULT, - DefaultHttpResponseParserFactory.INSTANCE); + MeteredManagedHttpClientConnectionFactory connectionFactory = createConnectionFactory(networkBufferSize); connMgrBuilder.setConnectionFactory(connectionFactory); connMgrBuilder.setSSLSocketFactory(sslConnectionSocketFactory); diff --git a/client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java b/client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java index 5dc1e6269..9c56e53ce 100644 --- a/client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java +++ b/client-v2/src/test/java/com/clickhouse/client/HttpTransportTests.java @@ -1160,6 +1160,37 @@ public void testErrorWithSendProgressHeaders() throws Exception { } + @DataProvider(name = "connectionPoolEnabled") + public static Object[][] connectionPoolEnabled() { + return new Object[][] { { true }, { false } }; + } + + @Test(groups = { "integration" }, dataProvider = "connectionPoolEnabled") + public void testResponseWithManyProgressHeaders(boolean connectionPoolEnabled) throws Exception { + if (isCloud()) { + return; // mocked server + } + + ClickHouseNode server = getServer(ClickHouseProtocol.HTTP); + try (Client client = new Client.Builder().addEndpoint(server.getBaseUri()) + .setUsername("default") + .setPassword(ClickHouseServerForTest.getPassword()) + .enableConnectionPool(connectionPoolEnabled) + .build()) { + + // The server sends one X-ClickHouse-Progress header per progress interval, so a + // slow query produces many more response headers than the HTTP/1 parser default. + QuerySettings settings = new QuerySettings() + .serverSetting("send_progress_in_http_headers", "1") + .serverSetting("http_headers_progress_interval_ms", "10") + .serverSetting("max_block_size", "1"); + + List records = client.queryAll( + "SELECT number, sleep(0.05) FROM numbers(150)", settings); + Assert.assertEquals(records.size(), 150); + } + } + @Test(groups = { "integration" }, dataProvider = "testUserAgentHasCompleteProductName_dataProvider", dataProviderClass = HttpTransportTests.class) public void testUserAgentHasCompleteProductName(String clientName, Pattern userAgentPattern) throws Exception { if (isCloud()) {