-
Notifications
You must be signed in to change notification settings - Fork 1k
Enable default connection health monitoring for CRT HTTP clients #6818
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| { | ||
| "type": "bugfix", | ||
| "category": "AWS CRT HTTP Client", | ||
| "contributor": "", | ||
| "description": "Enabled default connection health monitoring for the AWS CRT HTTP client. Connections that remain stalled below 1 byte per second for the duration the read/write timeout (default 30 seconds) are now automatically terminated. This behavior can be overridden via ConnectionHealthConfiguration." | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| /* | ||
| * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"). | ||
| * You may not use this file except in compliance with the License. | ||
| * A copy of the License is located at | ||
| * | ||
| * http://aws.amazon.com/apache2.0 | ||
| * | ||
| * or in the "license" file accompanying this file. This file is distributed | ||
| * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either | ||
| * express or implied. See the License for the specific language governing | ||
| * permissions and limitations under the License. | ||
| */ | ||
|
|
||
| package software.amazon.awssdk.http.crt; | ||
|
|
||
| import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
| import static software.amazon.awssdk.http.crt.CrtHttpClientTestUtils.createRequest; | ||
|
|
||
| import java.io.ByteArrayInputStream; | ||
| import java.io.IOException; | ||
| import java.net.ServerSocket; | ||
| import java.net.Socket; | ||
| import java.net.URI; | ||
| import java.time.Duration; | ||
| import java.util.concurrent.CompletionException; | ||
|
Check warning on line 27 in http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/NonResponsiveServerTest.java
|
||
| import java.util.concurrent.ExecutionException; | ||
|
Check warning on line 28 in http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/NonResponsiveServerTest.java
|
||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.concurrent.TimeoutException; | ||
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import software.amazon.awssdk.crt.Log; | ||
| import software.amazon.awssdk.http.ExecutableHttpRequest; | ||
| import software.amazon.awssdk.http.HttpExecuteRequest; | ||
| import software.amazon.awssdk.http.RecordingResponseHandler; | ||
| import software.amazon.awssdk.http.SdkHttpConfigurationOption; | ||
| import software.amazon.awssdk.http.SdkHttpRequest; | ||
| import software.amazon.awssdk.http.async.AsyncExecuteRequest; | ||
| import software.amazon.awssdk.http.async.SdkAsyncHttpClient; | ||
| import software.amazon.awssdk.http.SdkHttpClient; | ||
| import software.amazon.awssdk.utils.AttributeMap; | ||
|
|
||
| /** | ||
| * Functional tests verifying that the default connection health configuration | ||
| * (applied when no explicit {@link ConnectionHealthConfiguration} is set) | ||
| * correctly terminates connections to non-responding servers. | ||
| */ | ||
| class NonResponsiveServerTest { | ||
|
|
||
| private static final Duration SHORT_TIMEOUT = Duration.ofSeconds(2); | ||
| private static final AttributeMap SHORT_TIMEOUTS = AttributeMap.builder() | ||
| .put(SdkHttpConfigurationOption.READ_TIMEOUT, SHORT_TIMEOUT) | ||
| .put(SdkHttpConfigurationOption.WRITE_TIMEOUT, SHORT_TIMEOUT) | ||
| .build(); | ||
|
|
||
| private ServerSocket serverSocket; | ||
|
|
||
| @BeforeEach | ||
| void setUp() throws IOException { | ||
| Log.initLoggingToStdout(Log.LogLevel.Warn); | ||
| serverSocket = new ServerSocket(0); | ||
| // Accept connections in a daemon thread but never respond | ||
| Thread acceptThread = new Thread(() -> { | ||
| while (!serverSocket.isClosed()) { | ||
| try { | ||
| Socket socket = serverSocket.accept(); | ||
|
Check warning on line 68 in http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/NonResponsiveServerTest.java
|
||
| // Hold the connection open, never send a response | ||
| Thread.sleep(Long.MAX_VALUE); | ||
|
Check warning on line 70 in http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/NonResponsiveServerTest.java
|
||
| } catch (Exception e) { | ||
| // Server shutting down | ||
| } | ||
| } | ||
| }); | ||
| acceptThread.setDaemon(true); | ||
| acceptThread.start(); | ||
| } | ||
|
|
||
| @AfterEach | ||
| void tearDown() throws IOException { | ||
| if (serverSocket != null && !serverSocket.isClosed()) { | ||
| serverSocket.close(); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| void syncClient_noExplicitHealthConfig_serverNeverResponds_shouldThrow() { | ||
| try (SdkHttpClient client = AwsCrtHttpClient.builder().buildWithDefaults(SHORT_TIMEOUTS)) { | ||
| URI uri = URI.create("http://localhost:" + serverSocket.getLocalPort()); | ||
| SdkHttpRequest request = createRequest(uri); | ||
| ExecutableHttpRequest executableRequest = client.prepareRequest( | ||
| HttpExecuteRequest.builder().request(request) | ||
| .contentStreamProvider(() -> new ByteArrayInputStream(new byte[0])) | ||
| .build()); | ||
| assertThatThrownBy(executableRequest::call).isInstanceOf(IOException.class) | ||
| .hasMessageContaining("failure to meet throughput minimum"); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| void asyncClient_noExplicitHealthConfig_serverNeverResponds_shouldCompleteExceptionally() | ||
| throws InterruptedException, TimeoutException { | ||
|
Check warning on line 103 in http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/NonResponsiveServerTest.java
|
||
| try (SdkAsyncHttpClient client = AwsCrtAsyncHttpClient.builder().buildWithDefaults(SHORT_TIMEOUTS)) { | ||
| URI uri = URI.create("http://localhost:" + serverSocket.getLocalPort()); | ||
| SdkHttpRequest request = createRequest(uri); | ||
| RecordingResponseHandler recorder = new RecordingResponseHandler(); | ||
|
|
||
| client.execute(AsyncExecuteRequest.builder() | ||
| .request(request) | ||
| .requestContentPublisher(new EmptyPublisher()) | ||
| .responseHandler(recorder) | ||
| .build()); | ||
|
|
||
| assertThatThrownBy(() -> recorder.completeFuture().get(10, TimeUnit.SECONDS)) | ||
| .hasCauseInstanceOf(IOException.class) | ||
| .hasMessageContaining("failure to meet throughput minimum"); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly worried about this configuration - but I also don't understand the details of how this is measured and applied. I guess what I'm worried about is cases where healthy connections for say, a response with only a few bytes might round down to 0 and be < 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! Kiro didn't think it's a risk (response below). 😛 cc @TingDaoK to help confirm.
Traced through the CRT C code to understand exactly how this works. The short answer is:
minThroughputBytesPerSecond=1is safe and won't kill healthy connections with small responses.How throughput is measured
The monitor runs every 1 second. It doesn't simply divide bytes by 1 second — it divides bytes by the time a stream was actually active during that interval (throughput calculation):
So if a 5-byte response completes in 50ms, throughput =
5 * 1000 / 50 = 100 bytes/sec. Even the worst case — 1 byte over a full 1000ms — gives exactly 1byte/sec, which still passes the threshold.
When throughput is checked
There are also guards that prevent false positives:
skipped (first tick → no previous ID match).
was_inactive == false). If the last stream completes mid-interval, the check is skipped.So for a small/fast response, either:
was_inactive=truefor H2)pending_msis proportionally smallThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! In that case I think the 1 byte setting makes sense :-)