From 5c869c12027c6ef48c1c6ccbb469b5be885eed7a Mon Sep 17 00:00:00 2001 From: Zoe Wang <33073555+zoewangg@users.noreply.github.com> Date: Fri, 2 Feb 2024 16:54:08 -0800 Subject: [PATCH 1/2] Fix request cancellation logic in the AWS CRT Sync HTTP client --- .../bugfix-AWSCRTSyncHTTPClient-660cc21.json | 6 ++ .../awssdk/http/crt/AwsCrtHttpClient.java | 6 +- .../crt/AwsCrtHttpClientWireMockTest.java | 28 +++++++ test/protocol-tests/pom.xml | 6 ++ .../CrtHttpClientApiCallTimeoutTest.java | 73 +++++++++++++++++++ 5 files changed, 118 insertions(+), 1 deletion(-) create mode 100644 .changes/next-release/bugfix-AWSCRTSyncHTTPClient-660cc21.json create mode 100644 test/protocol-tests/src/test/java/software/amazon/awssdk/protocol/tests/timeout/CrtHttpClientApiCallTimeoutTest.java diff --git a/.changes/next-release/bugfix-AWSCRTSyncHTTPClient-660cc21.json b/.changes/next-release/bugfix-AWSCRTSyncHTTPClient-660cc21.json new file mode 100644 index 000000000000..6b6521056ecb --- /dev/null +++ b/.changes/next-release/bugfix-AWSCRTSyncHTTPClient-660cc21.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "AWS CRT Sync HTTP Client", + "contributor": "", + "description": "Fixed an issue where `CancellationException` was thrown incorrectly from AWS CRT Sync HTTP client when execution time exceeded the total configured API call attempt timeout or API call timeout. Now it throws `ApiCallAttemptTimeoutException`/`ApiCallTimeoutException` accordingly. See [#4820](https://github.com/aws/aws-sdk-java-v2/issues/4820)" +} diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtHttpClient.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtHttpClient.java index 3ee178abeb9a..7fe8aae70e58 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtHttpClient.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtHttpClient.java @@ -129,6 +129,10 @@ public HttpExecuteResponse call() throws IOException { throw (HttpException) cause; } + if (cause instanceof InterruptedException) { + Thread.currentThread().interrupt(); + throw new IOException("Request was cancelled"); + } throw new RuntimeException(e.getCause()); } } @@ -136,7 +140,7 @@ public HttpExecuteResponse call() throws IOException { @Override public void abort() { if (responseFuture != null) { - responseFuture.cancel(true); + responseFuture.completeExceptionally(new IOException("Request ws cancelled")); } } } diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/AwsCrtHttpClientWireMockTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/AwsCrtHttpClientWireMockTest.java index e052f225a4c9..47b0f2be5bdb 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/AwsCrtHttpClientWireMockTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/AwsCrtHttpClientWireMockTest.java @@ -28,7 +28,11 @@ import com.github.tomakehurst.wiremock.junit.WireMockRule; import java.io.ByteArrayInputStream; +import java.io.IOException; import java.net.URI; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Rule; @@ -51,16 +55,20 @@ public class AwsCrtHttpClientWireMockTest { public WireMockRule mockServer = new WireMockRule(wireMockConfig() .dynamicPort()); + private static ScheduledExecutorService executorService; + @BeforeClass public static void setup() { System.setProperty("aws.crt.debugnative", "true"); Log.initLoggingToStdout(Log.LogLevel.Warn); + executorService = Executors.newScheduledThreadPool(1); } @AfterClass public static void tearDown() { // Verify there is no resource leak. CrtResource.waitForNoResources(); + executorService.shutdown(); } @Test @@ -107,6 +115,26 @@ public void sharedEventLoopGroup_closeOneClient_shouldNotAffectOtherClients() th } } + @Test + public void abortRequest_shouldFailTheExceptionWithIOException() throws Exception { + try (SdkHttpClient client = AwsCrtHttpClient.create()) { + String body = randomAlphabetic(10); + URI uri = URI.create("http://localhost:" + mockServer.port()); + stubFor(any(urlPathEqualTo("/")).willReturn(aResponse().withFixedDelay(1000).withBody(body))); + SdkHttpRequest request = createRequest(uri); + + HttpExecuteRequest.Builder executeRequestBuilder = HttpExecuteRequest.builder(); + executeRequestBuilder.request(request) + .contentStreamProvider(() -> new ByteArrayInputStream(new byte[0])); + + ExecutableHttpRequest executableRequest = client.prepareRequest(executeRequestBuilder.build()); + executorService.schedule(() -> executableRequest.abort(), 100, TimeUnit.MILLISECONDS); + executableRequest.abort(); + assertThatThrownBy(() -> executableRequest.call()).isInstanceOf(IOException.class) + .hasMessageContaining("cancelled"); + } + } + /** * Make a simple request and wait for it to finish. * diff --git a/test/protocol-tests/pom.xml b/test/protocol-tests/pom.xml index e4756a837f86..a294c9d66bb4 100644 --- a/test/protocol-tests/pom.xml +++ b/test/protocol-tests/pom.xml @@ -219,6 +219,12 @@ rxjava test + + software.amazon.awssdk + aws-crt-client + ${awsjavasdk.version} + test + diff --git a/test/protocol-tests/src/test/java/software/amazon/awssdk/protocol/tests/timeout/CrtHttpClientApiCallTimeoutTest.java b/test/protocol-tests/src/test/java/software/amazon/awssdk/protocol/tests/timeout/CrtHttpClientApiCallTimeoutTest.java new file mode 100644 index 000000000000..847fc9be45ad --- /dev/null +++ b/test/protocol-tests/src/test/java/software/amazon/awssdk/protocol/tests/timeout/CrtHttpClientApiCallTimeoutTest.java @@ -0,0 +1,73 @@ +/* + * 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.protocol.tests.timeout; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.any; +import static com.github.tomakehurst.wiremock.client.WireMock.anyUrl; +import static com.github.tomakehurst.wiremock.client.WireMock.stubFor; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; +import com.github.tomakehurst.wiremock.junit5.WireMockTest; +import java.net.URI; +import java.time.Duration; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.core.exception.ApiCallAttemptTimeoutException; +import software.amazon.awssdk.core.exception.ApiCallTimeoutException; +import software.amazon.awssdk.http.crt.AwsCrtHttpClient; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.protocolrestjson.ProtocolRestJsonClient; +import software.amazon.awssdk.services.protocolrestjson.ProtocolRestJsonClientBuilder; + +@WireMockTest +public class CrtHttpClientApiCallTimeoutTest { + + private ProtocolRestJsonClientBuilder clientBuilder; + + @BeforeEach + public void setup(WireMockRuntimeInfo wiremock) { + clientBuilder = ProtocolRestJsonClient.builder() + .region(Region.US_WEST_1) + .endpointOverride(URI.create("http://localhost:" + wiremock.getHttpPort())) + .httpClientBuilder(AwsCrtHttpClient.builder()) + .credentialsProvider(() -> AwsBasicCredentials.create("akid", "skid")); + } + + @Test + void apiCallAttemptExceeded_shouldThrowApiCallAttemptTimeoutException() { + stubFor(any(anyUrl()).willReturn(aResponse().withStatus(200).withBody("{}").withFixedDelay(2000))); + try (ProtocolRestJsonClient client = + clientBuilder.overrideConfiguration(b -> b.apiCallAttemptTimeout(Duration.ofMillis(10))) + .build()) { + + + assertThatThrownBy(() -> client.allTypes()).isInstanceOf(ApiCallAttemptTimeoutException.class); + } + } + + @Test + void apiCallExceeded_shouldThrowApiCallAttemptTimeoutException() { + stubFor(any(anyUrl()).willReturn(aResponse().withStatus(200).withBody("{}").withFixedDelay(2000))); + try (ProtocolRestJsonClient client = clientBuilder.overrideConfiguration(b -> b.apiCallTimeout(Duration.ofMillis(10))) + .build()) { + + assertThatThrownBy(() -> client.allTypes()).isInstanceOf(ApiCallTimeoutException.class); + } + } +} From 612008312045ba4c36bae265f4b488084fadc64d Mon Sep 17 00:00:00 2001 From: Zoe Wang <33073555+zoewangg@users.noreply.github.com> Date: Tue, 6 Feb 2024 16:49:06 -0800 Subject: [PATCH 2/2] Address feedback --- .../java/software/amazon/awssdk/http/crt/AwsCrtHttpClient.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtHttpClient.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtHttpClient.java index 7fe8aae70e58..4db0cfcee8fb 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtHttpClient.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtHttpClient.java @@ -131,7 +131,7 @@ public HttpExecuteResponse call() throws IOException { if (cause instanceof InterruptedException) { Thread.currentThread().interrupt(); - throw new IOException("Request was cancelled"); + throw new IOException("Request was cancelled", cause); } throw new RuntimeException(e.getCause()); }