From 885192e61faacc078425889e5d64ea25725487cb Mon Sep 17 00:00:00 2001 From: John Viegas Date: Thu, 24 Dec 2020 16:16:06 -0800 Subject: [PATCH] Fix for Issue #1684 Changes in handleTimeoutCausedException in TimeoutExceptionHandlingStage --- .../bugfix-AWSSDKforJavav2-b5a2807.json | 6 ++ .../stages/TimeoutExceptionHandlingStage.java | 4 +- .../TimeoutExceptionHandlingStageTest.java | 28 +++++++ ...vaClientRetryOnTimeoutIntegrationTest.java | 79 +++++++++++++++++++ 4 files changed, 116 insertions(+), 1 deletion(-) create mode 100644 .changes/next-release/bugfix-AWSSDKforJavav2-b5a2807.json create mode 100644 services/dynamodb/src/it/java/software/amazon/awssdk/services/dynamodb/DynamoDbJavaClientRetryOnTimeoutIntegrationTest.java diff --git a/.changes/next-release/bugfix-AWSSDKforJavav2-b5a2807.json b/.changes/next-release/bugfix-AWSSDKforJavav2-b5a2807.json new file mode 100644 index 000000000000..448aa62da4b2 --- /dev/null +++ b/.changes/next-release/bugfix-AWSSDKforJavav2-b5a2807.json @@ -0,0 +1,6 @@ +{ + "category": "AWS SDK for Java v2", + "contributor": "", + "type": "bugfix", + "description": "Fix for [#1684](https://github.com/aws/aws-sdk-java-v2/issues/1684) Some of the Retry attempts which failed due to the API TimeOuts did not successfully retried but ended up with AbortedException." +} diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/TimeoutExceptionHandlingStage.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/TimeoutExceptionHandlingStage.java index f7325a25873d..37e85381074e 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/TimeoutExceptionHandlingStage.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/TimeoutExceptionHandlingStage.java @@ -25,6 +25,7 @@ import software.amazon.awssdk.core.client.config.SdkClientOption; import software.amazon.awssdk.core.exception.AbortedException; import software.amazon.awssdk.core.exception.ApiCallAttemptTimeoutException; +import software.amazon.awssdk.core.exception.SdkClientException; import software.amazon.awssdk.core.exception.SdkInterruptedException; import software.amazon.awssdk.core.internal.http.HttpClientDependencies; import software.amazon.awssdk.core.internal.http.RequestExecutionContext; @@ -90,7 +91,8 @@ public Response execute(SdkHttpFullRequest request, RequestExecutionCon */ private Exception translatePipelineException(RequestExecutionContext context, Exception e) { if (e instanceof InterruptedException || e instanceof IOException || - e instanceof AbortedException || Thread.currentThread().isInterrupted()) { + e instanceof AbortedException || Thread.currentThread().isInterrupted() + || (e instanceof SdkClientException && isCausedByApiCallAttemptTimeout(context))) { return handleTimeoutCausedException(context, e); } return e; diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/http/pipeline/stages/TimeoutExceptionHandlingStageTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/http/pipeline/stages/TimeoutExceptionHandlingStageTest.java index 753f1babdb1a..285090be9f10 100644 --- a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/http/pipeline/stages/TimeoutExceptionHandlingStageTest.java +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/http/pipeline/stages/TimeoutExceptionHandlingStageTest.java @@ -34,6 +34,7 @@ import software.amazon.awssdk.core.client.config.SdkClientConfiguration; import software.amazon.awssdk.core.exception.AbortedException; import software.amazon.awssdk.core.exception.ApiCallAttemptTimeoutException; +import software.amazon.awssdk.core.exception.SdkClientException; import software.amazon.awssdk.core.http.NoopTestRequest; import software.amazon.awssdk.core.internal.http.HttpClientDependencies; import software.amazon.awssdk.core.internal.http.RequestExecutionContext; @@ -164,6 +165,33 @@ public void interruptFlagWasSet_causedByApiCallTimeout_shouldThrowInterruptExcep verifyInterruptStatusPreserved(); } + @Test + public void apiCallAttemptTimeoutException_causedBySdkClientException_as_apiCallAttemptTimeoutTask_Caused_SdkClientException() throws Exception { + when(apiCallTimeoutTask.hasExecuted()).thenReturn(false); + when(apiCallAttemptTimeoutTask.hasExecuted()).thenReturn(true); + when(requestPipeline.execute(any(), any())).thenThrow(SdkClientException.create("")); + verifyExceptionThrown(ApiCallAttemptTimeoutException.class); + } + + @Test + public void interruptedException_causedByApiCallAttemptTimeoutTask() throws Exception { + when(apiCallTimeoutTask.hasExecuted()).thenReturn(true); + when(apiCallAttemptTimeoutTask.hasExecuted()).thenReturn(true); + when(requestPipeline.execute(any(), any())).thenThrow(SdkClientException.class); + verifyExceptionThrown(InterruptedException.class); + } + + + @Test + public void abortedException_causedByApiCallAttemptTimeoutTask_shouldNotPropagate() throws Exception { + when(apiCallTimeoutTask.hasExecuted()).thenReturn(false); + when(apiCallAttemptTimeoutTask.hasExecuted()).thenReturn(true); + when(requestPipeline.execute(any(), any())).thenThrow(AbortedException.class); + verifyExceptionThrown(ApiCallAttemptTimeoutException.class); + } + + + private void verifyInterruptStatusPreserved() { assertThat(Thread.currentThread().isInterrupted()).isTrue(); diff --git a/services/dynamodb/src/it/java/software/amazon/awssdk/services/dynamodb/DynamoDbJavaClientRetryOnTimeoutIntegrationTest.java b/services/dynamodb/src/it/java/software/amazon/awssdk/services/dynamodb/DynamoDbJavaClientRetryOnTimeoutIntegrationTest.java new file mode 100644 index 000000000000..163607d23e60 --- /dev/null +++ b/services/dynamodb/src/it/java/software/amazon/awssdk/services/dynamodb/DynamoDbJavaClientRetryOnTimeoutIntegrationTest.java @@ -0,0 +1,79 @@ +/* + * 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.services.dynamodb; + +import org.junit.Test; +import software.amazon.awssdk.core.client.config.ClientOverrideConfiguration; +import software.amazon.awssdk.core.exception.ApiCallAttemptTimeoutException; +import software.amazon.awssdk.core.retry.RetryPolicy; +import software.amazon.awssdk.core.retry.conditions.OrRetryCondition; +import software.amazon.awssdk.core.retry.conditions.RetryCondition; +import software.amazon.awssdk.testutils.service.AwsTestBase; + +import java.time.Duration; +import java.util.concurrent.atomic.AtomicInteger; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + + +/** + * Simple test to check all the retries are made when all the API calls timeouts. + */ +public class DynamoDbJavaClientRetryOnTimeoutIntegrationTest extends AwsTestBase { + + private static DynamoDbClient ddb; + private final Integer RETRY_ATTEMPTS = 3; + + public static void setupWithRetryPolicy(RetryPolicy retryPolicy, Integer attemptTimeOutMillis) throws Exception { + ddb = DynamoDbClient.builder() + .overrideConfiguration(ClientOverrideConfiguration.builder() + .retryPolicy(retryPolicy) + .apiCallAttemptTimeout(Duration.ofMillis(attemptTimeOutMillis)) //forces really quick api call timeout + .build()) + .build(); + + } + + public static RetryPolicy createRetryPolicyWithCounter(AtomicInteger counter, Integer numOfAttempts) { + final RetryPolicy retryPolicy = RetryPolicy.defaultRetryPolicy().toBuilder() + .numRetries(numOfAttempts) + .retryCondition(OrRetryCondition.create( + context -> { + counter.incrementAndGet(); + return false; + }, RetryCondition.defaultRetryCondition())).build(); + + return retryPolicy; + + } + + @Test + public void testRetryAttemptsOnTimeOut() throws Exception { + AtomicInteger atomicInteger = new AtomicInteger(0); + Boolean apiTimeOutExceptionOccurred = Boolean.FALSE; + setupWithRetryPolicy(createRetryPolicyWithCounter(atomicInteger, RETRY_ATTEMPTS), 2); + try { + + ddb.listTables(); + } catch (ApiCallAttemptTimeoutException e) { + apiTimeOutExceptionOccurred = true; + } + assertEquals(RETRY_ATTEMPTS.intValue(), atomicInteger.get()); + assertTrue(apiTimeOutExceptionOccurred); + } + +}