From b8dcec98901d55b378bf6fc7fe796f609ee7c8ba Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Wed, 21 May 2025 13:16:31 -0700 Subject: [PATCH 1/4] Fix CompletableFuture hangs when RetryStrategy/MetricsCollector raise errors --- .../AsyncApiCallMetricCollectionStage.java | 3 + .../pipeline/stages/AsyncRetryableStage.java | 15 +- ...yncClientMetricCollectorExceptionTest.java | 129 ++++++++++++++++++ ...AsyncClientRetryStrategyExceptionTest.java | 110 +++++++++++++++ 4 files changed, 255 insertions(+), 2 deletions(-) create mode 100644 core/sdk-core/src/test/java/software/amazon/awssdk/core/client/AsyncClientMetricCollectorExceptionTest.java create mode 100644 core/sdk-core/src/test/java/software/amazon/awssdk/core/client/AsyncClientRetryStrategyExceptionTest.java diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/AsyncApiCallMetricCollectionStage.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/AsyncApiCallMetricCollectionStage.java index 09016026be1c..1d7d040971cf 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/AsyncApiCallMetricCollectionStage.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/AsyncApiCallMetricCollectionStage.java @@ -57,6 +57,9 @@ public CompletableFuture execute(SdkHttpFullRequest input, RequestExecu } else { future.complete(r); } + }).exceptionally(t -> { + future.completeExceptionally(t); + return null; }); return CompletableFutureUtils.forwardExceptionTo(future, executeFuture); diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/AsyncRetryableStage.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/AsyncRetryableStage.java index d92c264a1b39..c3f3314af829 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/AsyncRetryableStage.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/AsyncRetryableStage.java @@ -75,7 +75,11 @@ private RetryingExecutor(SdkHttpFullRequest request, RequestExecutionContext con public CompletableFuture> execute() { CompletableFuture> future = new CompletableFuture<>(); - attemptFirstExecute(future); + try { + attemptFirstExecute(future); + } catch (Throwable t) { + future.completeExceptionally(t); + } return future; } @@ -126,6 +130,9 @@ private void attemptExecute(CompletableFuture> future) { retryableStageHelper.recordAttemptSucceeded(); future.complete(response); + }).exceptionally(t -> { + future.completeExceptionally(t); + return null; }); } @@ -149,7 +156,11 @@ public void maybeAttemptExecute(CompletableFuture> future) { private void maybeRetryExecute(CompletableFuture> future, Exception exception) { retryableStageHelper.setLastException(exception); - maybeAttemptExecute(future); + try { + maybeAttemptExecute(future); + } catch (Throwable t) { + future.completeExceptionally(t); + } } } } diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/client/AsyncClientMetricCollectorExceptionTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/client/AsyncClientMetricCollectorExceptionTest.java new file mode 100644 index 000000000000..eb93def6b1ef --- /dev/null +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/client/AsyncClientMetricCollectorExceptionTest.java @@ -0,0 +1,129 @@ +/* + * 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.core.client; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doThrow; +import static org.mockito.Mockito.when; +import static software.amazon.awssdk.core.internal.util.AsyncResponseHandlerTestUtils.noOpResponseHandler; +import static utils.HttpTestUtils.testAsyncClientBuilder; + +import java.time.Duration; +import java.util.Collections; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.stubbing.Answer; +import software.amazon.awssdk.core.Response; +import software.amazon.awssdk.core.SdkResponse; +import software.amazon.awssdk.core.async.EmptyPublisher; +import software.amazon.awssdk.core.http.ExecutionContext; +import software.amazon.awssdk.core.http.NoopTestRequest; +import software.amazon.awssdk.core.interceptor.ExecutionAttributes; +import software.amazon.awssdk.core.interceptor.ExecutionInterceptorChain; +import software.amazon.awssdk.core.interceptor.InterceptorContext; +import software.amazon.awssdk.core.internal.http.AmazonAsyncHttpClient; +import software.amazon.awssdk.core.metrics.CoreMetric; +import software.amazon.awssdk.core.protocol.VoidSdkResponse; +import software.amazon.awssdk.http.SdkHttpFullRequest; +import software.amazon.awssdk.http.SdkHttpFullResponse; +import software.amazon.awssdk.http.SdkHttpResponse; +import software.amazon.awssdk.http.async.AsyncExecuteRequest; +import software.amazon.awssdk.http.async.SdkAsyncHttpClient; +import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; +import software.amazon.awssdk.metrics.MetricCollector; +import software.amazon.awssdk.retries.DefaultRetryStrategy; +import utils.ValidSdkObjects; + +/** + * Tests to verify that exceptions thrown by the MetricCollector are reported through the returned future. + * {@link java.util.concurrent.CompletableFuture}. + * + * @see AsyncClientHandlerExceptionTest + */ +@RunWith(MockitoJUnitRunner.class) +public class AsyncClientMetricCollectorExceptionTest { + + public static final String MESSAGE = "test exception"; + + @Mock + private MetricCollector metricCollector; + + @Mock + private SdkAsyncHttpClient asyncHttpClient; + + @Test + public void exceptionInReportMetricReportedInFuture() throws Exception { + when(metricCollector.createChild(any())).thenReturn(metricCollector); + Exception exception = new RuntimeException(MESSAGE); + doThrow(exception).when(metricCollector).reportMetric(eq(CoreMetric.API_CALL_DURATION), any(Duration.class)); + + CompletableFuture responseFuture = makeRequest(); + + assertThatThrownBy(() -> responseFuture.get(1, TimeUnit.SECONDS)).hasRootCause(exception); + } + + private CompletableFuture makeRequest() throws Exception { + when(asyncHttpClient.execute(any(AsyncExecuteRequest.class))).thenAnswer((Answer>) invocationOnMock -> { + SdkAsyncHttpResponseHandler handler = invocationOnMock.getArgument(0, AsyncExecuteRequest.class).responseHandler(); + handler.onHeaders(SdkHttpFullResponse.builder() + .statusCode(200) + .build()); + handler.onStream(new EmptyPublisher<>()); + return CompletableFuture.completedFuture(null); + }); + + AmazonAsyncHttpClient asyncClient = testAsyncClientBuilder() + .retryStrategy(DefaultRetryStrategy.doNotRetry()) + .asyncHttpClient(asyncHttpClient) + .build(); + + SdkHttpFullRequest httpFullRequest = ValidSdkObjects.sdkHttpFullRequest().build(); + NoopTestRequest sdkRequest = NoopTestRequest.builder().build(); + InterceptorContext interceptorContext = InterceptorContext + .builder() + .request(sdkRequest) + .httpRequest(httpFullRequest) + .build(); + + Response response = + Response.builder() + .isSuccess(true) + .response(VoidSdkResponse.builder().build()) + .httpResponse(SdkHttpResponse.builder().statusCode(200).build()) + .build(); + + return asyncClient + .requestExecutionBuilder() + .originalRequest(sdkRequest) + .request(httpFullRequest) + .executionContext( + ExecutionContext + .builder() + .executionAttributes(new ExecutionAttributes()) + .interceptorContext(interceptorContext) + .metricCollector(metricCollector) + .interceptorChain(new ExecutionInterceptorChain(Collections.emptyList())) + .build() + ) + .execute(noOpResponseHandler(response)); + } +} diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/client/AsyncClientRetryStrategyExceptionTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/client/AsyncClientRetryStrategyExceptionTest.java new file mode 100644 index 000000000000..983be14c8451 --- /dev/null +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/client/AsyncClientRetryStrategyExceptionTest.java @@ -0,0 +1,110 @@ +/* + * 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.core.client; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.when; +import static software.amazon.awssdk.core.internal.util.AsyncResponseHandlerTestUtils.noOpResponseHandler; +import static utils.HttpTestUtils.testAsyncClientBuilder; + +import java.time.Duration; +import java.util.Collections; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; +import software.amazon.awssdk.core.SdkResponse; +import software.amazon.awssdk.core.http.ExecutionContext; +import software.amazon.awssdk.core.http.NoopTestRequest; +import software.amazon.awssdk.core.interceptor.ExecutionAttributes; +import software.amazon.awssdk.core.interceptor.ExecutionInterceptorChain; +import software.amazon.awssdk.core.interceptor.InterceptorContext; +import software.amazon.awssdk.core.internal.http.AmazonAsyncHttpClient; +import software.amazon.awssdk.http.SdkHttpFullRequest; +import software.amazon.awssdk.metrics.MetricCollector; +import software.amazon.awssdk.retries.api.AcquireInitialTokenResponse; +import software.amazon.awssdk.retries.api.RetryStrategy; +import software.amazon.awssdk.retries.api.RetryToken; +import utils.ValidSdkObjects; + +/** + * Tests to verify that exceptions thrown by the RetryStrategy are reported through the returned future. + * {@link java.util.concurrent.CompletableFuture}. + * + * @see AsyncClientHandlerExceptionTest + */ +@RunWith(MockitoJUnitRunner.class) +public class AsyncClientRetryStrategyExceptionTest { + + public static final String MESSAGE = "test exception"; + + @Mock + private RetryStrategy retryStrategy; + + @Test + public void exceptionInInitialTokenReportedInFuture() { + Exception exception = new RuntimeException(MESSAGE); + when(retryStrategy.acquireInitialToken(any())).thenThrow(exception); + + CompletableFuture responseFuture = makeRequest(); + + assertThatThrownBy(() -> responseFuture.get(1, TimeUnit.SECONDS)).hasRootCause(exception); + } + + @Test + public void exceptionInRefreshTokenReportedInFuture() { + when(retryStrategy.acquireInitialToken(any())).thenReturn( + AcquireInitialTokenResponse.create(new RetryToken() { + }, Duration.ZERO) + ); + Exception exception = new RuntimeException(MESSAGE); + when(retryStrategy.refreshRetryToken(any())).thenThrow(exception); + + CompletableFuture responseFuture = makeRequest(); + + assertThatThrownBy(() -> responseFuture.get(1, TimeUnit.SECONDS)).hasRootCause(exception); + } + + private CompletableFuture makeRequest() { + AmazonAsyncHttpClient asyncClient = testAsyncClientBuilder().retryStrategy(retryStrategy).build(); + + SdkHttpFullRequest httpFullRequest = ValidSdkObjects.sdkHttpFullRequest().build(); + NoopTestRequest sdkRequest = NoopTestRequest.builder().build(); + InterceptorContext interceptorContext = InterceptorContext + .builder() + .request(sdkRequest) + .httpRequest(httpFullRequest) + .build(); + + return asyncClient + .requestExecutionBuilder() + .originalRequest(sdkRequest) + .request(httpFullRequest) + .executionContext( + ExecutionContext + .builder() + .executionAttributes(new ExecutionAttributes()) + .interceptorContext(interceptorContext) + .metricCollector(MetricCollector.create("test")) + .interceptorChain(new ExecutionInterceptorChain(Collections.emptyList())) + .build() + ) + .execute(noOpResponseHandler()); + } +} From 91569d79938a50042287c87f9c3e28d11e08e7b8 Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Wed, 21 May 2025 13:22:31 -0700 Subject: [PATCH 2/4] add changelog --- .changes/next-release/bugfix-AWSSDKforJavav2-dc851b9.json | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/next-release/bugfix-AWSSDKforJavav2-dc851b9.json diff --git a/.changes/next-release/bugfix-AWSSDKforJavav2-dc851b9.json b/.changes/next-release/bugfix-AWSSDKforJavav2-dc851b9.json new file mode 100644 index 000000000000..33ce3574da8f --- /dev/null +++ b/.changes/next-release/bugfix-AWSSDKforJavav2-dc851b9.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "AWS SDK for Java v2", + "contributor": "", + "description": "Fix CompletableFuture hanging when RetryStrategy/MetricsCollector raise errors" +} From f19d933782bcc3a981564cd5a09098f0eed47c2b Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Wed, 21 May 2025 14:05:04 -0700 Subject: [PATCH 3/4] Fix failing test --- .../internal/http/pipeline/stages/AsyncRetryableStage.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/AsyncRetryableStage.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/AsyncRetryableStage.java index c3f3314af829..3e3d79a68524 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/AsyncRetryableStage.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/http/pipeline/stages/AsyncRetryableStage.java @@ -130,9 +130,6 @@ private void attemptExecute(CompletableFuture> future) { retryableStageHelper.recordAttemptSucceeded(); future.complete(response); - }).exceptionally(t -> { - future.completeExceptionally(t); - return null; }); } From d6fb65c6ee8f1f26ca89fcadd8fc4481113d61e5 Mon Sep 17 00:00:00 2001 From: Alex Woods Date: Wed, 21 May 2025 15:00:42 -0700 Subject: [PATCH 4/4] Remove useless exception from test method signature --- .../core/client/AsyncClientMetricCollectorExceptionTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/client/AsyncClientMetricCollectorExceptionTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/client/AsyncClientMetricCollectorExceptionTest.java index eb93def6b1ef..52de25b3063f 100644 --- a/core/sdk-core/src/test/java/software/amazon/awssdk/core/client/AsyncClientMetricCollectorExceptionTest.java +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/client/AsyncClientMetricCollectorExceptionTest.java @@ -71,7 +71,7 @@ public class AsyncClientMetricCollectorExceptionTest { private SdkAsyncHttpClient asyncHttpClient; @Test - public void exceptionInReportMetricReportedInFuture() throws Exception { + public void exceptionInReportMetricReportedInFuture() { when(metricCollector.createChild(any())).thenReturn(metricCollector); Exception exception = new RuntimeException(MESSAGE); doThrow(exception).when(metricCollector).reportMetric(eq(CoreMetric.API_CALL_DURATION), any(Duration.class)); @@ -81,7 +81,7 @@ public void exceptionInReportMetricReportedInFuture() throws Exception { assertThatThrownBy(() -> responseFuture.get(1, TimeUnit.SECONDS)).hasRootCause(exception); } - private CompletableFuture makeRequest() throws Exception { + private CompletableFuture makeRequest() { when(asyncHttpClient.execute(any(AsyncExecuteRequest.class))).thenAnswer((Answer>) invocationOnMock -> { SdkAsyncHttpResponseHandler handler = invocationOnMock.getArgument(0, AsyncExecuteRequest.class).responseHandler(); handler.onHeaders(SdkHttpFullResponse.builder()