Skip to content

Commit 5c5ce0d

Browse files
zoewanggzoewangg
committed
Make sure CRT resources are closed
1 parent 92b9b2a commit 5c5ce0d

File tree

3 files changed

+66
-58
lines changed

3 files changed

+66
-58
lines changed

‎http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/AwsCrtAsyncHttpClientSpiVerificationTest.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
import org.reactivestreams.Subscriber;
5353
import org.reactivestreams.Subscription;
5454
import software.amazon.awssdk.crt.CrtResource;
55+
import software.amazon.awssdk.crt.Log;
5556
import software.amazon.awssdk.crt.http.HttpException;
5657
import software.amazon.awssdk.http.RecordingResponseHandler;
5758
import software.amazon.awssdk.http.SdkHttpMethod;
@@ -73,6 +74,8 @@ public class AwsCrtAsyncHttpClientSpiVerificationTest {
7374

7475
@BeforeClass
7576
public static void setup() throws Exception {
77+
System.setProperty("aws.crt.debugnative", "true");
78+
Log.initLoggingToStdout(Log.LogLevel.Warn);
7679
client = AwsCrtAsyncHttpClient.builder()
7780
.connectionHealthConfiguration(b -> b.minimumThroughputInBps(4068L)
7881
.minimumThroughputTimeout(Duration.ofSeconds(3)))

‎http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/AwsCrtHttpClientSpiVerificationTest.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import org.junit.Rule;
4444
import org.junit.Test;
4545
import software.amazon.awssdk.crt.CrtResource;
46+
import software.amazon.awssdk.crt.Log;
4647
import software.amazon.awssdk.crt.http.HttpException;
4748
import software.amazon.awssdk.http.ExecutableHttpRequest;
4849
import software.amazon.awssdk.http.HttpExecuteRequest;
@@ -63,6 +64,8 @@ public class AwsCrtHttpClientSpiVerificationTest {
6364

6465
@BeforeClass
6566
public static void setup() throws Exception {
67+
System.setProperty("aws.crt.debugnative", "true");
68+
Log.initLoggingToStdout(Log.LogLevel.Warn);
6669
client = AwsCrtHttpClient.builder()
6770
.connectionHealthConfiguration(b -> b.minimumThroughputInBps(4068L)
6871
.minimumThroughputTimeout(Duration.ofSeconds(3)))

‎test/http-client-tests/src/main/java/software/amazon/awssdk/http/SdkHttpClientTestSuite.java

Lines changed: 60 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -109,12 +109,12 @@ public void supportsResponseCode500() throws Exception {
109109

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

114-
SdkHttpFullRequest request = mockSdkRequest("https://localhost:" + mockServer.httpsPort(), SdkHttpMethod.POST);
115-
116-
assertThatThrownBy(client.prepareRequest(HttpExecuteRequest.builder().request(request).build())::call)
115+
assertThatThrownBy(client.prepareRequest(HttpExecuteRequest.builder().request(request).build())::call)
117116
.isInstanceOf(SSLHandshakeException.class);
117+
}
118118
}
119119

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

124124
SdkHttpClientOptions httpClientOptions = new SdkHttpClientOptions();
125125
httpClientOptions.trustAll(true);
126-
SdkHttpClient client = createSdkHttpClient(httpClientOptions);
127-
128-
stubForMockRequest(200);
129-
130-
for (int i = 0; i < 5; i++) {
131-
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), SdkHttpMethod.POST);
132-
HttpExecuteResponse response =
133-
client.prepareRequest(HttpExecuteRequest.builder()
134-
.request(req)
135-
.contentStreamProvider(req.contentStreamProvider().orElse(null))
136-
.build())
137-
.call();
138-
response.responseBody().ifPresent(IoUtils::drainInputStream);
139-
}
140-
141-
// connection pool growth strategies vary across client implementations. Some, such as the CRT grow connection counts
142-
// by a factor of 2, while some grow strictly as requested. Mainly we want to test that it kicks in at some point and
143-
// doesn't create a new connection for all 5 requests. This proves that while allowing variance in this behavior.
144-
assertThat(CONNECTION_COUNTER.openedConnections()).isGreaterThanOrEqualTo(initialOpenedConnections + 1);
145-
assertThat(CONNECTION_COUNTER.openedConnections()).isLessThanOrEqualTo(initialOpenedConnections + 2);
126+
try (SdkHttpClient client = createSdkHttpClient(httpClientOptions)) {
127+
stubForMockRequest(200);
128+
129+
for (int i = 0; i < 5; i++) {
130+
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), SdkHttpMethod.POST);
131+
HttpExecuteResponse response =
132+
client.prepareRequest(HttpExecuteRequest.builder()
133+
.request(req)
134+
.contentStreamProvider(req.contentStreamProvider().orElse(null))
135+
.build())
136+
.call();
137+
response.responseBody().ifPresent(IoUtils::drainInputStream);
138+
}
146139

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

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

153152
SdkHttpClientOptions httpClientOptions = new SdkHttpClientOptions();
154153
httpClientOptions.trustAll(true);
155-
SdkHttpClient client = createSdkHttpClient(httpClientOptions);
156-
157-
stubForMockRequest(503);
158-
159-
for (int i = 0; i < 5; i++) {
160-
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), SdkHttpMethod.POST);
161-
HttpExecuteResponse response =
162-
client.prepareRequest(HttpExecuteRequest.builder()
163-
.request(req)
164-
.contentStreamProvider(req.contentStreamProvider().orElse(null))
165-
.build())
166-
.call();
167-
response.responseBody().ifPresent(IoUtils::drainInputStream);
168-
}
154+
try (SdkHttpClient client = createSdkHttpClient(httpClientOptions)) {
155+
stubForMockRequest(503);
156+
157+
for (int i = 0; i < 5; i++) {
158+
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), SdkHttpMethod.POST);
159+
HttpExecuteResponse response =
160+
client.prepareRequest(HttpExecuteRequest.builder()
161+
.request(req)
162+
.contentStreamProvider(req.contentStreamProvider().orElse(null))
163+
.build())
164+
.call();
165+
response.responseBody().ifPresent(IoUtils::drainInputStream);
166+
}
169167

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

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

186186
selfSignedServer.start();
187187

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

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

207-
testForResponseCodeUsingHttps(createSdkHttpClient(httpClientOptions), HttpURLConnection.HTTP_OK);
206+
try (SdkHttpClient client = createSdkHttpClient(httpClientOptions)) {
207+
testForResponseCodeUsingHttps(client, HttpURLConnection.HTTP_OK);
208+
}
209+
208210
}
209211

210212
@Test
@@ -221,19 +223,19 @@ protected void testForResponseCode(int returnCode) throws Exception {
221223
}
222224

223225
private void testForResponseCode(int returnCode, SdkHttpMethod method) throws Exception {
224-
SdkHttpClient client = createSdkHttpClient();
225-
226-
stubForMockRequest(returnCode);
227-
228-
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), method);
229-
HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder()
230-
.request(req)
231-
.contentStreamProvider(req.contentStreamProvider()
232-
.orElse(null))
233-
.build())
234-
.call();
235-
236-
validateResponse(rsp, returnCode, method);
226+
try (SdkHttpClient client = createSdkHttpClient()) {
227+
stubForMockRequest(returnCode);
228+
229+
SdkHttpFullRequest req = mockSdkRequest("http://localhost:" + mockServer.port(), method);
230+
HttpExecuteResponse rsp = client.prepareRequest(HttpExecuteRequest.builder()
231+
.request(req)
232+
.contentStreamProvider(req.contentStreamProvider()
233+
.orElse(null))
234+
.build())
235+
.call();
236+
237+
validateResponse(rsp, returnCode, method);
238+
}
237239
}
238240

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

0 commit comments

Comments
 (0)