Skip to content

Commit

Permalink
fix: ClosableHttpClient.execute() resource leak on API catalog (#3722)
Browse files Browse the repository at this point in the history
  • Loading branch information
@richard-salac
richard-salac authored Sep 11, 2024
1 parent 564757f commit a330907
Show file tree
Hide file tree
Showing 39 changed files with 645 additions and 604 deletions.
1 change: 1 addition & 0 deletions api-catalog-services/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ dependencies {

testImplementation libs.spring.boot.starter.test
testImplementation libs.spring.mock.mvc
testImplementation(testFixtures(project(":apiml-common")))

compileOnly libs.lombok
annotationProcessor libs.lombok
Expand Down
48 changes: 27 additions & 21 deletions ...g-services/src/main/java/org/zowe/apiml/apicatalog/instance/InstanceRetrievalService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,17 @@
import com.netflix.appinfo.InstanceInfo;
import com.netflix.discovery.converters.jackson.EurekaJsonJacksonCodec;
import com.netflix.discovery.shared.Applications;
import jakarta.validation.constraints.NotBlank;
import lombok.extern.slf4j.Slf4j;
import org.apache.hc.client5.http.classic.methods.HttpGet;
import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
import org.apache.hc.core5.http.*;
import org.apache.hc.core5.http.Header;
import org.apache.hc.core5.http.HttpEntity;
import org.apache.hc.core5.http.HttpHeaders;
import org.apache.hc.core5.http.io.entity.EntityUtils;
import org.apache.hc.core5.http.message.BasicHeader;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.HttpStatus;
import org.springframework.http.MediaType;
import org.springframework.stereotype.Service;
import org.zowe.apiml.apicatalog.discovery.DiscoveryConfigProperties;
Expand All @@ -32,7 +35,6 @@
import org.zowe.apiml.product.logging.annotations.InjectApimlLogger;
import org.zowe.apiml.product.registry.ApplicationWrapper;

import jakarta.validation.constraints.NotBlank;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.ArrayList;
Expand Down Expand Up @@ -139,30 +141,34 @@ private Applications extractApplications(String responseBody) {
* @param eurekaServiceInstanceRequest information used to query the discovery service
* @return ResponseEntity<String> query response
*/
private String queryDiscoveryForInstances(EurekaServiceInstanceRequest eurekaServiceInstanceRequest) throws IOException, ParseException {
private String queryDiscoveryForInstances(EurekaServiceInstanceRequest eurekaServiceInstanceRequest) throws IOException {
HttpGet httpGet = new HttpGet(eurekaServiceInstanceRequest.getEurekaRequestUrl());
for (Header header : createRequestHeader(eurekaServiceInstanceRequest)) {
httpGet.setHeader(header);
}
CloseableHttpResponse response = httpClient.execute(httpGet);
final int statusCode = response.getCode();
final HttpEntity responseEntity = response.getEntity();
String responseBody = "";
if (responseEntity != null) {
responseBody = EntityUtils.toString(responseEntity, StandardCharsets.UTF_8);
}
if (statusCode >= HttpStatus.SC_OK && statusCode < HttpStatus.SC_MULTIPLE_CHOICES) {
return responseBody;
}

apimlLog.log("org.zowe.apiml.apicatalog.serviceRetrievalRequestFailed",
eurekaServiceInstanceRequest.getServiceId(),
eurekaServiceInstanceRequest.getEurekaRequestUrl(),
statusCode,
response.getReasonPhrase() != null ? response.getReasonPhrase() : responseBody
return httpClient.execute(httpGet, response -> {
final int statusCode = response.getCode();
final HttpEntity responseEntity = response.getEntity();

String responseBody = "";
if (responseEntity != null) {
responseBody = EntityUtils.toString(responseEntity, StandardCharsets.UTF_8);
}

if (HttpStatus.valueOf(statusCode).is2xxSuccessful()) {
return responseBody;
}

apimlLog.log("org.zowe.apiml.apicatalog.serviceRetrievalRequestFailed",
eurekaServiceInstanceRequest.getServiceId(),
eurekaServiceInstanceRequest.getEurekaRequestUrl(),
statusCode,
response.getReasonPhrase() != null ? response.getReasonPhrase() : responseBody
);

return null;
return null;
});
}

/**
Expand All @@ -177,7 +183,7 @@ private InstanceInfo extractSingleInstanceFromApplication(String serviceId, Stri
try {
application = mapper.readValue(responseBody, ApplicationWrapper.class);
} catch (IOException e) {
log.debug("Could not extract service: " + serviceId + " info from discovery --" + e.getMessage(), e);
log.debug("Could not extract service: {} info from discovery --{}", serviceId, e.getMessage(), e);
}


Expand Down
49 changes: 23 additions & 26 deletions ...vices/src/main/java/org/zowe/apiml/apicatalog/services/status/APIDocRetrievalService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,10 @@
import lombok.NonNull;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.hc.client5.http.classic.methods.HttpGet;
import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
import org.apache.hc.core5.http.*;
import org.apache.hc.core5.http.HttpStatus;
import org.apache.hc.core5.http.io.entity.EntityUtils;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty;
import org.springframework.http.MediaType;
Expand All @@ -35,8 +33,8 @@
import org.zowe.apiml.eurekaservice.client.util.EurekaMetadataParser;
import org.zowe.apiml.message.log.ApimlLogger;
import org.zowe.apiml.product.gateway.GatewayClient;
import org.zowe.apiml.product.instance.ServiceAddress;
import org.zowe.apiml.product.instance.InstanceInitializationException;
import org.zowe.apiml.product.instance.ServiceAddress;
import org.zowe.apiml.product.logging.annotations.InjectApimlLogger;
import org.zowe.apiml.product.routing.RoutedService;
import org.zowe.apiml.product.routing.RoutedServices;
Expand All @@ -46,7 +44,6 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Map;
import java.util.Optional;

/**
* Retrieves the API documentation for a registered service
Expand All @@ -60,7 +57,6 @@
@Slf4j
public class APIDocRetrievalService {

@Autowired
@Qualifier("secureHttpClientWithoutKeystore")
private final CloseableHttpClient secureHttpClientWithoutKeystore;

Expand Down Expand Up @@ -311,17 +307,21 @@ private String getApiDocContentByUrl(@NonNull String serviceId, String apiDocUrl
HttpGet httpGet = new HttpGet(apiDocUrl);
httpGet.setHeader(org.apache.http.HttpHeaders.ACCEPT, MediaType.APPLICATION_JSON_VALUE);

var responseCodeBodyPair = secureHttpClientWithoutKeystore.execute(httpGet, response -> {
var responseBody = EntityUtils.toString(response.getEntity(), StandardCharsets.UTF_8);
return Pair.of(response.getCode(), responseBody);
return secureHttpClientWithoutKeystore.execute(httpGet, response -> {
String responseBody = "";
var responseEntity = response.getEntity();
if (responseEntity != null) {
responseBody = EntityUtils.toString(responseEntity, StandardCharsets.UTF_8);
}

if (HttpStatus.SC_OK == response.getCode()) {
return responseBody;
} else {
throw new ApiDocNotFoundException("No API Documentation was retrieved due to " + serviceId +
" server error: '" + responseBody + "'.");
}
}
);

if (responseCodeBodyPair.getLeft() != HttpStatus.SC_OK) {
throw new ApiDocNotFoundException("No API Documentation was retrieved due to " + serviceId +
" server error: '" + responseCodeBodyPair.getRight() + "'.");
}
return responseCodeBodyPair.getRight();
}

/**
Expand Down Expand Up @@ -364,19 +364,16 @@ private ApiInfo findApi(List<ApiInfo> apiInfos, String apiVersion) {
String apiId = api.length > 0 ? api[0] : "";
String version = api.length > 1 ? api[1].replace("v", "") : "";

Optional<ApiInfo> result = apiInfos.stream()
return apiInfos.stream()
.filter(
f -> apiId.equals(f.getApiId()) && (version == null || version.equals(f.getVersion()))
f -> apiId.equals(f.getApiId()) && (version.equals(f.getVersion()))
)
.findFirst();

if (!result.isPresent()) {
String errMessage = String.format("Error finding api doc: there is no api doc for '%s %s'.", apiId, version);
log.error(errMessage);
throw new ApiDocNotFoundException(errMessage);
} else {
return result.get();
}
.findFirst()
.orElseThrow(() -> {
String errMessage = String.format("Error finding api doc: there is no api doc for '%s %s'.", apiId, version);
log.error(errMessage);
return new ApiDocNotFoundException(errMessage);
});
}

private InstanceInfo getInstanceInfo(String serviceId) {
Expand Down
29 changes: 16 additions & 13 deletions api-catalog-services/src/main/java/org/zowe/apiml/apicatalog/staticapi/StaticAPIService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,15 @@
import lombok.extern.slf4j.Slf4j;
import org.apache.hc.client5.http.classic.methods.HttpPost;
import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
import org.apache.hc.core5.http.HttpEntity;
import org.springframework.beans.factory.annotation.Qualifier;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.http.HttpStatus;
import org.springframework.stereotype.Service;
import org.zowe.apiml.apicatalog.discovery.DiscoveryConfigProperties;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.util.ArrayList;
import java.util.Base64;
Expand Down Expand Up @@ -57,27 +58,29 @@ public StaticAPIResponse refresh() {

try {
HttpPost post = getHttpRequest(discoveryServiceUrl);
CloseableHttpResponse response = httpClient.execute(post);
final HttpEntity responseEntity = response.getEntity();
String responseBody = "";
if (responseEntity != null) {
responseBody = new BufferedReader(new InputStreamReader(responseEntity.getContent())).lines().collect(Collectors.joining("\n"));
}
// Return response if successful response or if none have been successful and this is the last URL to try
if (isSuccessful(response) || i == discoveryServiceUrls.size() - 1) {
var staticApiResponse = httpClient.execute(post, response -> {
final HttpEntity responseEntity = response.getEntity();
String responseBody = "";
if (responseEntity != null) {
responseBody = new BufferedReader(new InputStreamReader(responseEntity.getContent())).lines().collect(Collectors.joining("\n"));
}
return new StaticAPIResponse(response.getCode(), responseBody);
}
});

} catch (Exception e) {
// Return response if successful or if none have been successful and this is the last URL to try
if (isSuccessful(staticApiResponse) || i == discoveryServiceUrls.size() - 1) {
return staticApiResponse;
}
} catch (IOException e) {
log.debug("Error refreshing static APIs from {}, error message: {}", discoveryServiceUrl, e.getMessage());
}
}

return new StaticAPIResponse(500, "Error making static API refresh request to the Discovery Service");
}

private boolean isSuccessful(CloseableHttpResponse response) {
return response.getCode() >= 200 && response.getCode() <= 299;
private boolean isSuccessful(StaticAPIResponse response) {
return HttpStatus.valueOf(response.getStatusCode()).is2xxSuccessful();
}

private HttpPost getHttpRequest(String discoveryServiceUrl) {
Expand Down
18 changes: 11 additions & 7 deletions ...rvices/src/test/java/org/zowe/apiml/apicatalog/instance/InstanceRetrievalServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
import org.apache.commons.io.IOUtils;
import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
import org.apache.hc.client5.http.impl.classic.CloseableHttpResponse;
import org.apache.hc.core5.http.ClassicHttpRequest;
import org.apache.hc.core5.http.HttpStatus;
import org.apache.hc.core5.http.io.HttpClientResponseHandler;
import org.apache.hc.core5.http.io.entity.BasicHttpEntity;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
Expand All @@ -33,6 +35,7 @@
import org.zowe.apiml.product.constants.CoreService;
import org.zowe.apiml.product.instance.InstanceInitializationException;
import org.zowe.apiml.product.registry.ApplicationWrapper;
import org.zowe.apiml.util.HttpClientMockHelper;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
Expand All @@ -58,15 +61,16 @@ class InstanceRetrievalServiceTest {
private DiscoveryConfigProperties discoveryConfigProperties;

@Mock
CloseableHttpClient httpClient;
private CloseableHttpClient httpClient;

@Mock
private CloseableHttpResponse response;

@BeforeEach
void setup() throws IOException {
response = mock(CloseableHttpResponse.class);
when(response.getCode()).thenReturn(HttpStatus.SC_OK);
when(httpClient.execute(any())).thenReturn(response);
void setup() {
HttpClientMockHelper.mockExecuteWithResponse(httpClient, response);
HttpClientMockHelper.mockResponse(response, HttpStatus.SC_OK);

instanceRetrievalService = new InstanceRetrievalService(discoveryConfigProperties, httpClient);
}

Expand All @@ -75,7 +79,7 @@ void whenDiscoveryServiceIsNotAvailable_thenTryOthersFromTheList() throws IOExce
when(response.getCode()).thenReturn(HttpStatus.SC_FORBIDDEN).thenReturn(HttpStatus.SC_OK);

instanceRetrievalService.getAllInstancesFromDiscovery(false);
verify(httpClient, times(2)).execute(any());
verify(httpClient, times(2)).execute(any(ClassicHttpRequest.class), any(HttpClientResponseHandler.class));
}

@Test
Expand Down Expand Up @@ -137,7 +141,7 @@ void testGetAllInstancesFromDiscovery_whenResponseCodeIsNotSuccess() {
}

@Test
void testGetAllInstancesFromDiscovery_whenResponseCodeIsSuccessWithUnParsedJsonText() throws IOException {
void testGetAllInstancesFromDiscovery_whenResponseCodeIsSuccessWithUnParsedJsonText() {
Applications actualApplications = instanceRetrievalService.getAllInstancesFromDiscovery(false);
assertNull(actualApplications);
}
Expand Down
Loading

0 comments on commit a330907

Please sign in to comment.