Skip to content

Do not buffer the Response stream using BufferedHttpEntity #6200

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

Merged
merged 20 commits into from
Jun 24, 2025
Merged

Do not buffer the Response stream using BufferedHttpEntity #6200

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9e8fc15
Fix architecture test failures for apache5.x
joviegas May 29, 2025
70a120e
Merge branch 'feature/master/apache5x' into joviegas/apache-5-achitec…
joviegas May 29, 2025
c7b09a2
Merge issues resolved
joviegas May 29, 2025
068cada
Checkstyle issues
joviegas May 29, 2025
4766e74
Update to use PoolingHttpClientConnectionManager class reference that…
joviegas May 30, 2025
c81efdc
Merge branch 'master' into joviegas/apache-5-achitecture-test-fix
joviegas May 30, 2025
6b64589
Merge branch 'feature/master/apache5x' into joviegas/apache-5-achitec…
joviegas May 30, 2025
89debeb
Merge branch 'feature/master/apache5x' into joviegas/apache-5-achitec…
joviegas Jun 2, 2025
7f9e77f
Fix stream reset failure in RepeatableInputStreamRequestEntity by sto…
joviegas Jun 3, 2025
5fb2d35
writeTo_ConcurrentWrites_HandlesCorrectly no longer needed since even…
joviegas Jun 3, 2025
52fedbd
Merge branch 'feature/master/apache5x' into joviegas/apache-5-achitec…
joviegas Jun 9, 2025
8b83790
Fix connectionPoolingWorks by setting skipping setConnectionTimeToLi…
joviegas Jun 9, 2025
14a2ead
disableAutomaticRetries in Apache 5.x since SDK handles retries , als…
joviegas Jun 17, 2025
9431959
Merge branch 'feature/master/apache5x' into joviegas/apache-5-achitec…
joviegas Jun 17, 2025
5f33ad9
Added Test case for Async , handled review ocmments
joviegas Jun 19, 2025
270c0f7
Donot do buffer the response using BufferedHttpEntity since it might …
joviegas Jun 21, 2025
1a18785
merge from master
joviegas Jun 21, 2025
e2d16ad
Fix compilation issues
joviegas Jun 21, 2025
85efdaf
Fix checkstyle issues
joviegas Jun 21, 2025
bb70f7d
Remove test which are specific to apache http
joviegas Jun 21, 2025
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
42 changes: 42 additions & 0 deletions ...client/src/test/java/software/amazon/awssdk/http/apache/ApacheHttpClientWireMockTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -46,13 +47,15 @@
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;
import software.amazon.awssdk.http.SdkHttpMethod;
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 {
Expand Down Expand Up @@ -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};
Comment on lines +185 to +188
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we don't use parameterized test instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test extends SdkHttpClientTestSuite which is based on Junit 4


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);
}
Expand Down
49 changes: 32 additions & 17 deletions ...s/apache5-client/src/main/java/software/amazon/awssdk/http/apache5/Apache5HttpClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
Expand All @@ -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()
Expand All @@ -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,
Expand Down
44 changes: 44 additions & 0 deletions ...ient/src/test/java/software/amazon/awssdk/http/apache5/Apache5HttpClientWireMockTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -47,13 +48,15 @@
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;
import software.amazon.awssdk.http.SdkHttpMethod;
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 {
Expand Down Expand Up @@ -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);
}
Expand Down
8 changes: 2 additions & 6 deletions ...apache5-client/src/test/java/software/amazon/awssdk/http/apache5/MetricReportingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<HttpExecuteResponse> 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);

Expand Down
Loading