[Core Components] OEmbedClientImpl leaks network connections #2717
Conversation
vladbailescu
commented
Apr 8, 2024
vladbailescu
commented
- Refactored code to move HTTP client creation to a dedicated method and wrap the whole method that does the reading in try-with-resources
* Refactored code to move HTTP client creation to a dedicated method and wrap the whole method that does the reading in try-with-resources
| } | ||
|
|
||
| protected InputStream getData(String url, HttpClient httpClient) throws IOException, IllegalArgumentException { | ||
| HttpResponse response = httpClient.execute(new HttpGet(url)); |
There was a problem hiding this comment.
I really recommend implementing https://www.javadoc.io/doc/org.apache.httpcomponents/httpclient/4.3.2/org/apache/http/client/ResponseHandler.html instead and pass that to execute. Otherwise you need to manually make sure to consume the inputstream fully (e.g. with https://hc.apache.org/httpcomponents-core-4.4.x/current/httpcore/apidocs/org/apache/http/util/EntityUtils.html#consumeQuietly(org.apache.http.HttpEntity)).
There was a problem hiding this comment.
Alternatively you need to close the HttpResponse (but this is more effort, since you need to defer until the inputstream is read). In the best case the HttpClient is being reused across multiple requests (in order to cache HTTP connections), then releasing individual connections/responses/inputstreams become crucial.
There was a problem hiding this comment.
The JSON/XML un-marshalling should take care of consuming the input stream. Seems a bit ineffective to consume it to a string/buffer (could be large) and then feed it through the un-marshalers
There was a problem hiding this comment.
The idea is rather to call the unmarshaller inside your ResponseHandler, then you don't need to come up with your own exception handling for not-fully consumed input streams.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
...src/main/java/com/adobe/cq/wcm/core/components/internal/services/embed/OEmbedClientImpl.java
Dismissed
Show dismissed
Hide dismissed
