Ensure proper closure of httpResponse in error scenarios during retries#2497
Ensure proper closure of httpResponse in error scenarios during retries#2497chernser merged 2 commits intoClickHouse:mainfrom
httpResponse in error scenarios during retries#2497Conversation
|
PR review rate limit exceeded |
|
|
orsondmc
changed the title
httpResponse in error scenarios during ret…httpResponse in error scenarios during retries
|
lgtm! |
There was a problem hiding this comment.
Pull Request Overview
This PR adds explicit closing of the HTTP response in the retry loop to prevent potential connection leaks and cleans up some imports and Javadoc links.
- Ensure
ClassicHttpResponseis closed on exception paths during retries - Simplify URL instantiation and catch clauses by updating imports
- Correct Javadoc link reference for
setClientCertificate
| try { | ||
| httpResponse.close(); | ||
| } catch (IOException ex) { | ||
| throw new ClientException("Failed to close response", e); |
There was a problem hiding this comment.
The ClientException is wrapping the original exception e instead of the IOException ex from the close operation. It should pass ex as the cause to accurately represent the close failure.
| throw new ClientException("Failed to close response", e); | |
| throw new ClientException("Failed to close response", ex); |
| try { | ||
| httpResponse.close(); | ||
| } catch (IOException ex) { | ||
| throw new ClientException("Failed to close response", e); |
There was a problem hiding this comment.
Throwing a ClientException inside the close catch block will exit the retry loop prematurely, potentially skipping further retry attempts. Consider logging the close error instead and allowing the retry logic to continue.
| throw new ClientException("Failed to close response", e); | |
| LOG.error("Failed to close response", ex); |
| Endpoint selectedEndpoint = getNextAliveNode(); | ||
| RuntimeException lastException = null; | ||
| for (int i = 0; i <= retries; i++) { | ||
| ClassicHttpResponse httpResponse = null; |
There was a problem hiding this comment.
[nitpick] Consider using a try-with-resources statement for ClassicHttpResponse to ensure it's always closed automatically, simplifying the error-handling logic.
…tent resource cleanup.
|
I have been running this overnight and have not seen any of the leaks I was seeing. TBH, I feel like the entire way HTTP connections are being handled and closed needs to be rewritten in a safer way. The way it works right now is too complicated - transport is intertwined with the logic of the Client API. Happy to contribute a proposal for that as a follow up to this PR. |
|
@orsondmc Thank you for the contribution! |
|
My pleasure |
Summary
May resolve #1741 . In error cases, the httpResponse is not closed and this could lead to a connection leak.