Fix request cancellation logic in the AWS CRT Sync HTTP client (#4887)

* Fix request cancellation logic in the AWS CRT Sync HTTP client

* Address feedback

Author: zoewangg

.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)"
+}

http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/AwsCrtHttpClient.java

@@ -129,14 +129,18 @@ public HttpExecuteResponse call() throws IOException {
                     throw (HttpException) cause;
                 }
 
+                if (cause instanceof InterruptedException) {
+                    Thread.currentThread().interrupt();
+                    throw new IOException("Request was cancelled", cause);
+                }
                 throw new RuntimeException(e.getCause());
             }
         }
 
         @Override
         public void abort() {
             if (responseFuture != null) {
-                responseFuture.cancel(true);
+                responseFuture.completeExceptionally(new IOException("Request ws cancelled"));
             }
         }
     }

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.
      *

test/protocol-tests/pom.xml

@@ -219,6 +219,12 @@
             <artifactId>rxjava</artifactId>
             <scope>test</scope>
         </dependency>
+        <dependency>
+            <groupId>software.amazon.awssdk</groupId>
+            <artifactId>aws-crt-client</artifactId>
+            <version>${awsjavasdk.version}</version>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
     <build>

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);
+        }
+    }
+}