Skip to content

Commit 3cedfb1

Browse files
joviegasjoviegas
authored
Do not buffer the Response stream using BufferedHttpEntity (#6200)
* Fix architecture test failures for apache5.x * Checkstyle issues * Update to use PoolingHttpClientConnectionManager class reference that is implementation of HttpClientConnectionManager * Fix stream reset failure in RepeatableInputStreamRequestEntity by storing content reference to avoid multiple ContentStreamProvider.newStream() calls that cause IOException when retrying requests with non-resettable streams * writeTo_ConcurrentWrites_HandlesCorrectly no longer needed since even Apache 4.x doesnot suports this * Fix connectionPoolingWorks by setting skipping setConnectionTimeToLive is value is set to 0 since 0 is treated as Infinite timeToLive in Sdk and Apache 4.x but treated as immediate closeConnection in apache 5.x * disableAutomaticRetries in Apache 5.x since SDK handles retries , also define Apache5 dependencies in .brazil.json * Added Test case for Async , handled review ocmments * Donot do buffer the response using BufferedHttpEntity since it might cause memory issue, this behaviour is same as Apache4.x * Fix compilation issues * Fix checkstyle issues * Remove test which are specific to apache http
1 parent ad30c25 commit 3cedfb1

File tree

4 files changed

+120
-23
lines changed

4 files changed

+120
-23
lines changed

‎http-clients/apache-client/src/test/java/software/amazon/awssdk/http/apache/ApacheHttpClientWireMockTest.java

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import static com.github.tomakehurst.wiremock.client.WireMock.any;
2020
import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo;
2121
import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
22+
import static org.assertj.core.api.Assertions.assertThat;
2223
import static org.mockito.Mockito.verify;
2324
import static org.mockito.Mockito.when;
2425
import static software.amazon.awssdk.http.SdkHttpConfigurationOption.TRUST_ALL_CERTIFICATES;
@@ -46,13 +47,15 @@
4647
import org.mockito.Mock;
4748
import org.mockito.junit.MockitoJUnitRunner;
4849
import software.amazon.awssdk.http.HttpExecuteRequest;
50+
import software.amazon.awssdk.http.HttpExecuteResponse;
4951
import software.amazon.awssdk.http.SdkHttpClient;
5052
import software.amazon.awssdk.http.SdkHttpClientTestSuite;
5153
import software.amazon.awssdk.http.SdkHttpFullRequest;
5254
import software.amazon.awssdk.http.SdkHttpMethod;
5355
import software.amazon.awssdk.http.apache.internal.ApacheHttpRequestConfig;
5456
import software.amazon.awssdk.http.apache.internal.impl.ConnectionManagerAwareHttpClient;
5557
import software.amazon.awssdk.utils.AttributeMap;
58+
import software.amazon.awssdk.utils.IoUtils;
5659

5760
@RunWith(MockitoJUnitRunner.class)
5861
public class ApacheHttpClientWireMockTest extends SdkHttpClientTestSuite {
@@ -179,6 +182,45 @@ public void explicitNullDnsResolver_WithLocalhost_successful() throws Exception
179182
overrideDnsResolver("localhost", true);
180183
}
181184

185+
@Test
186+
public void handlesVariousContentLengths() throws Exception {
187+
SdkHttpClient client = createSdkHttpClient();
188+
int[] contentLengths = {0, 1, 100, 1024, 65536};
189+
190+
for (int length : contentLengths) {
191+
String path = "/content-length-" + length;
192+
byte[] body = new byte[length];
193+
for (int i = 0; i < length; i++) {
194+
body[i] = (byte) ('A' + (i % 26));
195+
}
196+
197+
mockServer.stubFor(any(urlPathEqualTo(path))
198+
.willReturn(aResponse()
199+
.withStatus(200)
200+
.withHeader("Content-Length", String.valueOf(length))
201+
.withBody(body)));
202+
203+
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port() + path, SdkHttpMethod.GET);
204+
HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder()
205+
.request(req)
206+
.build())
207+
.call();
208+
209+
assertThat(rsp.httpResponse().statusCode()).isEqualTo(200);
210+
211+
if (length == 0) {
212+
// Empty body should still have a response body present, but EOF immediately
213+
if (rsp.responseBody().isPresent()) {
214+
assertThat(rsp.responseBody().get().read()).isEqualTo(-1);
215+
}
216+
} else {
217+
assertThat(rsp.responseBody()).isPresent();
218+
byte[] readBody = IoUtils.toByteArray(rsp.responseBody().get());
219+
assertThat(readBody).isEqualTo(body);
220+
}
221+
}
222+
}
223+
182224
private void overrideDnsResolver(String hostName) throws IOException {
183225
overrideDnsResolver(hostName, false);
184226
}

‎http-clients/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/Apache5HttpClient.java

Lines changed: 32 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import static software.amazon.awssdk.http.apache5.internal.conn.ClientConnectionRequestFactory.THREAD_LOCAL_REQUEST_METRIC_COLLECTOR;
2424
import static software.amazon.awssdk.utils.NumericUtils.saturatedCast;
2525

26+
import java.io.ByteArrayInputStream;
2627
import java.io.IOException;
2728
import java.net.InetAddress;
2829
import java.security.KeyManagementException;
@@ -56,10 +57,11 @@
5657
import org.apache.hc.client5.http.ssl.SSLConnectionSocketFactory;
5758
import org.apache.hc.core5.http.ClassicHttpResponse;
5859
import org.apache.hc.core5.http.Header;
60+
import org.apache.hc.core5.http.HttpEntity;
61+
import org.apache.hc.core5.http.HttpResponse;
5962
import org.apache.hc.core5.http.config.Registry;
6063
import org.apache.hc.core5.http.impl.io.HttpRequestExecutor;
6164
import org.apache.hc.core5.http.io.SocketConfig;
62-
import org.apache.hc.core5.http.io.entity.BufferedHttpEntity;
6365
import org.apache.hc.core5.io.CloseMode;
6466
import org.apache.hc.core5.pool.PoolStats;
6567
import org.apache.hc.core5.ssl.SSLInitializationException;
@@ -262,20 +264,14 @@ private HttpExecuteResponse execute(HttpUriRequestBase apacheRequest, MetricColl
262264
HttpClientContext localRequestContext = Apache5Utils.newClientContext(requestConfig.proxyConfiguration());
263265
THREAD_LOCAL_REQUEST_METRIC_COLLECTOR.set(metricCollector);
264266
try {
265-
return httpClient.execute(apacheRequest, localRequestContext, response -> {
266-
267-
// TODO : This is required since Apache5 closes streams immediately, check memory impacts because of this.
268-
if (response.getEntity() != null) {
269-
response.setEntity(new BufferedHttpEntity(response.getEntity()));
270-
}
271-
return createResponse(response, apacheRequest);
272-
});
267+
HttpResponse httpResponse = httpClient.execute(apacheRequest, localRequestContext);
268+
// Create a connection-aware input stream that closes the response when closed
269+
return createResponse(httpResponse, apacheRequest);
273270
} finally {
274271
THREAD_LOCAL_REQUEST_METRIC_COLLECTOR.remove();
275272
}
276273
}
277274

278-
279275
private HttpUriRequestBase toApacheRequest(HttpExecuteRequest request) {
280276
return apacheHttpRequestFactory.create(request, requestConfig);
281277
}
@@ -288,7 +284,7 @@ private HttpUriRequestBase toApacheRequest(HttpExecuteRequest request) {
288284
* @throws IOException If there were any problems getting any response information from the
289285
* HttpClient method object.
290286
*/
291-
private HttpExecuteResponse createResponse(ClassicHttpResponse apacheHttpResponse,
287+
private HttpExecuteResponse createResponse(HttpResponse apacheHttpResponse,
292288
HttpUriRequestBase apacheRequest) throws IOException {
293289
SdkHttpResponse.Builder responseBuilder =
294290
SdkHttpResponse.builder()
@@ -302,17 +298,36 @@ private HttpExecuteResponse createResponse(ClassicHttpResponse apacheHttpRespons
302298
responseBuilder.appendHeader(header.getName(), header.getValue());
303299

304300
}
305-
306-
AbortableInputStream responseBody = apacheHttpResponse.getEntity() != null ?
307-
toAbortableInputStream(apacheHttpResponse, apacheRequest) : null;
308-
301+
AbortableInputStream responseBody = getResponseBody(apacheHttpResponse, apacheRequest);
309302
return HttpExecuteResponse.builder().response(responseBuilder.build()).responseBody(responseBody).build();
310303

311304
}
312305

313-
private AbortableInputStream toAbortableInputStream(ClassicHttpResponse apacheHttpResponse,
306+
private AbortableInputStream getResponseBody(HttpResponse apacheHttpResponse,
307+
HttpUriRequestBase apacheRequest) throws IOException {
308+
AbortableInputStream responseBody = null;
309+
if (apacheHttpResponse instanceof ClassicHttpResponse) {
310+
ClassicHttpResponse classicResponse = (ClassicHttpResponse) apacheHttpResponse;
311+
HttpEntity entity = classicResponse.getEntity();
312+
if (entity != null) {
313+
if (entity.getContentLength() == 0) {
314+
// Close immediately for empty responses
315+
classicResponse.close();
316+
responseBody = AbortableInputStream.create(new ByteArrayInputStream(new byte[0]));
317+
} else {
318+
responseBody = toAbortableInputStream(classicResponse, apacheRequest);
319+
}
320+
} else {
321+
// No entity, close the response immediately
322+
classicResponse.close();
323+
}
324+
}
325+
return responseBody;
326+
}
327+
328+
private AbortableInputStream toAbortableInputStream(ClassicHttpResponse apacheResponse,
314329
HttpUriRequestBase apacheRequest) throws IOException {
315-
return AbortableInputStream.create(apacheHttpResponse.getEntity().getContent(), apacheRequest::abort);
330+
return AbortableInputStream.create(apacheResponse.getEntity().getContent(), apacheRequest::abort);
316331
}
317332

318333
private Apache5HttpRequestConfig createRequestConfig(DefaultBuilder builder,

‎http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/Apache5HttpClientWireMockTest.java

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@
1919
import static com.github.tomakehurst.wiremock.client.WireMock.any;
2020
import static com.github.tomakehurst.wiremock.client.WireMock.urlPathEqualTo;
2121
import static com.github.tomakehurst.wiremock.core.WireMockConfiguration.wireMockConfig;
22+
import static org.assertj.core.api.Assertions.assertThat;
2223
import static org.mockito.Mockito.verify;
2324
import static org.mockito.Mockito.when;
2425
import static software.amazon.awssdk.http.SdkHttpConfigurationOption.TRUST_ALL_CERTIFICATES;
@@ -47,13 +48,15 @@
4748
import org.mockito.Mock;
4849
import org.mockito.junit.MockitoJUnitRunner;
4950
import software.amazon.awssdk.http.HttpExecuteRequest;
51+
import software.amazon.awssdk.http.HttpExecuteResponse;
5052
import software.amazon.awssdk.http.SdkHttpClient;
5153
import software.amazon.awssdk.http.SdkHttpClientTestSuite;
5254
import software.amazon.awssdk.http.SdkHttpFullRequest;
5355
import software.amazon.awssdk.http.SdkHttpMethod;
5456
import software.amazon.awssdk.http.apache5.internal.Apache5HttpRequestConfig;
5557
import software.amazon.awssdk.http.apache5.internal.impl.ConnectionManagerAwareHttpClient;
5658
import software.amazon.awssdk.utils.AttributeMap;
59+
import software.amazon.awssdk.utils.IoUtils;
5760

5861
@RunWith(MockitoJUnitRunner.class)
5962
public class Apache5HttpClientWireMockTest extends SdkHttpClientTestSuite {
@@ -173,6 +176,47 @@ public void explicitNullDnsResolver_WithLocalhost_successful() throws Exception
173176
overrideDnsResolver("localhost", true);
174177
}
175178

179+
180+
181+
@Test
182+
public void handlesVariousContentLengths() throws Exception {
183+
SdkHttpClient client = createSdkHttpClient();
184+
int[] contentLengths = {0, 1, 100, 1024, 65536};
185+
186+
for (int length : contentLengths) {
187+
String path = "/content-length-" + length;
188+
byte[] body = new byte[length];
189+
for (int i = 0; i < length; i++) {
190+
body[i] = (byte) ('A' + (i % 26));
191+
}
192+
193+
mockServer.stubFor(any(urlPathEqualTo(path))
194+
.willReturn(aResponse()
195+
.withStatus(200)
196+
.withHeader("Content-Length", String.valueOf(length))
197+
.withBody(body)));
198+
199+
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port() + path, SdkHttpMethod.GET);
200+
HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder()
201+
.request(req)
202+
.build())
203+
.call();
204+
205+
assertThat(rsp.httpResponse().statusCode()).isEqualTo(200);
206+
207+
if (length == 0) {
208+
// Empty body should still have a response body present, but EOF immediately
209+
if (rsp.responseBody().isPresent()) {
210+
assertThat(rsp.responseBody().get().read()).isEqualTo(-1);
211+
}
212+
} else {
213+
assertThat(rsp.responseBody()).isPresent();
214+
byte[] readBody = IoUtils.toByteArray(rsp.responseBody().get());
215+
assertThat(readBody).isEqualTo(body);
216+
}
217+
}
218+
}
219+
176220
private void overrideDnsResolver(String hostName) throws IOException {
177221
overrideDnsResolver(hostName, false);
178222
}

‎http-clients/apache5-client/src/test/java/software/amazon/awssdk/http/apache5/MetricReportingTest.java

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,12 +64,8 @@ public class MetricReportingTest {
6464
@Before
6565
public void methodSetup() throws IOException {
6666

67-
ClassicHttpResponse httpResponse = new BasicClassicHttpResponse(200, "OK");
68-
when(mockHttpClient.execute(any(HttpUriRequest.class), any(HttpContext.class), any(HttpClientResponseHandler.class)))
69-
.thenAnswer(invocation -> {
70-
HttpClientResponseHandler<HttpExecuteResponse> handler = invocation.getArgument(2);
71-
return handler.handleResponse(httpResponse);
72-
});
67+
when(mockHttpClient.execute(any(HttpUriRequest.class), any(HttpContext.class)))
68+
.thenReturn(new BasicClassicHttpResponse(200, "OK"));
7369

7470
when(mockHttpClient.getHttpClientConnectionManager()).thenReturn(cm);
7571

0 commit comments

Comments
 (0)