Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Core Components] OEmbedClientImpl leaks network connections #2717

Merged
merged 4 commits into from
Apr 10, 2024
Merged

[Core Components] OEmbedClientImpl leaks network connections #2717

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9e09447
[Core Components] OEmbedClientImpl leaks network connections
vladbailescu Apr 8, 2024
fd46da1
Merge branch 'main' into issue/SITES-14725-2
vladbailescu Apr 9, 2024
10676f7
try-with-resources for input stream too
vladbailescu Apr 9, 2024
94b62a8
Merge branch 'main' into issue/SITES-14725-2
vladbailescu Apr 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 17 additions & 27 deletions .../main/java/com/adobe/cq/wcm/core/components/internal/services/embed/OEmbedClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import java.io.IOException;
import java.io.InputStream;
import java.net.MalformedURLException;
import java.net.URISyntaxException;
import java.util.HashMap;
import java.util.Map;
import java.util.regex.Pattern;
Expand Down Expand Up @@ -103,17 +102,14 @@ public OEmbedResponse getResponse(String url) {
if (config == null) {
return null;
}
if (OEmbedResponse.Format.JSON == OEmbedResponse.Format.fromString(config.format())) {
try {
OEmbedResponse.Format format = OEmbedResponse.Format.fromString(config.format());
try (CloseableHttpClient httpClient = getHttpClient()) {
if (OEmbedResponse.Format.JSON == format) {
String jsonURL = buildURL(config.endpoint(), url, OEmbedResponse.Format.JSON.getValue(), null, null);
return mapper.readValue(getData(jsonURL), OEmbedJSONResponseImpl.class);
} catch (IllegalArgumentException | IOException ioex) {
LOGGER.error("Failed to read JSON response", ioex);
}
} else if (jaxbContext != null && OEmbedResponse.Format.XML == OEmbedResponse.Format.fromString(config.format())) {
try {
return mapper.readValue(getData(jsonURL, httpClient), OEmbedJSONResponseImpl.class);
} else if (jaxbContext != null && OEmbedResponse.Format.XML == format) {
String xmlURL = buildURL(config.endpoint(), url, OEmbedResponse.Format.XML.getValue(), null, null);
try (InputStream xmlStream = getData(xmlURL)) {
try (InputStream xmlStream = getData(xmlURL, httpClient)) {
//Disable XXE
SAXParserFactory spf = SAXParserFactory.newInstance();
spf.setFeature("http://xml.org/sax/features/external-general-entities", false);
Expand All @@ -125,9 +121,9 @@ public OEmbedResponse getResponse(String url) {
Unmarshaller jaxbUnmarshaller = jaxbContext.createUnmarshaller();
return (OEmbedResponse) jaxbUnmarshaller.unmarshal(xmlSource);
}
} catch (IllegalArgumentException | SAXException | ParserConfigurationException | JAXBException | IOException e) {
LOGGER.error("Failed to read XML response", e);
}
} catch (IllegalArgumentException | SAXException | ParserConfigurationException | JAXBException | IOException e) {
LOGGER.error("Failed to read " + format + " response", e);
}
return null;
}
Expand Down Expand Up @@ -156,24 +152,18 @@ protected OEmbedClientImplConfigurationFactory.Config getConfiguration(String ur
return null;
}

protected InputStream getData(String url) throws IOException, IllegalArgumentException {
protected CloseableHttpClient getHttpClient() {
RequestConfig rc = RequestConfig.custom().setConnectTimeout(connectionTimeout).setSocketTimeout(soTimeout)
.build();
HttpResponse response;
if (httpClientBuilderFactory != null
&& httpClientBuilderFactory.newBuilder() != null) {
try (CloseableHttpClient httpClient = httpClientBuilderFactory.newBuilder()
.setDefaultRequestConfig(rc)
.build()){
response = httpClient.execute(new HttpGet(url));
}
.build();
if (httpClientBuilderFactory != null && httpClientBuilderFactory.newBuilder() != null) {
return httpClientBuilderFactory.newBuilder().setDefaultRequestConfig(rc).build();
} else {
try (CloseableHttpClient httpClient = HttpClients.custom()
.setDefaultRequestConfig(rc)
.build()){
response = httpClient.execute(new HttpGet(url));
}
return HttpClients.custom().setDefaultRequestConfig(rc).build();
}
}

protected InputStream getData(String url, HttpClient httpClient) throws IOException, IllegalArgumentException {
HttpResponse response = httpClient.execute(new HttpGet(url));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@kwin kwin Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kwin , the ResponseHandler-based implementation would look something like: #2718

I can't say I particularly like it or find it better, the HttpResponse is not closable and we are not reusing the client any way between calls.

Dismissed Show dismissed Hide dismissed
return response.getEntity().getContent();
}

Expand Down
Loading