[GitHub] [helix] pkuwm commented on a change in pull request #1472: Fix connection leakage in CustomRestClient

[GitBox <gi...@apache.org>]

pkuwm commented on a change in pull request #1472:
URL: https://github.com/apache/helix/pull/1472#discussion_r506746581



##########
File path: helix-rest/src/main/java/org/apache/helix/rest/client/CustomRestClientImpl.java
##########
@@ -116,25 +116,36 @@ protected JsonNode getJsonObject(HttpResponse httpResponse) throws IOException {
       LOG.info("Expected HttpResponse statusCode: {}", HttpStatus.SC_OK);
       return jsonConverter.convert(getJsonObject(httpResponse));
     } else {
+      // Ensure entity is fully consumed so stream is closed.
+      EntityUtils.consumeQuietly(httpResponse.getEntity());
       throw new ClientProtocolException("Unexpected response status: " + status + ", reason: "
           + httpResponse.getStatusLine().getReasonPhrase());
     }
   }
 
   @VisibleForTesting
   protected HttpResponse post(String url, Map<String, String> payloads) throws IOException {
+    HttpPost postRequest = new HttpPost(url);
     try {
-      HttpPost postRequest = new HttpPost(url);
       postRequest.setHeader("Accept", ACCEPT_CONTENT_TYPE);
       StringEntity entity = new StringEntity(OBJECT_MAPPER.writeValueAsString(payloads),
           ContentType.APPLICATION_JSON);
       postRequest.setEntity(entity);
       LOG.info("Executing request: {}, headers: {}, entity: {}", postRequest.getRequestLine(),
           postRequest.getAllHeaders(), postRequest.getEntity());
-      return _httpClient.execute(postRequest);
+
+      HttpResponse response = _httpClient.execute(postRequest);

Review comment:
       I tried that. If we move to `handleResponse`, we'll have to change more code, eg. method signature of `handleResponse` to have an extra parameter `payloads`, and we need to change the callers as well. That sounds more work to me and it doesn't look better than this. I prefer this way so it is cleaner.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org