Skip to content

Make sure CRT resources are closed in tests #4753

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 1 commit into from
Dec 8, 2023
Merged

Make sure CRT resources are closed in tests #4753

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions ...c/test/java/software/amazon/awssdk/http/crt/AwsCrtAsyncHttpClientSpiVerificationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
import org.reactivestreams.Subscriber;
import org.reactivestreams.Subscription;
import software.amazon.awssdk.crt.CrtResource;
import software.amazon.awssdk.crt.Log;
import software.amazon.awssdk.crt.http.HttpException;
import software.amazon.awssdk.http.RecordingResponseHandler;
import software.amazon.awssdk.http.SdkHttpMethod;
Expand All @@ -73,6 +74,8 @@ public class AwsCrtAsyncHttpClientSpiVerificationTest {

@BeforeClass
public static void setup() throws Exception {
System.setProperty("aws.crt.debugnative", "true");
Log.initLoggingToStdout(Log.LogLevel.Warn);
client = AwsCrtAsyncHttpClient.builder()
.connectionHealthConfiguration(b -> b.minimumThroughputInBps(4068L)
.minimumThroughputTimeout(Duration.ofSeconds(3)))
Expand Down
3 changes: 3 additions & 0 deletions ...nt/src/test/java/software/amazon/awssdk/http/crt/AwsCrtHttpClientSpiVerificationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import org.junit.Rule;
import org.junit.Test;
import software.amazon.awssdk.crt.CrtResource;
import software.amazon.awssdk.crt.Log;
import software.amazon.awssdk.crt.http.HttpException;
import software.amazon.awssdk.http.ExecutableHttpRequest;
import software.amazon.awssdk.http.HttpExecuteRequest;
Expand All @@ -63,6 +64,8 @@ public class AwsCrtHttpClientSpiVerificationTest {

@BeforeClass
public static void setup() throws Exception {
System.setProperty("aws.crt.debugnative", "true");
Log.initLoggingToStdout(Log.LogLevel.Warn);
client = AwsCrtHttpClient.builder()
.connectionHealthConfiguration(b -> b.minimumThroughputInBps(4068L)
.minimumThroughputTimeout(Duration.ofSeconds(3)))
Expand Down
118 changes: 60 additions & 58 deletions test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientTestSuite.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,12 +109,12 @@ public void supportsResponseCode500() throws Exception {

@Test
public void validatesHttpsCertificateIssuer() {
SdkHttpClient client = createSdkHttpClient();
try (SdkHttpClient client = createSdkHttpClient()) {
SdkHttpFullRequest request = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), SdkHttpMethod.POST);

SdkHttpFullRequest request = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), SdkHttpMethod.POST);

assertThatThrownBy(client.prepareRequest(HttpExecuteRequest.builder().request(request).build())::call)
assertThatThrownBy(client.prepareRequest(HttpExecuteRequest.builder().request(request).build())::call)
.isInstanceOf(SSLHandshakeException.class);
}
}

@Test
Expand All @@ -123,27 +123,26 @@ public void connectionPoolingWorks() throws Exception {

SdkHttpClientOptions httpClientOptions = new SdkHttpClientOptions();
httpClientOptions.trustAll(true);
SdkHttpClient client = createSdkHttpClient(httpClientOptions);

stubForMockRequest(200);

for (int i = 0; i < 5; i++) {
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), SdkHttpMethod.POST);
HttpExecuteResponse response =
client.prepareRequest(HttpExecuteRequest.builder()
.request(req)
.contentStreamProvider(req.contentStreamProvider().orElse(null))
.build())
.call();
response.responseBody().ifPresent(IoUtils::drainInputStream);
}

// connection pool growth strategies vary across client implementations. Some, such as the CRT grow connection counts
// by a factor of 2, while some grow strictly as requested. Mainly we want to test that it kicks in at some point and
// doesn't create a new connection for all 5 requests. This proves that while allowing variance in this behavior.
assertThat(CONNECTION_COUNTER.openedConnections()).isGreaterThanOrEqualTo(initialOpenedConnections + 1);
assertThat(CONNECTION_COUNTER.openedConnections()).isLessThanOrEqualTo(initialOpenedConnections + 2);
try (SdkHttpClient client = createSdkHttpClient(httpClientOptions)) {
stubForMockRequest(200);

for (int i = 0; i < 5; i++) {
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), SdkHttpMethod.POST);
HttpExecuteResponse response =
client.prepareRequest(HttpExecuteRequest.builder()
.request(req)
.contentStreamProvider(req.contentStreamProvider().orElse(null))
.build())
.call();
response.responseBody().ifPresent(IoUtils::drainInputStream);
}

// connection pool growth strategies vary across client implementations. Some, such as the CRT grow connection counts
// by a factor of 2, while some grow strictly as requested. Mainly we want to test that it kicks in at some point and
// doesn't create a new connection for all 5 requests. This proves that while allowing variance in this behavior.
assertThat(CONNECTION_COUNTER.openedConnections()).isGreaterThanOrEqualTo(initialOpenedConnections + 1);
assertThat(CONNECTION_COUNTER.openedConnections()).isLessThanOrEqualTo(initialOpenedConnections + 2);
}
}

@Test
Expand All @@ -152,25 +151,26 @@ public void connectionsAreNotReusedOn5xxErrors() throws Exception {

SdkHttpClientOptions httpClientOptions = new SdkHttpClientOptions();
httpClientOptions.trustAll(true);
SdkHttpClient client = createSdkHttpClient(httpClientOptions);

stubForMockRequest(503);

for (int i = 0; i < 5; i++) {
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), SdkHttpMethod.POST);
HttpExecuteResponse response =
client.prepareRequest(HttpExecuteRequest.builder()
.request(req)
.contentStreamProvider(req.contentStreamProvider().orElse(null))
.build())
.call();
response.responseBody().ifPresent(IoUtils::drainInputStream);
}
try (SdkHttpClient client = createSdkHttpClient(httpClientOptions)) {
stubForMockRequest(503);

for (int i = 0; i < 5; i++) {
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), SdkHttpMethod.POST);
HttpExecuteResponse response =
client.prepareRequest(HttpExecuteRequest.builder()
.request(req)
.contentStreamProvider(req.contentStreamProvider().orElse(null))
.build())
.call();
response.responseBody().ifPresent(IoUtils::drainInputStream);
}

// don't couple this test to connection manager behaviors we don't have to. We want to make sure that the connection count
// increased by at least as many connections as we got 5xx errors back on. But the connection manager also predictively
// creates connections and we need to take those into account in a way that lets it remain a dynamic behavior.
assertThat(CONNECTION_COUNTER.openedConnections()).isGreaterThanOrEqualTo(initialOpenedConnections + 5);
// don't couple this test to connection manager behaviors we don't have to. We want to make sure that the
// connection count increased by at least as many connections as we got 5xx errors back on. But the connection
// manager also predictively creates connections and we need to take those into account in a way that lets it
// remain a dynamic behavior.
assertThat(CONNECTION_COUNTER.openedConnections()).isGreaterThanOrEqualTo(initialOpenedConnections + 5);
}
}

@Test
Expand All @@ -185,8 +185,7 @@ public void testCustomTlsTrustManager() throws Exception {

selfSignedServer.start();

try {
SdkHttpClient client = createSdkHttpClient(httpClientOptions);
try (SdkHttpClient client = createSdkHttpClient(httpClientOptions)) {
SdkHttpFullRequest request = mockSdkRequest("https://localhost:" + selfSignedServer.httpsPort(), SdkHttpMethod.POST);

client.prepareRequest(HttpExecuteRequest.builder()
Expand All @@ -204,7 +203,10 @@ public void testTrustAllWorks() throws Exception {
SdkHttpClientOptions httpClientOptions = new SdkHttpClientOptions();
httpClientOptions.trustAll(true);

testForResponseCodeUsingHttps(createSdkHttpClient(httpClientOptions), HttpURLConnection.HTTP_OK);
try (SdkHttpClient client = createSdkHttpClient(httpClientOptions)) {
testForResponseCodeUsingHttps(client, HttpURLConnection.HTTP_OK);
}

}

@Test
Expand All @@ -221,19 +223,19 @@ protected void testForResponseCode(int returnCode) throws Exception {
}

private void testForResponseCode(int returnCode, SdkHttpMethod method) throws Exception {
SdkHttpClient client = createSdkHttpClient();

stubForMockRequest(returnCode);

SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), method);
HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder()
.request(req)
.contentStreamProvider(req.contentStreamProvider()
.orElse(null))
.build())
.call();

validateResponse(rsp, returnCode, method);
try (SdkHttpClient client = createSdkHttpClient()) {
stubForMockRequest(returnCode);

SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), method);
HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder()
.request(req)
.contentStreamProvider(req.contentStreamProvider()
.orElse(null))
.build())
.call();

validateResponse(rsp, returnCode, method);
}
}

protected void testForResponseCodeUsingHttps(SdkHttpClient client, int returnCode) throws Exception {
Expand Down