Merge pull request #2497 from 5cmc/main

Ensure proper closure of `httpResponse` in error scenarios during retries

Author: chernser

client-v2/src/main/java/com/clickhouse/client/api/Client.java

@@ -55,6 +55,7 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.lang.reflect.InvocationTargetException;
+import java.net.MalformedURLException;
 import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.time.Duration;
@@ -121,7 +122,7 @@ public class Client implements AutoCloseable {
     private final Map<String, Object> configuration;
 
     private final Map<String, String> readOnlyConfig;
-    
+
     private final POJOSerDe pojoSerDe;
 
     private final ExecutorService sharedOperationExecutor;
@@ -291,7 +292,7 @@ public Builder() {
          */
         public Builder addEndpoint(String endpoint) {
             try {
-                URL endpointURL = new java.net.URL(endpoint);
+                URL endpointURL = new URL(endpoint);
 
                 if (endpointURL.getProtocol().equalsIgnoreCase("https")) {
                     addEndpoint(Protocol.HTTP, endpointURL.getHost(), endpointURL.getPort(), true);
@@ -300,7 +301,7 @@ public Builder addEndpoint(String endpoint) {
                 } else {
                     throw new IllegalArgumentException("Only HTTP and HTTPS protocols are supported");
                 }
-            } catch (java.net.MalformedURLException e) {
+            } catch (MalformedURLException e) {
                 throw new IllegalArgumentException("Endpoint should be a valid URL string, but was " + endpoint, e);
             }
             return this;
@@ -380,7 +381,7 @@ public Builder setAccessToken(String accessToken) {
 
         /**
          * Makes client to use SSL Client Certificate to authenticate with server.
-         * Client certificate should be set as well. {@link Client.Builder#setClientCertificate(String)}
+         * Client certificate should be set as well. {@link Builder#setClientCertificate(String)}
          * @param useSSLAuthentication
          * @return
          */
@@ -1583,8 +1584,9 @@ public CompletableFuture<QueryResponse> query(String sqlQuery, Map<String, Objec
                 Endpoint selectedEndpoint = getNextAliveNode();
                 RuntimeException lastException = null;
                 for (int i = 0; i <= retries; i++) {
+                    ClassicHttpResponse httpResponse = null;
                     try {
-                        ClassicHttpResponse httpResponse =
+                        httpResponse =
                                 httpClientHelper.executeRequest(selectedEndpoint, finalSettings.getAllSettings(), lz4Factory, output -> {
                                     output.write(sqlQuery.getBytes(StandardCharsets.UTF_8));
                                     output.close();
@@ -1614,6 +1616,7 @@ public CompletableFuture<QueryResponse> query(String sqlQuery, Map<String, Objec
                         return new QueryResponse(httpResponse, responseFormat, finalSettings, metrics);
 
                     } catch (Exception e) {
+                        httpClientHelper.closeQuietly(httpResponse);
                         lastException = httpClientHelper.wrapException(String.format("Query request failed (Attempt: %s/%s - Duration: %s)",
                                 (i + 1), (retries + 1), System.nanoTime() - startTime), e);
                         if (httpClientHelper.shouldRetry(e, finalSettings.getAllSettings())) {

client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java

@@ -428,8 +428,9 @@ public ClassicHttpResponse executeRequest(Endpoint server, Map<String, Object> r
 
         HttpClientContext context = HttpClientContext.create();
 
+        ClassicHttpResponse httpResponse = null;
         try {
-            ClassicHttpResponse httpResponse = httpClient.executeOpen(null, req, context);
+            httpResponse = httpClient.executeOpen(null, req, context);
             boolean serverCompression = ClientConfigProperties.COMPRESS_SERVER_RESPONSE.getOrDefault(requestConfig);
             httpResponse.setEntity(wrapResponseEntity(httpResponse.getEntity(), httpResponse.getCode(), serverCompression, useHttpCompression, lz4Factory, requestConfig));
 
@@ -448,14 +449,26 @@ public ClassicHttpResponse executeRequest(Endpoint server, Map<String, Object> r
             return httpResponse;
 
         } catch (UnknownHostException e) {
+            closeQuietly(httpResponse);
             LOG.warn("Host '{}' unknown", server.getBaseURL());
             throw e;
         } catch (ConnectException | NoRouteToHostException e) {
+            closeQuietly(httpResponse);
             LOG.warn("Failed to connect to '{}': {}", server.getBaseURL(), e.getMessage());
             throw e;
         }
     }
 
+    public void closeQuietly(ClassicHttpResponse httpResponse) {
+        if (httpResponse != null) {
+            try {
+                httpResponse.close();
+            } catch (IOException e) {
+                LOG.warn("Failed to close response");
+            }
+        }
+    }
+
     private static final ContentType CONTENT_TYPE = ContentType.create(ContentType.TEXT_PLAIN.getMimeType(), "UTF-8");
 
     private void addHeaders(HttpPost req, Map<String, Object> requestConfig) {