diff --git a/http-clients/apache-client/src/test/java/software/amazon/awssdk/http/apache/ApacheHttpClientWireMockTest.java b/http-clients/apache-client/src/test/java/software/amazon/awssdk/http/apache/ApacheHttpClientWireMockTest.java index d9eb7bdd80df..27c09184a035 100644 --- a/http-clients/apache-client/src/test/java/software/amazon/awssdk/http/apache/ApacheHttpClientWireMockTest.java +++ b/http-clients/apache-client/src/test/java/software/amazon/awssdk/http/apache/ApacheHttpClientWireMockTest.java @@ -19,6 +19,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.any; import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static software.amazon.awssdk.http.SdkHttpConfigurationOption.TRUST_ALL_CERTIFICATES; @@ -46,6 +47,7 @@ import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import software.amazon.awssdk.http.HttpExecuteRequest; +import software.amazon.awssdk.http.HttpExecuteResponse; import software.amazon.awssdk.http.SdkHttpClient; import software.amazon.awssdk.http.SdkHttpClientTestSuite; import software.amazon.awssdk.http.SdkHttpFullRequest; @@ -53,6 +55,7 @@ import software.amazon.awssdk.http.apache.internal.ApacheHttpRequestConfig; import software.amazon.awssdk.http.apache.internal.impl.ConnectionManagerAwareHttpClient; import software.amazon.awssdk.utils.AttributeMap; +import software.amazon.awssdk.utils.IoUtils; @RunWith(MockitoJUnitRunner.class) public class ApacheHttpClientWireMockTest extends SdkHttpClientTestSuite { @@ -179,6 +182,45 @@ public void explicitNullDnsResolver_WithLocalhost_successful() throws Exception overrideDnsResolver("localhost", true); } + @Test + public void handlesVariousContentLengths() throws Exception { + SdkHttpClient client = createSdkHttpClient(); + int[] contentLengths = {0, 1, 100, 1024, 65536}; + + for (int length : contentLengths) { + String path = "/content-length-" + length; + byte[] body = new byte[length]; + for (int i = 0; i < length; i++) { + body[i] = (byte) ('A' + (i % 26)); + } + + mockServer.stubFor(any(urlPathEqualTo(path)) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Length", String.valueOf(length)) + .withBody(body))); + + SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port() + path, SdkHttpMethod.GET); + HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder() + .request(req) + .build()) + .call(); + + assertThat(rsp.httpResponse().statusCode()).isEqualTo(200); + + if (length == 0) { + // Empty body should still have a response body present, but EOF immediately + if (rsp.responseBody().isPresent()) { + assertThat(rsp.responseBody().get().read()).isEqualTo(-1); + } + } else { + assertThat(rsp.responseBody()).isPresent(); + byte[] readBody = IoUtils.toByteArray(rsp.responseBody().get()); + assertThat(readBody).isEqualTo(body); + } + } + } + private void overrideDnsResolver(String hostName) throws IOException { overrideDnsResolver(hostName, false); } diff --git a/http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/Apache5HttpClient.java b/http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/Apache5HttpClient.java index 0af8b7bffb1b..8a21cbcb299d 100644 --- a/http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/Apache5HttpClient.java +++ b/http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/Apache5HttpClient.java @@ -23,6 +23,7 @@ import static software.amazon.awssdk.http.apache5.internal.conn.ClientConnectionRequestFactory.THREAD_LOCAL_REQUEST_METRIC_COLLECTOR; import static software.amazon.awssdk.utils.NumericUtils.saturatedCast; +import java.io.ByteArrayInputStream; import java.io.IOException; import java.net.InetAddress; import java.security.KeyManagementException; @@ -56,10 +57,11 @@ import org.apache.hc.client5.http.ssl.SSLConnectionSocketFactory; import org.apache.hc.core5.http.ClassicHttpResponse; import org.apache.hc.core5.http.Header; +import org.apache.hc.core5.http.HttpEntity; +import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.config.Registry; import org.apache.hc.core5.http.impl.io.HttpRequestExecutor; import org.apache.hc.core5.http.io.SocketConfig; -import org.apache.hc.core5.http.io.entity.BufferedHttpEntity; import org.apache.hc.core5.io.CloseMode; import org.apache.hc.core5.pool.PoolStats; import org.apache.hc.core5.ssl.SSLInitializationException; @@ -262,20 +264,14 @@ private HttpExecuteResponse execute(HttpUriRequestBase apacheRequest, MetricColl HttpClientContext localRequestContext = Apache5Utils.newClientContext(requestConfig.proxyConfiguration()); THREAD_LOCAL_REQUEST_METRIC_COLLECTOR.set(metricCollector); try { - return httpClient.execute(apacheRequest, localRequestContext, response -> { - - // TODO : This is required since Apache5 closes streams immediately, check memory impacts because of this. - if (response.getEntity() != null) { - response.setEntity(new BufferedHttpEntity(response.getEntity())); - } - return createResponse(response, apacheRequest); - }); + HttpResponse httpResponse = httpClient.execute(apacheRequest, localRequestContext); + // Create a connection-aware input stream that closes the response when closed + return createResponse(httpResponse, apacheRequest); } finally { THREAD_LOCAL_REQUEST_METRIC_COLLECTOR.remove(); } } - private HttpUriRequestBase toApacheRequest(HttpExecuteRequest request) { return apacheHttpRequestFactory.create(request, requestConfig); } @@ -288,7 +284,7 @@ private HttpUriRequestBase toApacheRequest(HttpExecuteRequest request) { * @throws IOException If there were any problems getting any response information from the * HttpClient method object. */ - private HttpExecuteResponse createResponse(ClassicHttpResponse apacheHttpResponse, + private HttpExecuteResponse createResponse(HttpResponse apacheHttpResponse, HttpUriRequestBase apacheRequest) throws IOException { SdkHttpResponse.Builder responseBuilder = SdkHttpResponse.builder() @@ -302,17 +298,36 @@ private HttpExecuteResponse createResponse(ClassicHttpResponse apacheHttpRespons responseBuilder.appendHeader(header.getName(), header.getValue()); } - - AbortableInputStream responseBody = apacheHttpResponse.getEntity() != null ? - toAbortableInputStream(apacheHttpResponse, apacheRequest) : null; - + AbortableInputStream responseBody = getResponseBody(apacheHttpResponse, apacheRequest); return HttpExecuteResponse.builder().response(responseBuilder.build()).responseBody(responseBody).build(); } - private AbortableInputStream toAbortableInputStream(ClassicHttpResponse apacheHttpResponse, + private AbortableInputStream getResponseBody(HttpResponse apacheHttpResponse, + HttpUriRequestBase apacheRequest) throws IOException { + AbortableInputStream responseBody = null; + if (apacheHttpResponse instanceof ClassicHttpResponse) { + ClassicHttpResponse classicResponse = (ClassicHttpResponse) apacheHttpResponse; + HttpEntity entity = classicResponse.getEntity(); + if (entity != null) { + if (entity.getContentLength() == 0) { + // Close immediately for empty responses + classicResponse.close(); + responseBody = AbortableInputStream.create(new ByteArrayInputStream(new byte[0])); + } else { + responseBody = toAbortableInputStream(classicResponse, apacheRequest); + } + } else { + // No entity, close the response immediately + classicResponse.close(); + } + } + return responseBody; + } + + private AbortableInputStream toAbortableInputStream(ClassicHttpResponse apacheResponse, HttpUriRequestBase apacheRequest) throws IOException { - return AbortableInputStream.create(apacheHttpResponse.getEntity().getContent(), apacheRequest::abort); + return AbortableInputStream.create(apacheResponse.getEntity().getContent(), apacheRequest::abort); } private Apache5HttpRequestConfig createRequestConfig(DefaultBuilder builder, diff --git a/http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/Apache5HttpClientWireMockTest.java b/http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/Apache5HttpClientWireMockTest.java index 5f9ffaf2d3e3..c5f609bd9e7b 100644 --- a/http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/Apache5HttpClientWireMockTest.java +++ b/http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/Apache5HttpClientWireMockTest.java @@ -19,6 +19,7 @@ import static com.github.tomakehurst.wiremock.client.WireMock.any; import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo; import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static software.amazon.awssdk.http.SdkHttpConfigurationOption.TRUST_ALL_CERTIFICATES; @@ -47,6 +48,7 @@ import org.mockito.Mock; import org.mockito.junit.MockitoJUnitRunner; import software.amazon.awssdk.http.HttpExecuteRequest; +import software.amazon.awssdk.http.HttpExecuteResponse; import software.amazon.awssdk.http.SdkHttpClient; import software.amazon.awssdk.http.SdkHttpClientTestSuite; import software.amazon.awssdk.http.SdkHttpFullRequest; @@ -54,6 +56,7 @@ import software.amazon.awssdk.http.apache5.internal.Apache5HttpRequestConfig; import software.amazon.awssdk.http.apache5.internal.impl.ConnectionManagerAwareHttpClient; import software.amazon.awssdk.utils.AttributeMap; +import software.amazon.awssdk.utils.IoUtils; @RunWith(MockitoJUnitRunner.class) public class Apache5HttpClientWireMockTest extends SdkHttpClientTestSuite { @@ -173,6 +176,47 @@ public void explicitNullDnsResolver_WithLocalhost_successful() throws Exception overrideDnsResolver("localhost", true); } + + + @Test + public void handlesVariousContentLengths() throws Exception { + SdkHttpClient client = createSdkHttpClient(); + int[] contentLengths = {0, 1, 100, 1024, 65536}; + + for (int length : contentLengths) { + String path = "/content-length-" + length; + byte[] body = new byte[length]; + for (int i = 0; i < length; i++) { + body[i] = (byte) ('A' + (i % 26)); + } + + mockServer.stubFor(any(urlPathEqualTo(path)) + .willReturn(aResponse() + .withStatus(200) + .withHeader("Content-Length", String.valueOf(length)) + .withBody(body))); + + SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port() + path, SdkHttpMethod.GET); + HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder() + .request(req) + .build()) + .call(); + + assertThat(rsp.httpResponse().statusCode()).isEqualTo(200); + + if (length == 0) { + // Empty body should still have a response body present, but EOF immediately + if (rsp.responseBody().isPresent()) { + assertThat(rsp.responseBody().get().read()).isEqualTo(-1); + } + } else { + assertThat(rsp.responseBody()).isPresent(); + byte[] readBody = IoUtils.toByteArray(rsp.responseBody().get()); + assertThat(readBody).isEqualTo(body); + } + } + } + private void overrideDnsResolver(String hostName) throws IOException { overrideDnsResolver(hostName, false); } diff --git a/http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/MetricReportingTest.java b/http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/MetricReportingTest.java index d2c3cb5147b5..8437f74e8859 100644 --- a/http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/MetricReportingTest.java +++ b/http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/MetricReportingTest.java @@ -64,12 +64,8 @@ public class MetricReportingTest { @Before public void methodSetup() throws IOException { - ClassicHttpResponse httpResponse = new BasicClassicHttpResponse(200, "OK"); - when(mockHttpClient.execute(any(HttpUriRequest.class), any(HttpContext.class), any(HttpClientResponseHandler.class))) - .thenAnswer(invocation -> { - HttpClientResponseHandler handler = invocation.getArgument(2); - return handler.handleResponse(httpResponse); - }); + when(mockHttpClient.execute(any(HttpUriRequest.class), any(HttpContext.class))) + .thenReturn(new BasicClassicHttpResponse(200, "OK")); when(mockHttpClient.getHttpClientConnectionManager()).thenReturn(cm);