Refactor HttpAPIClientHelper to improve response cleanup handling

[aleksei-griaznov]

Summary

Follow-up to #2497.

This PR improves resource management in the client implementation:

1. Ensures that ClassicHttpResponse is properly closed in case of unexpected exceptions in the executeRequest method of HttpAPIClientHelper:

   } catch (Exception e) {
       closeQuietly(httpResponse);
       throw e;
   }

2. Adds missing resource cleanup in the query method of com.clickhouse.client.api.Client.

These changes help prevent potential resource leaks and ensure consistent cleanup behavior across client operations.

[CLAassistant]

All committers have signed the CLA.

[windsurf-bot]

💡 To request another review, post a new comment with "/windsurf-review".

[windsurf-bot]

The readError method doesn't close the httpResponse after reading the error. Since the surrounding try-finally block was removed, this could lead to resource leaks. Consider closing the response in this method:

Suggested change

	if (httpResponse.getCode() == HttpStatus.SC_PROXY_AUTHENTICATION_REQUIRED) {
	throw new ClientMisconfigurationException("Proxy authentication required. Please check your proxy settings.");
	} else if (httpResponse.getCode() == HttpStatus.SC_BAD_GATEWAY) {
	httpResponse.close();
	throw new ClientException("Server returned '502 Bad gateway'. Check network and proxy settings.");
	} else if (httpResponse.getCode() >= HttpStatus.SC_BAD_REQUEST || httpResponse.containsHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE)) {
	try {
	throw readError(httpResponse);
	} finally {
	httpResponse.close();
	}
	throw readError(httpResponse);
	}
	public Exception readError(ClassicHttpResponse httpResponse) {
	try {
	int serverCode = getHeaderInt(httpResponse.getFirstHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE), 0);
	InputStream body = null;
	try {
	// ... existing code ...
	return new ServerException(serverCode, "Code: " + msg, httpResponse.getCode());
	} catch (Exception e) {
	LOG.error("Failed to read error message", e);
	return new ServerException(serverCode, String.format(ERROR_CODE_PREFIX_PATTERN, serverCode) + " <Unreadable error message> (transport error: " + httpResponse.getCode() + ")", httpResponse.getCode());
	}
	} finally {
	closeQuietly(httpResponse);
	}
	}

[aleksei-griaznov]

The resource is closed correctly: When throw readError(httpResponse) throws an exception at line 459, control passes to the catch (Exception e) block at line 471, where closeQuietly(httpResponse) is called at line 472.

[chernser]

@aleksei-griaznov this is true but we need to release resources asap - because user code may forget to close the response. Even it is a double check - it worth it.

[chernser]

Good day, @aleksei-griaznov !

Thank you for the contribution! Will review it shortly.
Please sing CLA so we can merge after the review.

Thanks!

[chernser]

We have done this in the HTTPAPIClientHelper because it is helper responsibility to handle closure on error.
Removing this lines not a problem but logic looks incomplete because we do not close resource as soon as we have got an error.
Please keep this part unchanged. We will return to it later while implementing Network layer.
Thanks!

[aleksei-griaznov]

Hi @chernser,

Thanks for the explanation!

My initial idea was that the exception thrown by throw readError(httpResponse) would be handled in the newly added catch block, with the response being closed within the same method executeRequest:

        } catch (Exception e) {
            closeQuietly(httpResponse);
            throw e;
        }

It seems safe to have both explicit in-place cleanup logic and a general one, so I've restored the logic with explicit HTTP response closing.

[chernser]

@aleksei-griaznov
overall looks good for to me. Please apply comment and it is good to go.

Thank you!

[aleksei-griaznov]

Hi @chernser,

Thanks — I’ve addressed the comments and also added debug logging to the following catch block:

        } catch (Exception e) {
            closeQuietly(httpResponse);
            LOG.debug("Failed to execute request to '{}': {}", server.getBaseURL(), e.getMessage(), e);
            throw e;
        }

It looks like only exceptions that pass the httpClientHelper.shouldRetry(..) condition are logged at a higher level in the Client#query(...) method:

                    } catch (Exception e) {
                        ...
                        if (httpClientHelper.shouldRetry(e, requestSettings.getAllSettings())) {
                            LOG.warn("Retrying.", e);
                            selectedEndpoint = getNextAliveNode();
                        } else {
                            throw lastException;
                        }
                    }

So this additional debug logging could be useful for troubleshooting cases where an exception doesn’t meet the retry condition and isn’t logged otherwise.

[chernser]

Thank you, @aleksei-griaznov !
I've approved and will merge once CI tests pass.