From 051f869bbf5f604a7fac5772fd848b57069f03bd Mon Sep 17 00:00:00 2001 From: Zoe Wang <33073555+zoewangg@users.noreply.github.com> Date: Mon, 11 May 2026 16:04:23 -0700 Subject: [PATCH 1/7] Switch from the CRT pull-based HttpRequestBodyStream model to the push-based HttpStreamBase.writeData() API to avoid potential deadlock issue when request body InputStream blocks. --- .../bugfix-AWSCRTHTTPClient-aee08c2.json | 6 ++ .../awssdk/http/crt/AwsCrtHttpClient.java | 38 ++++++++- .../http/crt/internal/CrtRequestExecutor.java | 52 ++++++++++--- .../internal/request/CrtRequestAdapter.java | 9 +-- .../request/CrtRequestInputStreamAdapter.java | 78 ------------------- .../crt/internal/CrtRequestExecutorTest.java | 18 ++--- pom.xml | 2 +- 7 files changed, 94 insertions(+), 109 deletions(-) create mode 100644 .changes/next-release/bugfix-AWSCRTHTTPClient-aee08c2.json delete mode 100644 http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestInputStreamAdapter.java diff --git a/.changes/next-release/bugfix-AWSCRTHTTPClient-aee08c2.json b/.changes/next-release/bugfix-AWSCRTHTTPClient-aee08c2.json new file mode 100644 index 000000000000..f0cc6298ec3c --- /dev/null +++ b/.changes/next-release/bugfix-AWSCRTHTTPClient-aee08c2.json @@ -0,0 +1,6 @@ +{ + "type": "bugfix", + "category": "AWS CRT HTTP Client", + "contributor": "", + "description": "Fixed a potential deadlock in `AwsCrtHttpClient` that could occur when the request body `InputStream` blocked waiting for data on the CRT event loop thread. This could happen when a blocking stream (e.g., a `BufferedInputStream` wrapping a `ResponseInputStream`) was used as a request body and the read depended on the same event loop thread to deliver data. Request body writing now happens on the caller thread." +} 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 9c6d769e48fa..11ef6efcc655 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 @@ -18,13 +18,17 @@ import static software.amazon.awssdk.http.HttpMetric.HTTP_CLIENT_NAME; import java.io.IOException; +import java.io.InputStream; import java.time.Duration; +import java.util.Arrays; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.function.Consumer; import software.amazon.awssdk.annotations.SdkPublicApi; import software.amazon.awssdk.crt.http.HttpException; +import software.amazon.awssdk.crt.http.HttpStreamBase; import software.amazon.awssdk.crt.http.HttpStreamManager; +import software.amazon.awssdk.http.ContentStreamProvider; import software.amazon.awssdk.http.ExecutableHttpRequest; import software.amazon.awssdk.http.HttpExecuteRequest; import software.amazon.awssdk.http.HttpExecuteResponse; @@ -35,6 +39,7 @@ import software.amazon.awssdk.http.crt.internal.AwsCrtClientBuilderBase; import software.amazon.awssdk.http.crt.internal.CrtRequestContext; import software.amazon.awssdk.http.crt.internal.CrtRequestExecutor; +import software.amazon.awssdk.http.crt.internal.CrtUtils; import software.amazon.awssdk.utils.AttributeMap; import software.amazon.awssdk.utils.CompletableFutureUtils; @@ -107,6 +112,8 @@ public ExecutableHttpRequest prepareRequest(HttpExecuteRequest request) { } private static final class CrtHttpRequest implements ExecutableHttpRequest { + private static final int WRITE_BUFFER_SIZE = 16 * 1024; + private final CrtRequestContext context; private volatile CompletableFuture responseFuture; @@ -119,7 +126,14 @@ public HttpExecuteResponse call() throws IOException { HttpExecuteResponse.Builder builder = HttpExecuteResponse.builder(); try { - responseFuture = new CrtRequestExecutor().execute(context); + CrtRequestExecutor.ExecutionResult execution = new CrtRequestExecutor().execute(context); + responseFuture = execution.responseFuture(); + + // Wait for the stream to be acquired, then write the request body from the caller thread. + // This avoids blocking the CRT event loop thread in InputStream.read(). + HttpStreamBase stream = CompletableFutureUtils.joinInterruptibly(execution.streamFuture()); + writeRequestBody(stream); + SdkHttpFullResponse response = CompletableFutureUtils.joinInterruptibly(responseFuture); builder.response(response); builder.responseBody(response.content().orElse(null)); @@ -140,6 +154,10 @@ public HttpExecuteResponse call() throws IOException { } if (cause instanceof HttpException) { + Throwable wrapped = CrtUtils.wrapCrtException(cause); + if (wrapped instanceof IOException) { + throw (IOException) wrapped; + } throw (HttpException) cause; } @@ -151,6 +169,24 @@ public HttpExecuteResponse call() throws IOException { } } + private void writeRequestBody(HttpStreamBase stream) throws IOException { + ContentStreamProvider provider = context.sdkRequest().contentStreamProvider().orElse(null); + if (provider == null) { + return; + } + + try (InputStream inputStream = provider.newStream()) { + byte[] buf = new byte[WRITE_BUFFER_SIZE]; + int read; + + while ((read = inputStream.read(buf, 0, buf.length)) >= 0) { + byte[] chunk = read == buf.length ? buf : Arrays.copyOf(buf, read); + CompletableFutureUtils.joinInterruptibly(stream.writeData(chunk, false)); + } + CompletableFutureUtils.joinInterruptibly(stream.writeData(null, true)); + } + } + @Override public void abort() { if (responseFuture != null) { diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java index 7165696435e1..7c764cf127df 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java @@ -32,37 +32,40 @@ @SdkInternalApi public final class CrtRequestExecutor { - public CompletableFuture execute(CrtRequestContext executionContext) { - CompletableFuture requestFuture = new CompletableFuture<>(); + public ExecutionResult execute(CrtRequestContext executionContext) { + CompletableFuture responseFuture = new CompletableFuture<>(); + CompletableFuture streamFuture; try { - doExecute(executionContext, requestFuture); + streamFuture = doExecute(executionContext, responseFuture); } catch (Throwable t) { - requestFuture.completeExceptionally(t); + responseFuture.completeExceptionally(t); + streamFuture = new CompletableFuture<>(); + streamFuture.completeExceptionally(t); } - return requestFuture; + return new ExecutionResult(streamFuture, responseFuture); } - private void doExecute(CrtRequestContext executionContext, CompletableFuture requestFuture) { + private CompletableFuture doExecute(CrtRequestContext executionContext, + CompletableFuture responseFuture) { MetricCollector metricCollector = executionContext.metricCollector(); boolean shouldPublishMetrics = metricCollector != null && !(metricCollector instanceof NoOpMetricCollector); long acquireStartTime = 0; if (shouldPublishMetrics) { - // go ahead and get acquireStartTime for the concurrency timer as early as possible, - // so it's as accurate as possible, but only do it in a branch since clock_gettime() - // results in a full sys call barrier (multiple mutexes and a hw interrupt). acquireStartTime = System.nanoTime(); } - HttpStreamBaseResponseHandler crtResponseHandler = new InputStreamAdaptingHttpStreamResponseHandler(requestFuture); + HttpStreamBaseResponseHandler crtResponseHandler = new InputStreamAdaptingHttpStreamResponseHandler(responseFuture); HttpRequest crtRequest = CrtRequestAdapter.toCrtRequest(executionContext); + boolean hasBody = executionContext.sdkRequest().contentStreamProvider().isPresent(); + CompletableFuture streamFuture = - executionContext.streamManager().acquireStream(crtRequest, crtResponseHandler); + executionContext.streamManager().acquireStream(crtRequest, crtResponseHandler, hasBody); long finalAcquireStartTime = acquireStartTime; @@ -73,8 +76,33 @@ private void doExecute(CrtRequestContext executionContext, CompletableFuture streamFuture; + private final CompletableFuture responseFuture; + + ExecutionResult(CompletableFuture streamFuture, + CompletableFuture responseFuture) { + this.streamFuture = streamFuture; + this.responseFuture = responseFuture; + } + + public CompletableFuture streamFuture() { + return streamFuture; + } + + public CompletableFuture responseFuture() { + return responseFuture; + } } } diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestAdapter.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestAdapter.java index 8672d80b0d1b..e97b1adb514e 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestAdapter.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestAdapter.java @@ -78,14 +78,7 @@ public static HttpRequest toCrtRequest(CrtRequestContext request) { HttpHeader[] crtHeaderArray = asArray(createHttpHeaderList(sdkRequest.getUri(), sdkExecuteRequest)); String finalEncodedPath = encodedPath + encodedQueryString; - return sdkExecuteRequest.contentStreamProvider() - .map(provider -> new HttpRequest(method, - finalEncodedPath, - crtHeaderArray, - new CrtRequestInputStreamAdapter(provider))) - .orElse(new HttpRequest(method, - finalEncodedPath, - crtHeaderArray, null)); + return new HttpRequest(method, finalEncodedPath, crtHeaderArray, null); } private static HttpHeader[] asArray(List crtHeaderList) { diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestInputStreamAdapter.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestInputStreamAdapter.java deleted file mode 100644 index 68f418b9e1df..000000000000 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestInputStreamAdapter.java +++ /dev/null @@ -1,78 +0,0 @@ -/* - * 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.internal.request; - -import static java.lang.Math.min; - -import java.io.IOException; -import java.io.InputStream; -import java.nio.ByteBuffer; -import software.amazon.awssdk.annotations.SdkInternalApi; -import software.amazon.awssdk.crt.http.HttpRequestBodyStream; -import software.amazon.awssdk.http.ContentStreamProvider; - -@SdkInternalApi -final class CrtRequestInputStreamAdapter implements HttpRequestBodyStream { - private static final int READ_BUFFER_SIZE = 16 * 1024; - - private final ContentStreamProvider provider; - private volatile InputStream providerStream; - private final byte[] readBuffer = new byte[READ_BUFFER_SIZE]; - - CrtRequestInputStreamAdapter(ContentStreamProvider provider) { - this.provider = provider; - } - - @Override - public boolean sendRequestBody(ByteBuffer bodyBytesOut) { - int read; - - try { - if (providerStream == null) { - createNewStream(); - } - - int toRead = min(READ_BUFFER_SIZE, bodyBytesOut.remaining()); - read = providerStream.read(readBuffer, 0, toRead); - - if (read > 0) { - bodyBytesOut.put(readBuffer, 0, read); - } - } catch (IOException ioe) { - throw new RuntimeException(ioe); - } - - return read < 0; - } - - @Override - public boolean resetPosition() { - try { - createNewStream(); - } catch (IOException ioe) { - throw new RuntimeException(ioe); - } - - return true; - } - - private void createNewStream() throws IOException { - if (providerStream != null) { - providerStream.close(); - } - providerStream = provider.newStream(); - } -} diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java index 456000ac1150..87a2849edc7b 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java @@ -87,7 +87,7 @@ public void execute_requestConversionFails_failsFuture() { .request(HttpExecuteRequest.builder().build()) .build(); - CompletableFuture executeFuture = requestExecutor.execute(context); + CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); assertThat(executeFuture).hasFailedWithThrowableThat().isInstanceOf(NullPointerException.class); } @@ -98,11 +98,11 @@ public void execute_acquireStreamFails_wrapsWithIOException() { CrtRequestContext context = crtRequestContext(); CompletableFuture completableFuture = new CompletableFuture<>(); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequest.class), Mockito.any(HttpStreamBaseResponseHandler.class))) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequest.class), Mockito.any(HttpStreamBaseResponseHandler.class), Mockito.anyBoolean())) .thenReturn(completableFuture); completableFuture.completeExceptionally(exception); - CompletableFuture executeFuture = requestExecutor.execute(context); + CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); assertThat(executeFuture).hasFailedWithThrowableThat().hasCause(exception).isInstanceOf(IOException.class); } @@ -113,10 +113,10 @@ public void execute_retryableException_wrapsWithIOException(Throwable throwable) CrtRequestContext context = crtRequestContext(); CompletableFuture completableFuture = CompletableFutureUtils.failedFuture(throwable); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequest.class), Mockito.any(HttpStreamBaseResponseHandler.class))) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequest.class), Mockito.any(HttpStreamBaseResponseHandler.class), Mockito.anyBoolean())) .thenReturn(completableFuture); - CompletableFuture executeFuture = requestExecutor.execute(context); + CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); assertThat(executeFuture).hasFailedWithThrowableThat().hasCause(throwable).isInstanceOf(IOException.class); } @@ -130,10 +130,10 @@ public void execute_httpException_mapsToCorrectException(Entry completableFuture = CompletableFutureUtils.failedFuture(exception); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequest.class), Mockito.any(HttpStreamBaseResponseHandler.class))) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequest.class), Mockito.any(HttpStreamBaseResponseHandler.class), Mockito.anyBoolean())) .thenReturn(completableFuture); - CompletableFuture executeFuture = requestExecutor.execute(context); + CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); assertThatThrownBy(executeFuture::join).hasCauseInstanceOf(expectedExceptionClass); } @@ -143,10 +143,10 @@ public void execute_nonRetryableHttpException_doesNotWrapWithIOException() { CrtRequestContext context = crtRequestContext(); CompletableFuture completableFuture = CompletableFutureUtils.failedFuture(exception); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequest.class), Mockito.any(HttpStreamBaseResponseHandler.class))) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequest.class), Mockito.any(HttpStreamBaseResponseHandler.class), Mockito.anyBoolean())) .thenReturn(completableFuture); - CompletableFuture executeFuture = requestExecutor.execute(context); + CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); assertThatThrownBy(executeFuture::join).hasCause(exception); } diff --git a/pom.xml b/pom.xml index 1b6bee34df5e..1848cf1dd892 100644 --- a/pom.xml +++ b/pom.xml @@ -131,7 +131,7 @@ 3.1.5 1.17.1 1.37 - 0.45.1 + 1.0.0-SNAPSHOT 5.10.3 From 2d49203b505d4761b8459e4ed8f97c407d16dec8 Mon Sep 17 00:00:00 2001 From: Zoe Wang <33073555+zoewangg@users.noreply.github.com> Date: Wed, 13 May 2026 13:02:32 -0700 Subject: [PATCH 2/7] Extract CrtStreamHandler to manage stream lifecycle Move stream lifecycle operations (writeData, incrementWindow, releaseConnection, closeConnection) from ResponseHandlerHelper into a dedicated CrtStreamHandler class. This separates header parsing from stream management and makes the shared stream guard explicit between the request executor and response handler. ResponseHandlerHelper now only handles response header parsing. --- .../awssdk/http/crt/AwsCrtHttpClient.java | 20 +-- .../crt/internal/CrtAsyncRequestExecutor.java | 6 +- .../http/crt/internal/CrtRequestExecutor.java | 46 ++----- .../http/crt/internal/CrtStreamHandler.java | 117 +++++++++++++++++ .../internal/response/CrtResponseAdapter.java | 30 +++-- ...reamAdaptingHttpStreamResponseHandler.java | 24 ++-- .../response/ResponseHandlerHelper.java | 44 ------- .../BaseHttpStreamResponseHandlerTest.java | 44 ++++--- .../crt/internal/CrtRequestExecutorTest.java | 10 +- .../crt/internal/CrtResponseHandlerTest.java | 12 +- .../crt/internal/CrtStreamHandlerTest.java | 90 +++++++++++++ ...AdaptingHttpStreamResponseHandlerTest.java | 22 ++-- .../internal/ResponseHandlerHelperTest.java | 124 ------------------ 13 files changed, 320 insertions(+), 269 deletions(-) create mode 100644 http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandler.java create mode 100644 http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandlerTest.java delete mode 100644 http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/ResponseHandlerHelperTest.java 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 11ef6efcc655..d81c286fc5f4 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 @@ -26,7 +26,6 @@ import java.util.function.Consumer; import software.amazon.awssdk.annotations.SdkPublicApi; import software.amazon.awssdk.crt.http.HttpException; -import software.amazon.awssdk.crt.http.HttpStreamBase; import software.amazon.awssdk.crt.http.HttpStreamManager; import software.amazon.awssdk.http.ContentStreamProvider; import software.amazon.awssdk.http.ExecutableHttpRequest; @@ -39,6 +38,7 @@ import software.amazon.awssdk.http.crt.internal.AwsCrtClientBuilderBase; import software.amazon.awssdk.http.crt.internal.CrtRequestContext; import software.amazon.awssdk.http.crt.internal.CrtRequestExecutor; +import software.amazon.awssdk.http.crt.internal.CrtStreamHandler; import software.amazon.awssdk.http.crt.internal.CrtUtils; import software.amazon.awssdk.utils.AttributeMap; import software.amazon.awssdk.utils.CompletableFutureUtils; @@ -126,13 +126,13 @@ public HttpExecuteResponse call() throws IOException { HttpExecuteResponse.Builder builder = HttpExecuteResponse.builder(); try { - CrtRequestExecutor.ExecutionResult execution = new CrtRequestExecutor().execute(context); - responseFuture = execution.responseFuture(); + CrtStreamHandler streamHandler = new CrtStreamHandler(); + responseFuture = new CrtRequestExecutor().execute(context, streamHandler); - // Wait for the stream to be acquired, then write the request body from the caller thread. + // Write the request body from the caller thread via the stream handler, + // which guards against concurrent stream close with a synchronized block. // This avoids blocking the CRT event loop thread in InputStream.read(). - HttpStreamBase stream = CompletableFutureUtils.joinInterruptibly(execution.streamFuture()); - writeRequestBody(stream); + writeRequestBody(streamHandler); SdkHttpFullResponse response = CompletableFutureUtils.joinInterruptibly(responseFuture); builder.response(response); @@ -169,21 +169,21 @@ public HttpExecuteResponse call() throws IOException { } } - private void writeRequestBody(HttpStreamBase stream) throws IOException { + private void writeRequestBody(CrtStreamHandler streamHandler) throws IOException { ContentStreamProvider provider = context.sdkRequest().contentStreamProvider().orElse(null); if (provider == null) { return; } + streamHandler.waitForStream(); try (InputStream inputStream = provider.newStream()) { byte[] buf = new byte[WRITE_BUFFER_SIZE]; int read; - while ((read = inputStream.read(buf, 0, buf.length)) >= 0) { byte[] chunk = read == buf.length ? buf : Arrays.copyOf(buf, read); - CompletableFutureUtils.joinInterruptibly(stream.writeData(chunk, false)); + CompletableFutureUtils.joinInterruptibly(streamHandler.writeData(chunk, false)); } - CompletableFutureUtils.joinInterruptibly(stream.writeData(null, true)); + CompletableFutureUtils.joinInterruptibly(streamHandler.writeData(null, true)); } } diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java index 7629683899a5..e3dfe024e9cd 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java @@ -67,8 +67,10 @@ private void doExecute(CrtAsyncRequestContext executionContext, HttpRequestBase crtRequest = toAsyncCrtRequest(executionContext); + CrtStreamHandler streamHandler = new CrtStreamHandler(); + HttpStreamBaseResponseHandler crtResponseHandler = - CrtResponseAdapter.toCrtResponseHandler(requestFuture, asyncRequest.responseHandler()); + CrtResponseAdapter.toCrtResponseHandler(requestFuture, asyncRequest.responseHandler(), streamHandler); CompletableFuture streamFuture = executionContext.streamManager().acquireStream(crtRequest, crtResponseHandler); @@ -83,6 +85,8 @@ private void doExecute(CrtAsyncRequestContext executionContext, if (throwable != null) { Throwable toThrow = wrapCrtException(throwable); reportAsyncFailure(toThrow, requestFuture, asyncRequest.responseHandler()); + } else { + streamHandler.setStream(stream); } }); } diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java index 7c764cf127df..98ace2a65766 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java @@ -22,7 +22,6 @@ import software.amazon.awssdk.annotations.SdkInternalApi; import software.amazon.awssdk.crt.http.HttpRequest; import software.amazon.awssdk.crt.http.HttpStreamBase; -import software.amazon.awssdk.crt.http.HttpStreamBaseResponseHandler; import software.amazon.awssdk.http.SdkHttpFullResponse; import software.amazon.awssdk.http.crt.internal.request.CrtRequestAdapter; import software.amazon.awssdk.http.crt.internal.response.InputStreamAdaptingHttpStreamResponseHandler; @@ -32,23 +31,22 @@ @SdkInternalApi public final class CrtRequestExecutor { - public ExecutionResult execute(CrtRequestContext executionContext) { + public CompletableFuture execute(CrtRequestContext executionContext, + CrtStreamHandler streamHandler) { CompletableFuture responseFuture = new CompletableFuture<>(); - CompletableFuture streamFuture; try { - streamFuture = doExecute(executionContext, responseFuture); + doExecute(executionContext, responseFuture, streamHandler); } catch (Throwable t) { responseFuture.completeExceptionally(t); - streamFuture = new CompletableFuture<>(); - streamFuture.completeExceptionally(t); } - return new ExecutionResult(streamFuture, responseFuture); + return responseFuture; } - private CompletableFuture doExecute(CrtRequestContext executionContext, - CompletableFuture responseFuture) { + private void doExecute(CrtRequestContext executionContext, + CompletableFuture responseFuture, + CrtStreamHandler streamHandler) { MetricCollector metricCollector = executionContext.metricCollector(); boolean shouldPublishMetrics = metricCollector != null && !(metricCollector instanceof NoOpMetricCollector); @@ -58,7 +56,8 @@ private CompletableFuture doExecute(CrtRequestContext executionC acquireStartTime = System.nanoTime(); } - HttpStreamBaseResponseHandler crtResponseHandler = new InputStreamAdaptingHttpStreamResponseHandler(responseFuture); + InputStreamAdaptingHttpStreamResponseHandler crtResponseHandler = + new InputStreamAdaptingHttpStreamResponseHandler(responseFuture, streamHandler); HttpRequest crtRequest = CrtRequestAdapter.toCrtRequest(executionContext); @@ -77,32 +76,9 @@ private CompletableFuture doExecute(CrtRequestContext executionC if (throwable != null) { Throwable toThrow = wrapCrtException(throwable); responseFuture.completeExceptionally(toThrow); + } else { + streamHandler.setStream(streamBase); } }); - - return streamFuture; - } - - /** - * Holds the result of submitting a request to CRT: the stream (for writing body data via - * {@code writeData}) and the response future (for reading the response). - */ - public static final class ExecutionResult { - private final CompletableFuture streamFuture; - private final CompletableFuture responseFuture; - - ExecutionResult(CompletableFuture streamFuture, - CompletableFuture responseFuture) { - this.streamFuture = streamFuture; - this.responseFuture = responseFuture; - } - - public CompletableFuture streamFuture() { - return streamFuture; - } - - public CompletableFuture responseFuture() { - return responseFuture; - } } } diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandler.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandler.java new file mode 100644 index 000000000000..97bdcd8c6f56 --- /dev/null +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandler.java @@ -0,0 +1,117 @@ +/* + * 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.internal; + +import java.io.IOException; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import software.amazon.awssdk.annotations.SdkInternalApi; +import software.amazon.awssdk.crt.http.HttpStreamBase; + +/** + * Manages the lifecycle of a CRT HTTP stream, providing thread-safe access to stream operations. + * Shared between the request executor (for writing body data) and the response handler (for + * incrementing the window and releasing/closing the connection). + */ +@SdkInternalApi +public final class CrtStreamHandler { + + private final Object streamLock = new Object(); + private final CountDownLatch streamLatch = new CountDownLatch(1); + private HttpStreamBase stream; + private boolean streamClosed; + + /** + * Sets the stream. Called once when the stream is acquired from the connection pool. + */ + public void setStream(HttpStreamBase stream) { + this.stream = stream; + streamLatch.countDown(); + } + + /** + * Blocks until the stream has been acquired. + */ + public void waitForStream() { + try { + streamLatch.await(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new RuntimeException("Interrupted while waiting for stream", e); + } + } + + /** + * Write data to the stream. The caller must ensure the stream is ready (via {@link #waitForStream()}) + * before calling this method. + */ + public CompletableFuture writeData(byte[] data, boolean endStream) { + if (streamLatch.getCount() != 0) { + CompletableFuture future = new CompletableFuture<>(); + future.completeExceptionally( + new IllegalStateException("writeData called before stream is ready. Call waitForStream() first.")); + return future; + } + synchronized (streamLock) { + if (streamClosed) { + CompletableFuture future = new CompletableFuture<>(); + future.completeExceptionally( + new IOException("Stream is already closed, cannot write data.")); + return future; + } + return stream.writeData(data, endStream); + } + } + + public void incrementWindow(int windowSize) { + if (streamLatch.getCount() != 0) { + throw new IllegalStateException("incrementWindow called before stream is ready."); + } + synchronized (streamLock) { + if (!streamClosed) { + stream.incrementWindow(windowSize); + } + } + } + + /** + * Release the connection back to the pool so that it may be reused. This should be called when the request + * completes successfully and the response has been fully consumed. + */ + public void releaseConnection() { + synchronized (streamLock) { + if (!streamClosed && stream != null) { + streamClosed = true; + stream.close(); + } + } + } + + /** + * Cancel and close the stream, forcing the underlying connection to shut down rather than be returned to the + * connection pool. This should be called on error paths or when the stream is aborted before the response is + * fully consumed. {@code cancel()} must be invoked before {@code close()} per the CRT contract. + */ + public void closeConnection() { + synchronized (streamLock) { + if (!streamClosed && stream != null) { + streamClosed = true; + stream.cancel(); + stream.close(); + } + } + } +} diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/CrtResponseAdapter.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/CrtResponseAdapter.java index 1beaa872b5f4..df44ab28cbe0 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/CrtResponseAdapter.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/CrtResponseAdapter.java @@ -30,6 +30,7 @@ import software.amazon.awssdk.http.SdkHttpResponse; import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; import software.amazon.awssdk.http.crt.AwsCrtAsyncHttpClient; +import software.amazon.awssdk.http.crt.internal.CrtStreamHandler; import software.amazon.awssdk.utils.Logger; import software.amazon.awssdk.utils.Validate; import software.amazon.awssdk.utils.async.SimplePublisher; @@ -48,27 +49,39 @@ public final class CrtResponseAdapter implements HttpStreamBaseResponseHandler { private final SimplePublisher responsePublisher; private final SdkHttpResponse.Builder responseBuilder; private final ResponseHandlerHelper responseHandlerHelper; + private final CrtStreamHandler streamHandler; private CrtResponseAdapter(CompletableFuture completionFuture, - SdkAsyncHttpResponseHandler responseHandler) { - this(completionFuture, responseHandler, new SimplePublisher<>()); + SdkAsyncHttpResponseHandler responseHandler, + CrtStreamHandler streamHandler) { + this(completionFuture, responseHandler, new SimplePublisher<>(), streamHandler); } @SdkTestInternalApi public CrtResponseAdapter(CompletableFuture completionFuture, SdkAsyncHttpResponseHandler responseHandler, SimplePublisher simplePublisher) { + this(completionFuture, responseHandler, simplePublisher, new CrtStreamHandler()); + } + + @SdkTestInternalApi + public CrtResponseAdapter(CompletableFuture completionFuture, + SdkAsyncHttpResponseHandler responseHandler, + SimplePublisher simplePublisher, + CrtStreamHandler streamHandler) { this.completionFuture = Validate.paramNotNull(completionFuture, "completionFuture"); this.responseHandler = Validate.paramNotNull(responseHandler, "responseHandler"); this.responseBuilder = SdkHttpResponse.builder(); this.responsePublisher = simplePublisher; + this.streamHandler = streamHandler; this.responseHandlerHelper = new ResponseHandlerHelper(responseBuilder); } public static HttpStreamBaseResponseHandler toCrtResponseHandler( CompletableFuture requestFuture, - SdkAsyncHttpResponseHandler responseHandler) { - return new CrtResponseAdapter(requestFuture, responseHandler); + SdkAsyncHttpResponseHandler responseHandler, + CrtStreamHandler streamHandler) { + return new CrtResponseAdapter(requestFuture, responseHandler, streamHandler); } @Override @@ -89,17 +102,16 @@ public int onResponseBody(HttpStreamBase stream, byte[] bodyBytesIn) { CompletableFuture writeFuture = responsePublisher.send(ByteBuffer.wrap(bodyBytesIn)); if (writeFuture.isDone() && !writeFuture.isCompletedExceptionally()) { - // Optimization: If write succeeded immediately, return non-zero to avoid the extra call back into the CRT. return bodyBytesIn.length; } writeFuture.whenComplete((result, failure) -> { if (failure != null) { failResponseHandlerAndFuture(failure); - responseHandlerHelper.closeConnection(); + streamHandler.closeConnection(); return; } - responseHandlerHelper.incrementWindow(bodyBytesIn.length); + streamHandler.incrementWindow(bodyBytesIn.length); }); return 0; @@ -122,7 +134,7 @@ private void onSuccessfulResponseComplete() { } completionFuture.complete(null); }); - responseHandlerHelper.releaseConnection(); + streamHandler.releaseConnection(); } private void onFailedResponseComplete(HttpException error) { @@ -130,7 +142,7 @@ private void onFailedResponseComplete(HttpException error) { Throwable toThrow = wrapWithIoExceptionIfRetryable(error); responsePublisher.error(toThrow); failResponseHandlerAndFuture(toThrow); - responseHandlerHelper.closeConnection(); + streamHandler.closeConnection(); } private void failResponseHandlerAndFuture(Throwable error) { diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/InputStreamAdaptingHttpStreamResponseHandler.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/InputStreamAdaptingHttpStreamResponseHandler.java index bb04d71fdb2e..6275f14ae51d 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/InputStreamAdaptingHttpStreamResponseHandler.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/InputStreamAdaptingHttpStreamResponseHandler.java @@ -31,6 +31,7 @@ import software.amazon.awssdk.http.SdkHttpResponse; import software.amazon.awssdk.http.async.AbortableInputStreamSubscriber; import software.amazon.awssdk.http.crt.AwsCrtHttpClient; +import software.amazon.awssdk.http.crt.internal.CrtStreamHandler; import software.amazon.awssdk.utils.Logger; import software.amazon.awssdk.utils.async.SimplePublisher; @@ -45,17 +46,21 @@ public final class InputStreamAdaptingHttpStreamResponseHandler implements HttpS private final CompletableFuture requestCompletionFuture; private final SdkHttpFullResponse.Builder responseBuilder; private final ResponseHandlerHelper responseHandlerHelper; + private final CrtStreamHandler streamHandler; - public InputStreamAdaptingHttpStreamResponseHandler(CompletableFuture requestCompletionFuture) { - this(requestCompletionFuture, new SimplePublisher<>()); + public InputStreamAdaptingHttpStreamResponseHandler(CompletableFuture requestCompletionFuture, + CrtStreamHandler streamHandler) { + this(requestCompletionFuture, new SimplePublisher<>(), streamHandler); } @SdkTestInternalApi public InputStreamAdaptingHttpStreamResponseHandler(CompletableFuture requestCompletionFuture, - SimplePublisher simplePublisher) { + SimplePublisher simplePublisher, + CrtStreamHandler streamHandler) { this.requestCompletionFuture = requestCompletionFuture; this.responseBuilder = SdkHttpResponse.builder(); this.simplePublisher = simplePublisher; + this.streamHandler = streamHandler; this.responseHandlerHelper = new ResponseHandlerHelper(responseBuilder); } @@ -66,7 +71,7 @@ public void onResponseHeaders(HttpStreamBase stream, int responseStatusCode, int // Propagate cancellation requestCompletionFuture.exceptionally(t -> { - responseHandlerHelper.closeConnection(); + streamHandler.closeConnection(); return null; }); } @@ -76,7 +81,7 @@ public int onResponseBody(HttpStreamBase stream, byte[] bodyBytesIn) { if (inputStreamSubscriber == null) { inputStreamSubscriber = AbortableInputStreamSubscriber.builder() - .doAfterClose(() -> responseHandlerHelper.closeConnection()) + .doAfterClose(() -> streamHandler.closeConnection()) .build(); simplePublisher.subscribe(inputStreamSubscriber); // For response with a payload, we need to complete the future here to allow downstream to retrieve the data from @@ -97,10 +102,10 @@ public int onResponseBody(HttpStreamBase stream, byte[] bodyBytesIn) { log.debug(() -> "The subscriber failed to receive the data, closing the connection and failing the future", failure); requestCompletionFuture.completeExceptionally(failure); - responseHandlerHelper.closeConnection(); + streamHandler.closeConnection(); return; } - responseHandlerHelper.incrementWindow(bodyBytesIn.length); + streamHandler.incrementWindow(bodyBytesIn.length); }); // Window will be incremented after the subscriber consumes the data, returning 0 here to disable it. @@ -120,16 +125,15 @@ private void onFailedResponseComplete(int errorCode) { Throwable toThrow = wrapWithIoExceptionIfRetryable(new HttpException(errorCode)); simplePublisher.error(toThrow); requestCompletionFuture.completeExceptionally(toThrow); - responseHandlerHelper.closeConnection(); + streamHandler.closeConnection(); } private void onSuccessfulResponseComplete() { // For response without a payload, for example, S3 PutObjectResponse, we need to complete the future // in onResponseComplete callback since onResponseBody will never be invoked. - requestCompletionFuture.complete(responseBuilder.build()); // requestCompletionFuture has been completed at this point, no need to notify the future simplePublisher.complete(); - responseHandlerHelper.releaseConnection(); + streamHandler.releaseConnection(); } } diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/ResponseHandlerHelper.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/ResponseHandlerHelper.java index b90b43210472..4c9d4813f5cf 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/ResponseHandlerHelper.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/ResponseHandlerHelper.java @@ -30,20 +30,12 @@ public class ResponseHandlerHelper { private final SdkHttpResponse.Builder responseBuilder; - private HttpStreamBase stream; - private boolean streamClosed; - private final Object streamLock = new Object(); public ResponseHandlerHelper(SdkHttpResponse.Builder responseBuilder) { this.responseBuilder = responseBuilder; } public void onResponseHeaders(HttpStreamBase stream, int responseStatusCode, int headerType, HttpHeader[] nextHeaders) { - synchronized (streamLock) { - if (this.stream == null) { - this.stream = stream; - } - } if (headerType == HttpHeaderBlock.MAIN.getValue()) { for (HttpHeader h : nextHeaders) { responseBuilder.appendHeader(h.getName(), h.getValue()); @@ -51,40 +43,4 @@ public void onResponseHeaders(HttpStreamBase stream, int responseStatusCode, int responseBuilder.statusCode(responseStatusCode); } } - - public void incrementWindow(int windowSize) { - synchronized (streamLock) { - if (!streamClosed && stream != null) { - stream.incrementWindow(windowSize); - } - } - } - - /** - * Release the connection back to the pool so that it may be reused. This should be called when the request - * completes successfully and the response has been fully consumed. - */ - public void releaseConnection() { - synchronized (streamLock) { - if (!streamClosed && stream != null) { - streamClosed = true; - stream.close(); - } - } - } - - /** - * Cancel and close the stream, forcing the underlying connection to shut down rather than be returned to the - * connection pool. This should be called on error paths or when the stream is aborted before the response is - * fully consumed. {@code cancel()} must be invoked before {@code close()} per the CRT contract. - */ - public void closeConnection() { - synchronized (streamLock) { - if (!streamClosed && stream != null) { - streamClosed = true; - stream.cancel(); - stream.close(); - } - } - } } diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/BaseHttpStreamResponseHandlerTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/BaseHttpStreamResponseHandlerTest.java index 9318f6af228a..959e4286d2c8 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/BaseHttpStreamResponseHandlerTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/BaseHttpStreamResponseHandlerTest.java @@ -25,22 +25,24 @@ import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicBoolean; import org.apache.commons.lang3.RandomStringUtils; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; -import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.stubbing.Answer; import software.amazon.awssdk.crt.http.HttpException; import software.amazon.awssdk.crt.http.HttpHeader; import software.amazon.awssdk.crt.http.HttpHeaderBlock; import software.amazon.awssdk.crt.http.HttpStream; import software.amazon.awssdk.crt.http.HttpStreamBaseResponseHandler; import software.amazon.awssdk.utils.async.SimplePublisher; +import software.amazon.awssdk.utils.async.SimplePublisher; @ExtendWith(MockitoExtension.class) public abstract class BaseHttpStreamResponseHandlerTest { @@ -54,14 +56,28 @@ public abstract class BaseHttpStreamResponseHandlerTest { HttpStreamBaseResponseHandler responseHandler; - abstract HttpStreamBaseResponseHandler responseHandler(); + CrtStreamHandler streamHandler; + + abstract HttpStreamBaseResponseHandler responseHandler(CrtStreamHandler streamHandler); - abstract HttpStreamBaseResponseHandler responseHandlerWithMockedPublisher(SimplePublisher simplePublisher); + abstract HttpStreamBaseResponseHandler responseHandlerWithMockedPublisher(SimplePublisher simplePublisher, + CrtStreamHandler streamHandler); @BeforeEach public void setUp() { requestFuture = new CompletableFuture<>(); - responseHandler = responseHandler(); + + // Simulate CRT refcount behavior: isNull() returns true after close() + AtomicBoolean closed = new AtomicBoolean(false); + Mockito.lenient().when(httpStream.isNull()).thenAnswer(invocation -> closed.get()); + Mockito.lenient().doAnswer((Answer) invocation -> { + closed.set(true); + return null; + }).when(httpStream).close(); + + streamHandler = new CrtStreamHandler(); + streamHandler.setStream(httpStream); + responseHandler = responseHandler(streamHandler); } @Test @@ -98,9 +114,8 @@ void failedToGetResponse_shouldCancelAndCloseStream() { responseHandler.onResponseComplete(httpStream, 1); assertThatThrownBy(() -> requestFuture.join()).hasRootCauseInstanceOf(HttpException.class); - InOrder inOrder = Mockito.inOrder(httpStream); - inOrder.verify(httpStream).cancel(); - inOrder.verify(httpStream).close(); + verify(httpStream).cancel(); + verify(httpStream, Mockito.atLeastOnce()).close(); } @Test @@ -123,7 +138,7 @@ void publisherWritesFutureFails_shouldCancelAndCloseStream() { CompletableFuture future = new CompletableFuture<>(); when(simplePublisher.send(any(ByteBuffer.class))).thenReturn(future); - HttpStreamBaseResponseHandler handler = responseHandlerWithMockedPublisher(simplePublisher); + HttpStreamBaseResponseHandler handler = responseHandlerWithMockedPublisher(simplePublisher, streamHandler); HttpHeader[] httpHeaders = getHttpHeaders(); handler.onResponseHeaders(httpStream, 200, HttpHeaderBlock.MAIN.getValue(), @@ -140,9 +155,8 @@ void publisherWritesFutureFails_shouldCancelAndCloseStream() { // we don't verify here because it behaves differently in async and sync } - InOrder inOrder = Mockito.inOrder(httpStream); - inOrder.verify(httpStream).cancel(); - inOrder.verify(httpStream).close(); + verify(httpStream).cancel(); + verify(httpStream, Mockito.atLeastOnce()).close(); verify(httpStream, never()).incrementWindow(anyInt()); } @@ -152,7 +166,7 @@ void publisherWritesFutureCompletesAfterStreamClosed_shouldNotInvokeIncrementWin when(simplePublisher.send(any(ByteBuffer.class))).thenReturn(future); when(simplePublisher.complete()).thenReturn(future); - HttpStreamBaseResponseHandler handler = responseHandlerWithMockedPublisher(simplePublisher); + HttpStreamBaseResponseHandler handler = responseHandlerWithMockedPublisher(simplePublisher, streamHandler); HttpHeader[] httpHeaders = getHttpHeaders(); @@ -166,7 +180,7 @@ void publisherWritesFutureCompletesAfterStreamClosed_shouldNotInvokeIncrementWin future.complete(null); requestFuture.join(); - verify(httpStream).close(); + verify(httpStream, Mockito.atLeastOnce()).close(); verify(httpStream, never()).incrementWindow(anyInt()); } @@ -176,7 +190,7 @@ void publisherWritesFutureCompletesBeforeStreamClosed_shouldInvokeIncrementWindo when(simplePublisher.send(any(ByteBuffer.class))).thenReturn(future); when(simplePublisher.complete()).thenReturn(future); - HttpStreamBaseResponseHandler handler = responseHandlerWithMockedPublisher(simplePublisher); + HttpStreamBaseResponseHandler handler = responseHandlerWithMockedPublisher(simplePublisher, streamHandler); HttpHeader[] httpHeaders = getHttpHeaders(); @@ -191,7 +205,7 @@ void publisherWritesFutureCompletesBeforeStreamClosed_shouldInvokeIncrementWindo handler.onResponseComplete(httpStream, 0); requestFuture.join(); verify(httpStream).incrementWindow(anyInt()); - verify(httpStream).close(); + verify(httpStream, Mockito.atLeastOnce()).close(); } static HttpHeader[] getHttpHeaders() { diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java index 87a2849edc7b..a27e778cd265 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java @@ -87,7 +87,7 @@ public void execute_requestConversionFails_failsFuture() { .request(HttpExecuteRequest.builder().build()) .build(); - CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); + CompletableFuture executeFuture = requestExecutor.execute(context, new CrtStreamHandler()); assertThat(executeFuture).hasFailedWithThrowableThat().isInstanceOf(NullPointerException.class); } @@ -102,7 +102,7 @@ public void execute_acquireStreamFails_wrapsWithIOException() { .thenReturn(completableFuture); completableFuture.completeExceptionally(exception); - CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); + CompletableFuture executeFuture = requestExecutor.execute(context, new CrtStreamHandler()); assertThat(executeFuture).hasFailedWithThrowableThat().hasCause(exception).isInstanceOf(IOException.class); } @@ -116,7 +116,7 @@ public void execute_retryableException_wrapsWithIOException(Throwable throwable) Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequest.class), Mockito.any(HttpStreamBaseResponseHandler.class), Mockito.anyBoolean())) .thenReturn(completableFuture); - CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); + CompletableFuture executeFuture = requestExecutor.execute(context, new CrtStreamHandler()); assertThat(executeFuture).hasFailedWithThrowableThat().hasCause(throwable).isInstanceOf(IOException.class); } @@ -133,7 +133,7 @@ public void execute_httpException_mapsToCorrectException(Entry executeFuture = requestExecutor.execute(context).responseFuture(); + CompletableFuture executeFuture = requestExecutor.execute(context, new CrtStreamHandler()); assertThatThrownBy(executeFuture::join).hasCauseInstanceOf(expectedExceptionClass); } @@ -146,7 +146,7 @@ public void execute_nonRetryableHttpException_doesNotWrapWithIOException() { Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequest.class), Mockito.any(HttpStreamBaseResponseHandler.class), Mockito.anyBoolean())) .thenReturn(completableFuture); - CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); + CompletableFuture executeFuture = requestExecutor.execute(context, new CrtStreamHandler()); assertThatThrownBy(executeFuture::join).hasCause(exception); } diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtResponseHandlerTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtResponseHandlerTest.java index e1caaeb021a9..aba67504c5a0 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtResponseHandlerTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtResponseHandlerTest.java @@ -33,34 +33,36 @@ import software.amazon.awssdk.crt.http.HttpStreamBaseResponseHandler; import software.amazon.awssdk.http.SdkHttpResponse; import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; +import software.amazon.awssdk.http.crt.internal.CrtStreamHandler; import software.amazon.awssdk.http.crt.internal.response.CrtResponseAdapter; import software.amazon.awssdk.utils.async.SimplePublisher; public class CrtResponseHandlerTest extends BaseHttpStreamResponseHandlerTest { @Override - HttpStreamBaseResponseHandler responseHandler() { + HttpStreamBaseResponseHandler responseHandler(CrtStreamHandler streamHandler) { AsyncResponseHandler responseHandler = new AsyncResponseHandler<>((response, executionAttributes) -> null, Function.identity(), new ExecutionAttributes()); responseHandler.prepare(); - return CrtResponseAdapter.toCrtResponseHandler(requestFuture, responseHandler); + return CrtResponseAdapter.toCrtResponseHandler(requestFuture, responseHandler, streamHandler); } @Override - HttpStreamBaseResponseHandler responseHandlerWithMockedPublisher(SimplePublisher simplePublisher) { + HttpStreamBaseResponseHandler responseHandlerWithMockedPublisher(SimplePublisher simplePublisher, + CrtStreamHandler streamHandler) { AsyncResponseHandler responseHandler = new AsyncResponseHandler<>((response, executionAttributes) -> null, Function.identity(), new ExecutionAttributes()); responseHandler.prepare(); - return new CrtResponseAdapter(requestFuture, responseHandler, simplePublisher); + return new CrtResponseAdapter(requestFuture, responseHandler, simplePublisher, streamHandler); } @Test void onResponseComplete_publisherCancelled_closesStream() { SdkAsyncHttpResponseHandler responseHandler = new TestAsyncHttpResponseHandler(); - HttpStreamBaseResponseHandler crtResponseHandler = CrtResponseAdapter.toCrtResponseHandler(requestFuture, responseHandler); + HttpStreamBaseResponseHandler crtResponseHandler = CrtResponseAdapter.toCrtResponseHandler(requestFuture, responseHandler, streamHandler); HttpHeader[] httpHeaders = getHttpHeaders(); crtResponseHandler.onResponseHeaders(httpStream, 200, HttpHeaderBlock.MAIN.getValue(), httpHeaders); diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandlerTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandlerTest.java new file mode 100644 index 000000000000..6ac5d39f7d2e --- /dev/null +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandlerTest.java @@ -0,0 +1,90 @@ +/* + * 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.internal; + +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; + +import java.util.concurrent.atomic.AtomicBoolean; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.stubbing.Answer; +import software.amazon.awssdk.crt.http.HttpStreamBase; + +@ExtendWith(MockitoExtension.class) +class CrtStreamHandlerTest { + + @Mock + private HttpStreamBase stream; + + private CrtStreamHandler streamHandler; + + @BeforeEach + void setUp() { + AtomicBoolean closed = new AtomicBoolean(false); + Mockito.lenient().when(stream.isNull()).thenAnswer(invocation -> closed.get()); + Mockito.lenient().doAnswer((Answer) invocation -> { + closed.set(true); + return null; + }).when(stream).close(); + + streamHandler = new CrtStreamHandler(); + streamHandler.setStream(stream); + } + + @Test + void releaseConnection_shouldCallClose() { + streamHandler.releaseConnection(); + + verify(stream, never()).cancel(); + verify(stream).close(); + } + + @Test + void closeConnection_shouldCallCancelAndClose() { + streamHandler.closeConnection(); + + verify(stream).cancel(); + verify(stream, Mockito.atLeastOnce()).close(); + } + + @Test + void incrementWindow_afterReleaseConnection_shouldBeNoOp() { + streamHandler.releaseConnection(); + streamHandler.incrementWindow(1024); + + verify(stream, never()).incrementWindow(1024); + } + + @Test + void incrementWindow_afterCloseConnection_shouldBeNoOp() { + streamHandler.closeConnection(); + streamHandler.incrementWindow(1024); + + verify(stream, never()).incrementWindow(1024); + } + + @Test + void incrementWindow_beforeClose_shouldWork() { + streamHandler.incrementWindow(1024); + + verify(stream).incrementWindow(1024); + } +} diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/InputStreamAdaptingHttpStreamResponseHandlerTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/InputStreamAdaptingHttpStreamResponseHandlerTest.java index 8c786624426b..2321ee0c8809 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/InputStreamAdaptingHttpStreamResponseHandlerTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/InputStreamAdaptingHttpStreamResponseHandlerTest.java @@ -31,19 +31,21 @@ import software.amazon.awssdk.crt.http.HttpStreamBaseResponseHandler; import software.amazon.awssdk.http.AbortableInputStream; import software.amazon.awssdk.http.SdkHttpFullResponse; +import software.amazon.awssdk.http.crt.internal.CrtStreamHandler; import software.amazon.awssdk.http.crt.internal.response.InputStreamAdaptingHttpStreamResponseHandler; import software.amazon.awssdk.utils.async.SimplePublisher; public class InputStreamAdaptingHttpStreamResponseHandlerTest extends BaseHttpStreamResponseHandlerTest { @Override - HttpStreamBaseResponseHandler responseHandler() { - return new InputStreamAdaptingHttpStreamResponseHandler(requestFuture); + HttpStreamBaseResponseHandler responseHandler(CrtStreamHandler streamHandler) { + return new InputStreamAdaptingHttpStreamResponseHandler(requestFuture, streamHandler); } @Override - HttpStreamBaseResponseHandler responseHandlerWithMockedPublisher(SimplePublisher simplePublisher) { - return new InputStreamAdaptingHttpStreamResponseHandler(requestFuture, simplePublisher); + HttpStreamBaseResponseHandler responseHandlerWithMockedPublisher(SimplePublisher simplePublisher, + CrtStreamHandler streamHandler) { + return new InputStreamAdaptingHttpStreamResponseHandler(requestFuture, simplePublisher, streamHandler); } @Test @@ -63,9 +65,8 @@ void abortStream_shouldCancelAndCloseStream() throws IOException { abortableInputStream.read(); abortableInputStream.abort(); - InOrder inOrder = Mockito.inOrder(httpStream); - inOrder.verify(httpStream).cancel(); - inOrder.verify(httpStream).close(); + verify(httpStream).cancel(); + verify(httpStream, Mockito.atLeastOnce()).close(); } @Test @@ -85,7 +86,7 @@ void closeStream_shouldCloseStreamWithoutCancel() throws IOException { abortableInputStream.read(); abortableInputStream.close(); - verify(httpStream).close(); + verify(httpStream, Mockito.atLeastOnce()).close(); } @Test @@ -97,8 +98,7 @@ void cancelFuture_shouldCancelAndCloseStream() { requestFuture.completeExceptionally(new RuntimeException()); - InOrder inOrder = Mockito.inOrder(httpStream); - inOrder.verify(httpStream).cancel(); - inOrder.verify(httpStream).close(); + verify(httpStream).cancel(); + verify(httpStream, Mockito.atLeastOnce()).close(); } } diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/ResponseHandlerHelperTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/ResponseHandlerHelperTest.java deleted file mode 100644 index 1714b9930ec4..000000000000 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/ResponseHandlerHelperTest.java +++ /dev/null @@ -1,124 +0,0 @@ -/* - * 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.internal; - -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; - -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.extension.ExtendWith; -import org.mockito.InOrder; -import org.mockito.Mock; -import org.mockito.Mockito; -import org.mockito.junit.jupiter.MockitoExtension; -import software.amazon.awssdk.crt.http.HttpHeader; -import software.amazon.awssdk.crt.http.HttpHeaderBlock; -import software.amazon.awssdk.crt.http.HttpStreamBase; -import software.amazon.awssdk.http.SdkHttpResponse; -import software.amazon.awssdk.http.crt.internal.response.ResponseHandlerHelper; - -@ExtendWith(MockitoExtension.class) -class ResponseHandlerHelperTest { - - @Mock - private HttpStreamBase stream; - - private ResponseHandlerHelper helper; - - @BeforeEach - void setUp() { - helper = new ResponseHandlerHelper(SdkHttpResponse.builder()); - // Register the stream via onResponseHeaders - HttpHeader[] headers = { new HttpHeader("Content-Length", "1") }; - helper.onResponseHeaders(stream, 200, HttpHeaderBlock.MAIN.getValue(), headers); - } - - @Test - void releaseConnection_shouldOnlyCallClose() { - helper.releaseConnection(); - - verify(stream, never()).cancel(); - verify(stream).close(); - } - - @Test - void closeConnection_shouldCallCancelThenClose() { - helper.closeConnection(); - - InOrder inOrder = Mockito.inOrder(stream); - inOrder.verify(stream).cancel(); - inOrder.verify(stream).close(); - } - - @Test - void releaseConnection_calledTwice_shouldOnlyCloseOnce() { - helper.releaseConnection(); - helper.releaseConnection(); - - verify(stream, Mockito.times(1)).close(); - } - - @Test - void closeConnection_calledTwice_shouldOnlyCloseOnce() { - helper.closeConnection(); - helper.closeConnection(); - - verify(stream, Mockito.times(1)).cancel(); - verify(stream, Mockito.times(1)).close(); - } - - @Test - void releaseConnection_afterCloseConnection_shouldBeNoOp() { - helper.closeConnection(); - helper.releaseConnection(); - - verify(stream, Mockito.times(1)).cancel(); - verify(stream, Mockito.times(1)).close(); - } - - @Test - void closeConnection_afterReleaseConnection_shouldBeNoOp() { - helper.releaseConnection(); - helper.closeConnection(); - - verify(stream, never()).cancel(); - verify(stream, Mockito.times(1)).close(); - } - - @Test - void incrementWindow_afterReleaseConnection_shouldBeNoOp() { - helper.releaseConnection(); - helper.incrementWindow(1024); - - verify(stream, never()).incrementWindow(1024); - } - - @Test - void incrementWindow_afterCloseConnection_shouldBeNoOp() { - helper.closeConnection(); - helper.incrementWindow(1024); - - verify(stream, never()).incrementWindow(1024); - } - - @Test - void incrementWindow_beforeClose_shouldWork() { - helper.incrementWindow(1024); - - verify(stream).incrementWindow(1024); - } -} From d25560d3da5db87d2647cb5366627c7cc0d6cb59 Mon Sep 17 00:00:00 2001 From: Zoe Wang <33073555+zoewangg@users.noreply.github.com> Date: Thu, 21 May 2026 12:38:15 -0700 Subject: [PATCH 3/7] Update CRT version --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 1848cf1dd892..971a3b04f13b 100644 --- a/pom.xml +++ b/pom.xml @@ -131,7 +131,7 @@ 3.1.5 1.17.1 1.37 - 1.0.0-SNAPSHOT + 0.45.4 5.10.3 From f722e5dcd4a99a9b551100e276d56f5211475614 Mon Sep 17 00:00:00 2001 From: Zoe Wang <33073555+zoewangg@users.noreply.github.com> Date: Fri, 22 May 2026 14:20:05 -0700 Subject: [PATCH 4/7] Update CRT version --- pom.xml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pom.xml b/pom.xml index 971a3b04f13b..1094cc59533d 100644 --- a/pom.xml +++ b/pom.xml @@ -131,7 +131,7 @@ 3.1.5 1.17.1 1.37 - 0.45.4 + 0.46.0 5.10.3 From 0b4a1bfbb36c40fccd789c4579710251caf9cf5f Mon Sep 17 00:00:00 2001 From: Zoe Wang <33073555+zoewangg@users.noreply.github.com> Date: Tue, 26 May 2026 16:07:53 -0700 Subject: [PATCH 5/7] Use async stream API in async code path --- http-clients/aws-crt-client/pom.xml | 26 ++ .../awssdk/http/crt/AwsCrtHttpClient.java | 10 +- .../crt/internal/CrtAsyncRequestExecutor.java | 75 ++-- .../http/crt/internal/CrtRequestExecutor.java | 77 +++- .../http/crt/internal/CrtStreamHandler.java | 112 ++++-- .../ResponseHandlerErrorNotifier.java | 57 +++ .../internal/request/CrtRequestAdapter.java | 4 +- .../request/CrtRequestBodyAdapter.java | 50 --- .../CrtRequestBodyPublisherSubscriber.java | 120 ++++++ .../internal/response/CrtResponseAdapter.java | 34 +- .../BaseHttpStreamResponseHandlerTest.java | 3 +- .../internal/CrtAsyncRequestExecutorTest.java | 361 +++++++++++++++++- .../crt/internal/CrtRequestExecutorTest.java | 48 ++- .../crt/internal/CrtResponseHandlerTest.java | 10 +- .../crt/internal/CrtStreamHandlerTest.java | 83 +++- ...RequestBodyPublisherSubscriberTckTest.java | 68 ++++ pom.xml | 2 +- 17 files changed, 943 insertions(+), 197 deletions(-) create mode 100644 http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/ResponseHandlerErrorNotifier.java delete mode 100644 http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyAdapter.java create mode 100644 http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriber.java create mode 100644 http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriberTckTest.java diff --git a/http-clients/aws-crt-client/pom.xml b/http-clients/aws-crt-client/pom.xml index 8f22dcb96e31..47516d1140c8 100644 --- a/http-clients/aws-crt-client/pom.xml +++ b/http-clients/aws-crt-client/pom.xml @@ -213,6 +213,32 @@ + + + org.apache.maven.plugins + maven-surefire-plugin + ${maven.surefire.version} + + + + junit + false + + + + + + org.apache.maven.surefire + surefire-junit-platform + ${maven.surefire.version} + + + org.apache.maven.surefire + surefire-testng + ${maven.surefire.version} + + + 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 d81c286fc5f4..87bb65cf05bd 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 @@ -126,13 +126,9 @@ public HttpExecuteResponse call() throws IOException { HttpExecuteResponse.Builder builder = HttpExecuteResponse.builder(); try { - CrtStreamHandler streamHandler = new CrtStreamHandler(); - responseFuture = new CrtRequestExecutor().execute(context, streamHandler); - - // Write the request body from the caller thread via the stream handler, - // which guards against concurrent stream close with a synchronized block. - // This avoids blocking the CRT event loop thread in InputStream.read(). - writeRequestBody(streamHandler); + CrtRequestExecutor.Result result = new CrtRequestExecutor().execute(context); + responseFuture = result.responseFuture(); + writeRequestBody(result.streamHandler()); SdkHttpFullResponse response = CompletableFutureUtils.joinInterruptibly(responseFuture); builder.response(response); diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java index e3dfe024e9cd..91cbbaff243b 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java @@ -26,25 +26,23 @@ import software.amazon.awssdk.crt.http.HttpStreamBaseResponseHandler; import software.amazon.awssdk.http.SdkCancellationException; import software.amazon.awssdk.http.async.AsyncExecuteRequest; -import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; +import software.amazon.awssdk.http.crt.internal.request.CrtRequestBodyPublisherSubscriber; import software.amazon.awssdk.http.crt.internal.response.CrtResponseAdapter; import software.amazon.awssdk.metrics.MetricCollector; import software.amazon.awssdk.metrics.NoOpMetricCollector; -import software.amazon.awssdk.utils.Logger; @SdkInternalApi public final class CrtAsyncRequestExecutor { - private static final Logger log = Logger.loggerFor(CrtAsyncRequestExecutor.class); - public CompletableFuture execute(CrtAsyncRequestContext executionContext) { AsyncExecuteRequest asyncRequest = executionContext.sdkRequest(); CompletableFuture requestFuture = createAsyncExecutionFuture(asyncRequest); + ResponseHandlerErrorNotifier errorNotifier = new ResponseHandlerErrorNotifier(asyncRequest.responseHandler()); try { - doExecute(executionContext, asyncRequest, requestFuture); + doExecute(executionContext, asyncRequest, requestFuture, errorNotifier); } catch (Throwable t) { - reportAsyncFailure(t, requestFuture, asyncRequest.responseHandler()); + reportAsyncFailure(t, requestFuture, errorNotifier); } return requestFuture; @@ -52,7 +50,8 @@ public CompletableFuture execute(CrtAsyncRequestContext executionContext) private void doExecute(CrtAsyncRequestContext executionContext, AsyncExecuteRequest asyncRequest, - CompletableFuture requestFuture) { + CompletableFuture requestFuture, + ResponseHandlerErrorNotifier errorNotifier) { MetricCollector metricCollector = executionContext.metricCollector(); boolean shouldPublishMetrics = metricCollector != null && !(metricCollector instanceof NoOpMetricCollector); @@ -67,28 +66,48 @@ private void doExecute(CrtAsyncRequestContext executionContext, HttpRequestBase crtRequest = toAsyncCrtRequest(executionContext); - CrtStreamHandler streamHandler = new CrtStreamHandler(); + CompletableFuture streamFuture = new CompletableFuture<>(); + CrtStreamHandler streamHandler = new CrtStreamHandler(streamFuture); HttpStreamBaseResponseHandler crtResponseHandler = - CrtResponseAdapter.toCrtResponseHandler(requestFuture, asyncRequest.responseHandler(), streamHandler); + CrtResponseAdapter.toCrtResponseHandler(requestFuture, asyncRequest.responseHandler(), streamHandler, errorNotifier); - CompletableFuture streamFuture = - executionContext.streamManager().acquireStream(crtRequest, crtResponseHandler); + CrtRequestBodyPublisherSubscriber bodySubscriber = + new CrtRequestBodyPublisherSubscriber(streamHandler, requestFuture, errorNotifier); long finalAcquireStartTime = acquireStartTime; - streamFuture.whenComplete((stream, throwable) -> { - if (shouldPublishMetrics) { - reportMetrics(executionContext.streamManager(), metricCollector, finalAcquireStartTime); - } + executionContext.streamManager().acquireStream(crtRequest, crtResponseHandler, true) + .handle((stream, throwable) -> { + if (shouldPublishMetrics) { + reportMetrics(executionContext.streamManager(), metricCollector, finalAcquireStartTime); + } + + if (throwable != null) { + handleAcquireFailure(throwable, streamFuture, requestFuture, errorNotifier); + return null; + } + try { + stream.activate(); + streamFuture.complete(stream); + asyncRequest.requestContentPublisher().subscribe(bodySubscriber); + } catch (Throwable t) { + handleAcquireFailure(t, streamFuture, requestFuture, errorNotifier); + } + return null; + }).exceptionally(t -> { + handleAcquireFailure(t, streamFuture, requestFuture, errorNotifier); + return null; + }); + } - if (throwable != null) { - Throwable toThrow = wrapCrtException(throwable); - reportAsyncFailure(toThrow, requestFuture, asyncRequest.responseHandler()); - } else { - streamHandler.setStream(stream); - } - }); + private void handleAcquireFailure(Throwable t, + CompletableFuture streamFuture, + CompletableFuture requestFuture, + ResponseHandlerErrorNotifier errorNotifier) { + Throwable toThrow = wrapCrtException(t); + streamFuture.completeExceptionally(toThrow); + reportAsyncFailure(toThrow, requestFuture, errorNotifier); } /** @@ -113,18 +132,10 @@ private CompletableFuture createAsyncExecutionFuture(AsyncExecuteRequest r return future; } - /** - * Notify the provided response handler and future of the failure. - */ private void reportAsyncFailure(Throwable cause, CompletableFuture executeFuture, - SdkAsyncHttpResponseHandler responseHandler) { - try { - responseHandler.onError(cause); - } catch (Exception e) { - log.error(() -> "SdkAsyncHttpResponseHandler " + responseHandler + " threw an exception in onError. It will be " - + "ignored.", e); - } + ResponseHandlerErrorNotifier errorNotifier) { + errorNotifier.tryNotifyError(cause); executeFuture.completeExceptionally(cause); } } diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java index 98ace2a65766..aff574635ff8 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java @@ -31,22 +31,26 @@ @SdkInternalApi public final class CrtRequestExecutor { - public CompletableFuture execute(CrtRequestContext executionContext, - CrtStreamHandler streamHandler) { + public Result execute(CrtRequestContext executionContext) { CompletableFuture responseFuture = new CompletableFuture<>(); + CompletableFuture streamFuture = new CompletableFuture<>(); + CrtStreamHandler streamHandler = new CrtStreamHandler(streamFuture); try { - doExecute(executionContext, responseFuture, streamHandler); + doExecute(executionContext, responseFuture, streamHandler, streamFuture); } catch (Throwable t) { + // Fail streamFuture too so any caller blocked in waitForStream() unblocks. + streamFuture.completeExceptionally(t); responseFuture.completeExceptionally(t); } - return responseFuture; + return new Result(responseFuture, streamHandler); } private void doExecute(CrtRequestContext executionContext, CompletableFuture responseFuture, - CrtStreamHandler streamHandler) { + CrtStreamHandler streamHandler, + CompletableFuture streamFuture) { MetricCollector metricCollector = executionContext.metricCollector(); boolean shouldPublishMetrics = metricCollector != null && !(metricCollector instanceof NoOpMetricCollector); @@ -63,22 +67,55 @@ private void doExecute(CrtRequestContext executionContext, boolean hasBody = executionContext.sdkRequest().contentStreamProvider().isPresent(); - CompletableFuture streamFuture = - executionContext.streamManager().acquireStream(crtRequest, crtResponseHandler, hasBody); - long finalAcquireStartTime = acquireStartTime; - streamFuture.whenComplete((streamBase, throwable) -> { - if (shouldPublishMetrics) { - reportMetrics(executionContext.streamManager(), metricCollector, finalAcquireStartTime); - } - - if (throwable != null) { - Throwable toThrow = wrapCrtException(throwable); - responseFuture.completeExceptionally(toThrow); - } else { - streamHandler.setStream(streamBase); - } - }); + executionContext.streamManager().acquireStream(crtRequest, crtResponseHandler, hasBody) + .handle((streamBase, throwable) -> { + if (shouldPublishMetrics) { + reportMetrics(executionContext.streamManager(), metricCollector, finalAcquireStartTime); + } + + if (throwable != null) { + handleAcquireFailure(throwable, streamFuture, responseFuture); + return null; + } + try { + streamBase.activate(); + streamFuture.complete(streamBase); + } catch (Throwable t) { + handleAcquireFailure(t, streamFuture, responseFuture); + } + return null; + }).exceptionally(t -> { + handleAcquireFailure(t, streamFuture, responseFuture); + return null; + }); + } + + private void handleAcquireFailure(Throwable t, + CompletableFuture streamFuture, + CompletableFuture responseFuture) { + Throwable toThrow = wrapCrtException(t); + streamFuture.completeExceptionally(toThrow); + responseFuture.completeExceptionally(toThrow); + } + + @SdkInternalApi + public static final class Result { + private final CompletableFuture responseFuture; + private final CrtStreamHandler streamHandler; + + private Result(CompletableFuture responseFuture, CrtStreamHandler streamHandler) { + this.responseFuture = responseFuture; + this.streamHandler = streamHandler; + } + + public CompletableFuture responseFuture() { + return responseFuture; + } + + public CrtStreamHandler streamHandler() { + return streamHandler; + } } } diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandler.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandler.java index 97bdcd8c6f56..ab4fd2d6040c 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandler.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandler.java @@ -17,72 +17,98 @@ import java.io.IOException; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.CountDownLatch; +import java.util.concurrent.CompletionException; import software.amazon.awssdk.annotations.SdkInternalApi; import software.amazon.awssdk.crt.http.HttpStreamBase; +import software.amazon.awssdk.utils.CompletableFutureUtils; /** * Manages the lifecycle of a CRT HTTP stream, providing thread-safe access to stream operations. * Shared between the request executor (for writing body data) and the response handler (for * incrementing the window and releasing/closing the connection). + * + *

The handler is constructed with a {@link CompletableFuture} representing stream acquisition. + * The caller (request executor) completes that future once the underlying CRT stream manager has + * either acquired the stream or failed. All operations on this handler chain off that future, so + * writes issued before acquisition completes are queued. */ @SdkInternalApi public final class CrtStreamHandler { private final Object streamLock = new Object(); - private final CountDownLatch streamLatch = new CountDownLatch(1); - private HttpStreamBase stream; + private final CompletableFuture streamFuture; private boolean streamClosed; - /** - * Sets the stream. Called once when the stream is acquired from the connection pool. - */ - public void setStream(HttpStreamBase stream) { - this.stream = stream; - streamLatch.countDown(); + public CrtStreamHandler(CompletableFuture streamFuture) { + this.streamFuture = streamFuture; } /** - * Blocks until the stream has been acquired. + * Blocks until the stream has been acquired or acquisition has failed. Returns the acquired + * stream on success. If acquisition failed, the failure cause is rethrown wrapped in a + * {@link CompletionException} so callers can use the same handling as for response futures. */ - public void waitForStream() { - try { - streamLatch.await(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - throw new RuntimeException("Interrupted while waiting for stream", e); - } + public HttpStreamBase waitForStream() { + return CompletableFutureUtils.joinInterruptibly(streamFuture); } /** - * Write data to the stream. The caller must ensure the stream is ready (via {@link #waitForStream()}) - * before calling this method. + * Write data to the stream. The returned future chains on stream acquisition: if the stream + * is not yet ready, the write is queued until the {@code streamFuture} passed to the + * constructor completes. Failures from either stream acquisition or the underlying CRT write + * are propagated as the original cause (not wrapped in {@link CompletionException}) so callers + * see the same exception type whether the failure happens before or after {@code thenCompose}- + * style chaining. */ public CompletableFuture writeData(byte[] data, boolean endStream) { - if (streamLatch.getCount() != 0) { - CompletableFuture future = new CompletableFuture<>(); - future.completeExceptionally( - new IllegalStateException("writeData called before stream is ready. Call waitForStream() first.")); - return future; - } - synchronized (streamLock) { - if (streamClosed) { - CompletableFuture future = new CompletableFuture<>(); - future.completeExceptionally( - new IOException("Stream is already closed, cannot write data.")); - return future; + CompletableFuture result = new CompletableFuture<>(); + streamFuture.handle((s, t) -> { + if (t != null) { + result.completeExceptionally(unwrap(t)); + return null; } - return stream.writeData(data, endStream); + doWrite(s, data, endStream, result); + return null; + }).exceptionally(t -> { + result.completeExceptionally(t); + closeConnection(); + return null; + }); + return result; + } + + private void doWrite(HttpStreamBase s, byte[] data, boolean endStream, CompletableFuture result) { + try { + CompletableFuture writeFuture; + synchronized (streamLock) { + if (streamClosed) { + result.completeExceptionally(new IOException("Stream is already closed, cannot write data.")); + return; + } + writeFuture = s.writeData(data, endStream); + } + writeFuture.whenComplete((v, err) -> { + if (err != null) { + result.completeExceptionally(unwrap(err)); + } else { + result.complete(null); + } + }); + } catch (Throwable th) { + result.completeExceptionally(th); + closeConnection(); } } + private static Throwable unwrap(Throwable t) { + return t instanceof CompletionException && t.getCause() != null ? t.getCause() : t; + } + public void incrementWindow(int windowSize) { - if (streamLatch.getCount() != 0) { - throw new IllegalStateException("incrementWindow called before stream is ready."); - } synchronized (streamLock) { - if (!streamClosed) { - stream.incrementWindow(windowSize); + HttpStreamBase s = streamFuture.getNow(null); + if (!streamClosed && s != null) { + s.incrementWindow(windowSize); } } } @@ -93,9 +119,10 @@ public void incrementWindow(int windowSize) { */ public void releaseConnection() { synchronized (streamLock) { - if (!streamClosed && stream != null) { + HttpStreamBase s = streamFuture.getNow(null); + if (!streamClosed && s != null) { streamClosed = true; - stream.close(); + s.close(); } } } @@ -107,10 +134,11 @@ public void releaseConnection() { */ public void closeConnection() { synchronized (streamLock) { - if (!streamClosed && stream != null) { + HttpStreamBase s = streamFuture.getNow(null); + if (!streamClosed && s != null) { streamClosed = true; - stream.cancel(); - stream.close(); + s.cancel(); + s.close(); } } } diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/ResponseHandlerErrorNotifier.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/ResponseHandlerErrorNotifier.java new file mode 100644 index 000000000000..94651087c0b8 --- /dev/null +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/ResponseHandlerErrorNotifier.java @@ -0,0 +1,57 @@ +/* + * 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.internal; + +import java.util.concurrent.atomic.AtomicBoolean; +import software.amazon.awssdk.annotations.SdkInternalApi; +import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; +import software.amazon.awssdk.utils.Logger; + +/** + * Coordinates at-most-once delivery of {@link SdkAsyncHttpResponseHandler#onError(Throwable)} across + * the multiple paths (response adapter, request body subscriber, executor failure paths) that may + * all attempt to notify the same handler. + */ +@SdkInternalApi +public final class ResponseHandlerErrorNotifier { + + private static final Logger log = Logger.loggerFor(ResponseHandlerErrorNotifier.class); + + private final SdkAsyncHttpResponseHandler responseHandler; + private final AtomicBoolean notified = new AtomicBoolean(false); + + public ResponseHandlerErrorNotifier(SdkAsyncHttpResponseHandler responseHandler) { + this.responseHandler = responseHandler; + } + + /** + * Atomically delivers {@code onError(t)} to the response handler at most once. Returns + * {@code true} if this caller delivered the notification, {@code false} if another caller + * already did. Exceptions thrown by the handler are caught and logged. + */ + public boolean tryNotifyError(Throwable t) { + if (!notified.compareAndSet(false, true)) { + return false; + } + try { + responseHandler.onError(t); + } catch (Exception e) { + log.error(() -> "SdkAsyncHttpResponseHandler " + responseHandler + " threw an exception in onError. It will be " + + "ignored.", e); + } + return true; + } +} diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestAdapter.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestAdapter.java index e97b1adb514e..f7fef41cfc4e 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestAdapter.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestAdapter.java @@ -50,14 +50,12 @@ public static HttpRequestBase toAsyncCrtRequest(CrtAsyncRequestContext request) .map(value -> "?" + value) .orElse(""); String path = encodedPath + encodedQueryString; - CrtRequestBodyAdapter crtRequestBodyAdapter = new CrtRequestBodyAdapter(sdkExecuteRequest.requestContentPublisher(), - request.readBufferSize()); HttpHeader[] crtHeaderArray = asArray(createAsyncHttpHeaderList(sdkRequest.getUri(), sdkExecuteRequest, request.protocol())); return new HttpRequest(method, path, crtHeaderArray, - crtRequestBodyAdapter); + null); } public static HttpRequest toCrtRequest(CrtRequestContext request) { diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyAdapter.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyAdapter.java deleted file mode 100644 index 6fa64d8a011d..000000000000 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyAdapter.java +++ /dev/null @@ -1,50 +0,0 @@ -/* - * 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.internal.request; - -import java.nio.ByteBuffer; -import java.util.concurrent.atomic.AtomicBoolean; -import software.amazon.awssdk.annotations.SdkInternalApi; -import software.amazon.awssdk.crt.http.HttpRequestBodyStream; -import software.amazon.awssdk.http.async.SdkHttpContentPublisher; -import software.amazon.awssdk.utils.async.ByteBufferStoringSubscriber; -import software.amazon.awssdk.utils.async.ByteBufferStoringSubscriber.TransferResult; - -@SdkInternalApi -final class CrtRequestBodyAdapter implements HttpRequestBodyStream { - private final SdkHttpContentPublisher requestPublisher; - private final ByteBufferStoringSubscriber requestBodySubscriber; - private final AtomicBoolean subscribed = new AtomicBoolean(false); - - CrtRequestBodyAdapter(SdkHttpContentPublisher requestPublisher, long readLimit) { - this.requestPublisher = requestPublisher; - this.requestBodySubscriber = new ByteBufferStoringSubscriber(readLimit); - } - - @Override - public boolean sendRequestBody(ByteBuffer bodyBytesOut) { - if (subscribed.compareAndSet(false, true)) { - requestPublisher.subscribe(requestBodySubscriber); - } - - return requestBodySubscriber.transferTo(bodyBytesOut) == TransferResult.END_OF_STREAM; - } - - @Override - public long getLength() { - return requestPublisher.contentLength().orElse(0L); - } -} diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriber.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriber.java new file mode 100644 index 000000000000..5bfcda2a0499 --- /dev/null +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriber.java @@ -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.internal.request; + +import java.nio.ByteBuffer; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicBoolean; +import org.reactivestreams.Subscriber; +import org.reactivestreams.Subscription; +import software.amazon.awssdk.annotations.SdkInternalApi; +import software.amazon.awssdk.http.crt.internal.CrtStreamHandler; +import software.amazon.awssdk.http.crt.internal.ResponseHandlerErrorNotifier; +import software.amazon.awssdk.utils.Validate; + +/** + * Subscriber that consumes the SDK request body publisher and pushes each chunk to the CRT stream + * via {@link CrtStreamHandler#writeData(byte[], boolean)}. Signals end-of-stream on {@code onComplete}, + * and tears down the stream on errors from either the publisher or CRT. + */ +@SdkInternalApi +public final class CrtRequestBodyPublisherSubscriber implements Subscriber { + + private final CrtStreamHandler streamHandler; + private final CompletableFuture executeFuture; + private final ResponseHandlerErrorNotifier errorNotifier; + private final AtomicBoolean terminated = new AtomicBoolean(false); + // Per Reactive Streams rule 2.7, calls to subscription.request and subscription.cancel must + // be performed serially. This lock provides the happens-before edge between calls made on + // different threads (publisher thread for onSubscribe; CRT event loop thread for whenComplete + // callbacks; either for handleError). + private final Object subscriptionLock = new Object(); + + private Subscription subscription; + + public CrtRequestBodyPublisherSubscriber(CrtStreamHandler streamHandler, + CompletableFuture executeFuture, + ResponseHandlerErrorNotifier errorNotifier) { + this.streamHandler = streamHandler; + this.executeFuture = executeFuture; + this.errorNotifier = errorNotifier; + } + + @Override + public void onSubscribe(Subscription s) { + Validate.paramNotNull(s, "s"); + synchronized (subscriptionLock) { + if (this.subscription != null) { + s.cancel(); + return; + } + this.subscription = s; + s.request(1); + } + } + + @Override + public void onNext(ByteBuffer buf) { + Validate.paramNotNull(buf, "buf"); + byte[] bytes = new byte[buf.remaining()]; + buf.get(bytes); + CompletableFuture writeFuture = streamHandler.writeData(bytes, false); + writeFuture.whenComplete((v, err) -> { + if (err != null) { + handleError(err, true); + } else { + requestNext(); + } + }); + } + + @Override + public void onComplete() { + streamHandler.writeData(null, true).whenComplete((v, err) -> { + if (err != null) { + handleError(err, false); + } + }); + } + + @Override + public void onError(Throwable t) { + Validate.paramNotNull(t, "t"); + handleError(t, false); + } + + private void requestNext() { + synchronized (subscriptionLock) { + subscription.request(1); + } + } + + private void handleError(Throwable t, boolean cancelSubscription) { + if (!terminated.compareAndSet(false, true)) { + return; + } + streamHandler.closeConnection(); + errorNotifier.tryNotifyError(t); + executeFuture.completeExceptionally(t); + if (cancelSubscription) { + synchronized (subscriptionLock) { + if (subscription != null) { + subscription.cancel(); + } + } + } + } +} diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/CrtResponseAdapter.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/CrtResponseAdapter.java index df44ab28cbe0..4023ae24372d 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/CrtResponseAdapter.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/CrtResponseAdapter.java @@ -31,6 +31,7 @@ import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; import software.amazon.awssdk.http.crt.AwsCrtAsyncHttpClient; import software.amazon.awssdk.http.crt.internal.CrtStreamHandler; +import software.amazon.awssdk.http.crt.internal.ResponseHandlerErrorNotifier; import software.amazon.awssdk.utils.Logger; import software.amazon.awssdk.utils.Validate; import software.amazon.awssdk.utils.async.SimplePublisher; @@ -50,38 +51,36 @@ public final class CrtResponseAdapter implements HttpStreamBaseResponseHandler { private final SdkHttpResponse.Builder responseBuilder; private final ResponseHandlerHelper responseHandlerHelper; private final CrtStreamHandler streamHandler; + private final ResponseHandlerErrorNotifier errorNotifier; private CrtResponseAdapter(CompletableFuture completionFuture, SdkAsyncHttpResponseHandler responseHandler, - CrtStreamHandler streamHandler) { - this(completionFuture, responseHandler, new SimplePublisher<>(), streamHandler); - } - - @SdkTestInternalApi - public CrtResponseAdapter(CompletableFuture completionFuture, - SdkAsyncHttpResponseHandler responseHandler, - SimplePublisher simplePublisher) { - this(completionFuture, responseHandler, simplePublisher, new CrtStreamHandler()); + CrtStreamHandler streamHandler, + ResponseHandlerErrorNotifier errorNotifier) { + this(completionFuture, responseHandler, new SimplePublisher<>(), streamHandler, errorNotifier); } @SdkTestInternalApi public CrtResponseAdapter(CompletableFuture completionFuture, SdkAsyncHttpResponseHandler responseHandler, SimplePublisher simplePublisher, - CrtStreamHandler streamHandler) { + CrtStreamHandler streamHandler, + ResponseHandlerErrorNotifier errorNotifier) { this.completionFuture = Validate.paramNotNull(completionFuture, "completionFuture"); this.responseHandler = Validate.paramNotNull(responseHandler, "responseHandler"); this.responseBuilder = SdkHttpResponse.builder(); this.responsePublisher = simplePublisher; this.streamHandler = streamHandler; + this.errorNotifier = Validate.paramNotNull(errorNotifier, "errorNotifier"); this.responseHandlerHelper = new ResponseHandlerHelper(responseBuilder); } public static HttpStreamBaseResponseHandler toCrtResponseHandler( CompletableFuture requestFuture, SdkAsyncHttpResponseHandler responseHandler, - CrtStreamHandler streamHandler) { - return new CrtResponseAdapter(requestFuture, responseHandler, streamHandler); + CrtStreamHandler streamHandler, + ResponseHandlerErrorNotifier errorNotifier) { + return new CrtResponseAdapter(requestFuture, responseHandler, streamHandler, errorNotifier); } @Override @@ -102,6 +101,7 @@ public int onResponseBody(HttpStreamBase stream, byte[] bodyBytesIn) { CompletableFuture writeFuture = responsePublisher.send(ByteBuffer.wrap(bodyBytesIn)); if (writeFuture.isDone() && !writeFuture.isCompletedExceptionally()) { + // Optimization: If write succeeded immediately, return non-zero to avoid the extra call back into the CRT. return bodyBytesIn.length; } @@ -146,15 +146,7 @@ private void onFailedResponseComplete(HttpException error) { } private void failResponseHandlerAndFuture(Throwable error) { - callResponseHandlerOnError(error); + errorNotifier.tryNotifyError(error); completionFuture.completeExceptionally(error); } - - private void callResponseHandlerOnError(Throwable error) { - try { - responseHandler.onError(error); - } catch (RuntimeException e) { - log.warn(() -> "Exception raised from SdkAsyncHttpResponseHandler#onError.", e); - } - } } diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/BaseHttpStreamResponseHandlerTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/BaseHttpStreamResponseHandlerTest.java index 959e4286d2c8..e8dcaf7ee6b4 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/BaseHttpStreamResponseHandlerTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/BaseHttpStreamResponseHandlerTest.java @@ -75,8 +75,7 @@ public void setUp() { return null; }).when(httpStream).close(); - streamHandler = new CrtStreamHandler(); - streamHandler.setStream(httpStream); + streamHandler = new CrtStreamHandler(CompletableFuture.completedFuture(httpStream)); responseHandler = responseHandler(streamHandler); } diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutorTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutorTest.java index 89d5e2fb2f65..98b572d126be 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutorTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutorTest.java @@ -23,11 +23,16 @@ import java.io.IOException; import java.net.ConnectException; import java.net.URI; -import javax.net.ssl.SSLHandshakeException; -import java.util.concurrent.CompletableFuture; -import java.util.stream.Stream; +import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; import java.util.AbstractMap.SimpleEntry; +import java.util.ArrayList; +import java.util.List; import java.util.Map.Entry; +import java.util.Optional; +import java.util.concurrent.CompletableFuture; +import java.util.stream.Stream; +import javax.net.ssl.SSLHandshakeException; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -38,6 +43,8 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; +import org.reactivestreams.Subscriber; +import org.reactivestreams.Subscription; import software.amazon.awssdk.crt.CrtRuntimeException; import software.amazon.awssdk.crt.http.HttpException; import software.amazon.awssdk.crt.http.HttpRequestBase; @@ -48,6 +55,7 @@ import software.amazon.awssdk.http.SdkHttpFullRequest; import software.amazon.awssdk.http.async.AsyncExecuteRequest; import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; +import software.amazon.awssdk.http.async.SdkHttpContentPublisher; import software.amazon.awssdk.utils.CompletableFutureUtils; @ExtendWith(MockitoExtension.class) @@ -105,7 +113,9 @@ public void execute_acquireStreamFails_invokesOnErrorAndWrapsWithIOException() { CrtAsyncRequestContext context = crtAsyncRequestContext(); CompletableFuture completableFuture = new CompletableFuture<>(); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), Mockito.any(HttpStreamBaseResponseHandler.class))) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) .thenReturn(completableFuture); completableFuture.completeExceptionally(exception); @@ -125,7 +135,9 @@ public void execute_crtRuntimeException_invokesOnError() { CrtAsyncRequestContext context = crtAsyncRequestContext(); CompletableFuture completableFuture = CompletableFutureUtils.failedFuture(exception); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), Mockito.any(HttpStreamBaseResponseHandler.class))) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) .thenReturn(completableFuture); CompletableFuture executeFuture = requestExecutor.execute(context); @@ -143,7 +155,9 @@ public void execute_requestCancelled_invokesOnError() { CrtAsyncRequestContext context = crtAsyncRequestContext(); CompletableFuture completableFuture = new CompletableFuture<>(); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), Mockito.any(HttpStreamBaseResponseHandler.class))) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) .thenReturn(completableFuture); CompletableFuture executeFuture = requestExecutor.execute(context); @@ -163,7 +177,9 @@ public void execute_retryableHttpException_wrapsWithIOException() { CrtAsyncRequestContext context = crtAsyncRequestContext(); CompletableFuture completableFuture = CompletableFutureUtils.failedFuture(exception); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), Mockito.any(HttpStreamBaseResponseHandler.class))) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) .thenReturn(completableFuture); CompletableFuture executeFuture = requestExecutor.execute(context); @@ -180,7 +196,9 @@ public void execute_httpException_mapsToCorrectException(Entry completableFuture = CompletableFutureUtils.failedFuture(exception); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), Mockito.any(HttpStreamBaseResponseHandler.class))) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) .thenReturn(completableFuture); CompletableFuture executeFuture = requestExecutor.execute(context); @@ -193,7 +211,9 @@ public void execute_nonRetryableHttpException_doesNotWrapWithIOException() { CrtAsyncRequestContext context = crtAsyncRequestContext(); CompletableFuture completableFuture = CompletableFutureUtils.failedFuture(exception); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), Mockito.any(HttpStreamBaseResponseHandler.class))) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) .thenReturn(completableFuture); CompletableFuture executeFuture = requestExecutor.execute(context); @@ -206,6 +226,329 @@ public void execute_nonRetryableHttpException_doesNotWrapWithIOException() { assertThatThrownBy(executeFuture::join).hasCause(exception); } + @Test + public void execute_streamActivateThrows_failsRequestFutureWithIoExceptionAndInvokesOnError() { + CrtRuntimeException activateError = new CrtRuntimeException("activate failed"); + CrtAsyncRequestContext context = crtAsyncRequestContext(); + + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) + .thenReturn(CompletableFuture.completedFuture(httpStream)); + Mockito.doThrow(activateError).when(httpStream).activate(); + + CompletableFuture executeFuture = requestExecutor.execute(context); + + assertThat(executeFuture).hasFailedWithThrowableThat() + .isInstanceOf(IOException.class) + .hasCauseInstanceOf(CrtRuntimeException.class); + ArgumentCaptor errorCaptor = ArgumentCaptor.forClass(Throwable.class); + Mockito.verify(responseHandler, Mockito.times(1)).onError(errorCaptor.capture()); + assertThat(errorCaptor.getValue()).isInstanceOf(IOException.class) + .hasCauseInstanceOf(CrtRuntimeException.class); + } + + @Test + public void execute_publisherSubscribeThrowsSynchronously_failsRequestFutureAndInvokesOnError() { + RuntimeException subscribeError = new RuntimeException("subscribe failure"); + SdkHttpContentPublisher throwingPublisher = new SdkHttpContentPublisher() { + @Override + public Optional contentLength() { + return Optional.of(0L); + } + + @Override + public void subscribe(Subscriber s) { + throw subscribeError; + } + }; + CrtAsyncRequestContext context = crtAsyncRequestContext(throwingPublisher); + + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) + .thenReturn(CompletableFuture.completedFuture(httpStream)); + + CompletableFuture executeFuture = requestExecutor.execute(context); + + assertThat(executeFuture).hasFailedWithThrowableThat().isSameAs(subscribeError); + Mockito.verify(responseHandler, Mockito.times(1)).onError(subscribeError); + } + + @Test + public void execute_publisherDeliversBody_writesAllChunksAndEndOfStream() { + byte[] chunk1 = "hello ".getBytes(StandardCharsets.UTF_8); + byte[] chunk2 = "world".getBytes(StandardCharsets.UTF_8); + TestPublisher publisher = TestPublisher.builder() + .contentLength(chunk1.length + chunk2.length) + .emit(ByteBuffer.wrap(chunk1)) + .emit(ByteBuffer.wrap(chunk2)) + .complete() + .build(); + CrtAsyncRequestContext context = crtAsyncRequestContext(publisher); + + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) + .thenReturn(CompletableFuture.completedFuture(httpStream)); + Mockito.when(httpStream.writeData(Mockito.any(byte[].class), Mockito.eq(false))) + .thenReturn(CompletableFuture.completedFuture(null)); + Mockito.when(httpStream.writeData(Mockito.isNull(), Mockito.eq(true))) + .thenReturn(CompletableFuture.completedFuture(null)); + + requestExecutor.execute(context); + + ArgumentCaptor dataCaptor = ArgumentCaptor.forClass(byte[].class); + Mockito.verify(httpStream, Mockito.times(2)).writeData(dataCaptor.capture(), Mockito.eq(false)); + assertThat(dataCaptor.getAllValues().get(0)).isEqualTo(chunk1); + assertThat(dataCaptor.getAllValues().get(1)).isEqualTo(chunk2); + Mockito.verify(httpStream, Mockito.times(1)).writeData(Mockito.isNull(), Mockito.eq(true)); + } + + @Test + public void execute_publisherSignalsError_failsExecuteFutureAndClosesConnection() { + RuntimeException publisherError = new RuntimeException("publisher failure"); + byte[] chunk = "data".getBytes(StandardCharsets.UTF_8); + TestPublisher publisher = TestPublisher.builder() + .contentLength(chunk.length) + .emit(ByteBuffer.wrap(chunk)) + .error(publisherError) + .build(); + CrtAsyncRequestContext context = crtAsyncRequestContext(publisher); + + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) + .thenReturn(CompletableFuture.completedFuture(httpStream)); + Mockito.when(httpStream.writeData(Mockito.any(byte[].class), Mockito.eq(false))) + .thenReturn(CompletableFuture.completedFuture(null)); + + CompletableFuture executeFuture = requestExecutor.execute(context); + + Mockito.verify(responseHandler).onError(publisherError); + Mockito.verify(httpStream).cancel(); + Mockito.verify(httpStream).close(); + assertThat(executeFuture).hasFailedWithThrowableThat().isSameAs(publisherError); + } + + @Test + public void execute_writeDataFutureFails_failsExecuteFutureAndClosesConnection() { + RuntimeException writeError = new RuntimeException("write failure"); + byte[] chunk = "data".getBytes(StandardCharsets.UTF_8); + TestPublisher publisher = TestPublisher.builder() + .contentLength(chunk.length) + .emit(ByteBuffer.wrap(chunk)) + .complete() + .build(); + CrtAsyncRequestContext context = crtAsyncRequestContext(publisher); + + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) + .thenReturn(CompletableFuture.completedFuture(httpStream)); + Mockito.when(httpStream.writeData(Mockito.any(byte[].class), Mockito.eq(false))) + .thenReturn(CompletableFutureUtils.failedFuture(writeError)); + // After the write failure cancels the subscription, the publisher must not emit further + // signals per Reactive Streams rule 1.6, but stub the EOS write defensively in case the + // subscriber's onComplete still fires before cancellation propagates. + Mockito.when(httpStream.writeData(Mockito.isNull(), Mockito.eq(true))) + .thenReturn(CompletableFuture.completedFuture(null)); + + CompletableFuture executeFuture = requestExecutor.execute(context); + + Mockito.verify(responseHandler).onError(writeError); + Mockito.verify(httpStream).cancel(); + Mockito.verify(httpStream).close(); + assertThat(executeFuture).hasFailedWithThrowableThat().isSameAs(writeError); + } + + @Test + public void execute_emptyBodyPublisher_subscribesAndSendsEndOfStreamMarker() { + // createProvider("") returns a publisher with contentLength = Optional.of(0L) that emits + // onComplete with no onNext. The subscriber should send writeData(null, true) as the EOS + // marker and never call writeData with body bytes. + CrtAsyncRequestContext context = crtAsyncRequestContext(); + + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) + .thenReturn(CompletableFuture.completedFuture(httpStream)); + Mockito.when(httpStream.writeData(Mockito.isNull(), Mockito.eq(true))) + .thenReturn(CompletableFuture.completedFuture(null)); + + requestExecutor.execute(context); + + ArgumentCaptor manualWritesCaptor = ArgumentCaptor.forClass(Boolean.class); + Mockito.verify(streamManager).acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + manualWritesCaptor.capture()); + assertThat(manualWritesCaptor.getValue()).isTrue(); + Mockito.verify(httpStream, Mockito.never()).writeData(Mockito.any(byte[].class), Mockito.eq(false)); + Mockito.verify(httpStream).writeData(Mockito.isNull(), Mockito.eq(true)); + } + + @Test + public void execute_chunkedBodyPublisher_useManualDataWritesIsTrue() { + byte[] chunk = "data".getBytes(StandardCharsets.UTF_8); + TestPublisher publisher = TestPublisher.builder() + .unknownContentLength() + .emit(ByteBuffer.wrap(chunk)) + .complete() + .build(); + CrtAsyncRequestContext context = crtAsyncRequestContext(publisher); + + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) + .thenReturn(CompletableFuture.completedFuture(httpStream)); + Mockito.when(httpStream.writeData(Mockito.any(byte[].class), Mockito.eq(false))) + .thenReturn(CompletableFuture.completedFuture(null)); + Mockito.when(httpStream.writeData(Mockito.isNull(), Mockito.eq(true))) + .thenReturn(CompletableFuture.completedFuture(null)); + + requestExecutor.execute(context); + + ArgumentCaptor manualWritesCaptor = ArgumentCaptor.forClass(Boolean.class); + Mockito.verify(streamManager).acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + manualWritesCaptor.capture()); + assertThat(manualWritesCaptor.getValue()).isTrue(); + Mockito.verify(httpStream).writeData(Mockito.eq(chunk), Mockito.eq(false)); + Mockito.verify(httpStream).writeData(Mockito.isNull(), Mockito.eq(true)); + } + + @Test + public void execute_nonEmptyBodyPublisher_useManualDataWritesIsTrue() { + byte[] chunk = "data".getBytes(StandardCharsets.UTF_8); + TestPublisher publisher = TestPublisher.builder() + .contentLength(chunk.length) + .emit(ByteBuffer.wrap(chunk)) + .complete() + .build(); + CrtAsyncRequestContext context = crtAsyncRequestContext(publisher); + + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) + .thenReturn(CompletableFuture.completedFuture(httpStream)); + Mockito.when(httpStream.writeData(Mockito.any(byte[].class), Mockito.eq(false))) + .thenReturn(CompletableFuture.completedFuture(null)); + Mockito.when(httpStream.writeData(Mockito.isNull(), Mockito.eq(true))) + .thenReturn(CompletableFuture.completedFuture(null)); + + requestExecutor.execute(context); + + ArgumentCaptor manualWritesCaptor = ArgumentCaptor.forClass(Boolean.class); + Mockito.verify(streamManager).acquireStream(Mockito.any(HttpRequestBase.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + manualWritesCaptor.capture()); + assertThat(manualWritesCaptor.getValue()).isTrue(); + } + + private CrtAsyncRequestContext crtAsyncRequestContext(SdkHttpContentPublisher publisher) { + SdkHttpFullRequest request = createRequest(URI.create("http://localhost")); + return CrtAsyncRequestContext.builder() + .readBufferSize(2000) + .streamManager(streamManager) + .request(AsyncExecuteRequest.builder() + .request(request) + .requestContentPublisher(publisher) + .responseHandler(responseHandler) + .build()) + .build(); + } + + /** + * A test {@link SdkHttpContentPublisher} that emits a fixed sequence of buffers, then either + * completes or signals an error. All emissions are delivered synchronously inside the first + * {@code request(n)} call. + */ + private static final class TestPublisher implements SdkHttpContentPublisher { + private final List buffers; + private final boolean complete; + private final Throwable error; + private final Optional contentLength; + + private TestPublisher(Builder b) { + this.buffers = b.buffers; + this.complete = b.complete; + this.error = b.error; + this.contentLength = b.contentLength; + } + + static Builder builder() { + return new Builder(); + } + + @Override + public Optional contentLength() { + return contentLength; + } + + @Override + public void subscribe(Subscriber s) { + s.onSubscribe(new Subscription() { + private boolean delivered; + + @Override + public void request(long n) { + if (delivered) { + return; + } + delivered = true; + for (ByteBuffer b : buffers) { + s.onNext(b); + } + if (error != null) { + s.onError(error); + } else if (complete) { + s.onComplete(); + } + } + + @Override + public void cancel() { + } + }); + } + + static final class Builder { + private final List buffers = new ArrayList<>(); + private boolean complete; + private Throwable error; + private Optional contentLength = Optional.of(0L); + + Builder emit(ByteBuffer buf) { + buffers.add(buf); + return this; + } + + Builder complete() { + this.complete = true; + return this; + } + + Builder error(Throwable t) { + this.error = t; + return this; + } + + Builder contentLength(long len) { + this.contentLength = Optional.of(len); + return this; + } + + Builder unknownContentLength() { + this.contentLength = Optional.empty(); + return this; + } + + TestPublisher build() { + return new TestPublisher(this); + } + } + } + private CrtAsyncRequestContext crtAsyncRequestContext() { SdkHttpFullRequest request = createRequest(URI.create("http://localhost")); return CrtAsyncRequestContext.builder() diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java index a27e778cd265..74f8c5c06069 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java @@ -24,6 +24,7 @@ import java.net.URI; import javax.net.ssl.SSLHandshakeException; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; import java.util.stream.Stream; import java.util.AbstractMap.SimpleEntry; import java.util.Map.Entry; @@ -87,7 +88,7 @@ public void execute_requestConversionFails_failsFuture() { .request(HttpExecuteRequest.builder().build()) .build(); - CompletableFuture executeFuture = requestExecutor.execute(context, new CrtStreamHandler()); + CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); assertThat(executeFuture).hasFailedWithThrowableThat().isInstanceOf(NullPointerException.class); } @@ -102,7 +103,7 @@ public void execute_acquireStreamFails_wrapsWithIOException() { .thenReturn(completableFuture); completableFuture.completeExceptionally(exception); - CompletableFuture executeFuture = requestExecutor.execute(context, new CrtStreamHandler()); + CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); assertThat(executeFuture).hasFailedWithThrowableThat().hasCause(exception).isInstanceOf(IOException.class); } @@ -116,7 +117,7 @@ public void execute_retryableException_wrapsWithIOException(Throwable throwable) Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequest.class), Mockito.any(HttpStreamBaseResponseHandler.class), Mockito.anyBoolean())) .thenReturn(completableFuture); - CompletableFuture executeFuture = requestExecutor.execute(context, new CrtStreamHandler()); + CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); assertThat(executeFuture).hasFailedWithThrowableThat().hasCause(throwable).isInstanceOf(IOException.class); } @@ -133,10 +134,47 @@ public void execute_httpException_mapsToCorrectException(Entry executeFuture = requestExecutor.execute(context, new CrtStreamHandler()); + CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); assertThatThrownBy(executeFuture::join).hasCauseInstanceOf(expectedExceptionClass); } + @Test + public void execute_doExecuteThrowsSynchronously_failsBothStreamFutureAndResponseFuture() { + CrtRequestContext context = CrtRequestContext.builder() + .streamManager(streamManager) + .request(HttpExecuteRequest.builder().build()) + .build(); + + CrtRequestExecutor.Result result = requestExecutor.execute(context); + + assertThat(result.responseFuture()).hasFailedWithThrowableThat().isInstanceOf(NullPointerException.class); + assertThatThrownBy(() -> result.streamHandler().waitForStream()) + .isInstanceOf(CompletionException.class) + .hasCauseInstanceOf(NullPointerException.class); + } + + @Test + public void execute_streamActivateThrows_failsBothFuturesWithIoExceptionWrappingCause() { + CrtRuntimeException activateError = new CrtRuntimeException("activate failed"); + CrtRequestContext context = crtRequestContext(); + + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequest.class), + Mockito.any(HttpStreamBaseResponseHandler.class), + Mockito.anyBoolean())) + .thenReturn(CompletableFuture.completedFuture(httpStream)); + Mockito.doThrow(activateError).when(httpStream).activate(); + + CrtRequestExecutor.Result result = requestExecutor.execute(context); + + assertThat(result.responseFuture()).hasFailedWithThrowableThat() + .isInstanceOf(IOException.class) + .hasCauseInstanceOf(CrtRuntimeException.class); + assertThatThrownBy(() -> result.streamHandler().waitForStream()) + .isInstanceOf(CompletionException.class) + .hasCauseInstanceOf(IOException.class) + .hasRootCauseInstanceOf(CrtRuntimeException.class); + } + @Test public void execute_nonRetryableHttpException_doesNotWrapWithIOException() { HttpException exception = new HttpException(0x0801); // AWS_ERROR_HTTP_HEADER_NOT_FOUND @@ -146,7 +184,7 @@ public void execute_nonRetryableHttpException_doesNotWrapWithIOException() { Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequest.class), Mockito.any(HttpStreamBaseResponseHandler.class), Mockito.anyBoolean())) .thenReturn(completableFuture); - CompletableFuture executeFuture = requestExecutor.execute(context, new CrtStreamHandler()); + CompletableFuture executeFuture = requestExecutor.execute(context).responseFuture(); assertThatThrownBy(executeFuture::join).hasCause(exception); } diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtResponseHandlerTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtResponseHandlerTest.java index aba67504c5a0..5068af83d699 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtResponseHandlerTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtResponseHandlerTest.java @@ -34,6 +34,7 @@ import software.amazon.awssdk.http.SdkHttpResponse; import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; import software.amazon.awssdk.http.crt.internal.CrtStreamHandler; +import software.amazon.awssdk.http.crt.internal.ResponseHandlerErrorNotifier; import software.amazon.awssdk.http.crt.internal.response.CrtResponseAdapter; import software.amazon.awssdk.utils.async.SimplePublisher; @@ -45,7 +46,8 @@ HttpStreamBaseResponseHandler responseHandler(CrtStreamHandler streamHandler) { executionAttributes) -> null, Function.identity(), new ExecutionAttributes()); responseHandler.prepare(); - return CrtResponseAdapter.toCrtResponseHandler(requestFuture, responseHandler, streamHandler); + return CrtResponseAdapter.toCrtResponseHandler(requestFuture, responseHandler, streamHandler, + new ResponseHandlerErrorNotifier(responseHandler)); } @Override @@ -55,14 +57,16 @@ HttpStreamBaseResponseHandler responseHandlerWithMockedPublisher(SimplePublisher executionAttributes) -> null, Function.identity(), new ExecutionAttributes()); responseHandler.prepare(); - return new CrtResponseAdapter(requestFuture, responseHandler, simplePublisher, streamHandler); + return new CrtResponseAdapter(requestFuture, responseHandler, simplePublisher, streamHandler, + new ResponseHandlerErrorNotifier(responseHandler)); } @Test void onResponseComplete_publisherCancelled_closesStream() { SdkAsyncHttpResponseHandler responseHandler = new TestAsyncHttpResponseHandler(); - HttpStreamBaseResponseHandler crtResponseHandler = CrtResponseAdapter.toCrtResponseHandler(requestFuture, responseHandler, streamHandler); + HttpStreamBaseResponseHandler crtResponseHandler = CrtResponseAdapter.toCrtResponseHandler(requestFuture, responseHandler, streamHandler, + new ResponseHandlerErrorNotifier(responseHandler)); HttpHeader[] httpHeaders = getHttpHeaders(); crtResponseHandler.onResponseHeaders(httpStream, 200, HttpHeaderBlock.MAIN.getValue(), httpHeaders); diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandlerTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandlerTest.java index 6ac5d39f7d2e..8a0e21c5a291 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandlerTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandlerTest.java @@ -15,9 +15,15 @@ package software.amazon.awssdk.http.crt.internal; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; +import java.io.IOException; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; +import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicBoolean; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -26,7 +32,9 @@ import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.stubbing.Answer; +import software.amazon.awssdk.crt.CrtRuntimeException; import software.amazon.awssdk.crt.http.HttpStreamBase; +import software.amazon.awssdk.utils.CompletableFutureUtils; @ExtendWith(MockitoExtension.class) class CrtStreamHandlerTest { @@ -45,8 +53,7 @@ void setUp() { return null; }).when(stream).close(); - streamHandler = new CrtStreamHandler(); - streamHandler.setStream(stream); + streamHandler = new CrtStreamHandler(CompletableFuture.completedFuture(stream)); } @Test @@ -87,4 +94,76 @@ void incrementWindow_beforeClose_shouldWork() { verify(stream).incrementWindow(1024); } + + @Test + void incrementWindow_beforeStreamReady_shouldBeNoOp() { + CrtStreamHandler handler = new CrtStreamHandler(new CompletableFuture<>()); + + handler.incrementWindow(1024); + + verify(stream, never()).incrementWindow(1024); + } + + @Test + void waitForStream_afterStreamFutureFailed_throwsCompletionExceptionWrappingCause() { + IOException cause = new IOException("acquire failed"); + CrtStreamHandler handler = new CrtStreamHandler(CompletableFutureUtils.failedFuture(cause)); + + assertThatThrownBy(handler::waitForStream) + .isInstanceOf(CompletionException.class) + .hasCause(cause); + } + + @Test + void writeData_underlyingWriteFails_propagatesOriginalCauseUnwrapped() { + RuntimeException writeError = new RuntimeException("write failure"); + Mockito.when(stream.writeData(Mockito.any(byte[].class), Mockito.eq(false))) + .thenReturn(CompletableFutureUtils.failedFuture(writeError)); + + CompletableFuture writeFuture = streamHandler.writeData(new byte[] {1, 2, 3}, false); + + assertThatThrownBy(writeFuture::get) + .isInstanceOf(ExecutionException.class) + .hasCauseReference(writeError); + } + + @Test + void writeData_streamAcquisitionFailed_propagatesOriginalCauseUnwrapped() { + IOException acquireFailure = new IOException("acquire failed"); + CrtStreamHandler handler = new CrtStreamHandler(CompletableFutureUtils.failedFuture(acquireFailure)); + + CompletableFuture writeFuture = handler.writeData(new byte[] {1, 2, 3}, false); + + assertThatThrownBy(writeFuture::get) + .isInstanceOf(ExecutionException.class) + .hasCauseReference(acquireFailure); + } + + @Test + void writeData_afterCloseConnection_failsWithIoException() { + streamHandler.closeConnection(); + + CompletableFuture writeFuture = streamHandler.writeData(new byte[] {1, 2, 3}, false); + + assertThat(writeFuture).isCompletedExceptionally(); + assertThatThrownBy(writeFuture::get) + .isInstanceOf(ExecutionException.class) + .hasCauseInstanceOf(IOException.class); + } + + @Test + void writeData_underlyingWriteThrowsSynchronously_failsFutureAndClosesConnection() { + CrtRuntimeException syncError = new CrtRuntimeException("AWS_ERROR_HTTP_STREAM_NOT_ACTIVATED"); + Mockito.when(stream.writeData(Mockito.any(byte[].class), Mockito.eq(false))) + .thenThrow(syncError); + + CompletableFuture writeFuture = streamHandler.writeData(new byte[] {1, 2, 3}, false); + + assertThat(writeFuture).isCompletedExceptionally(); + assertThatThrownBy(writeFuture::get) + .isInstanceOf(ExecutionException.class) + .hasCauseReference(syncError); + verify(stream).cancel(); + verify(stream).close(); + } } diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriberTckTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriberTckTest.java new file mode 100644 index 000000000000..ffd98e90802b --- /dev/null +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriberTckTest.java @@ -0,0 +1,68 @@ +/* + * 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.internal.request; + +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.nio.ByteBuffer; +import java.util.concurrent.CompletableFuture; +import org.reactivestreams.Subscriber; +import org.reactivestreams.tck.SubscriberBlackboxVerification; +import org.reactivestreams.tck.TestEnvironment; +import software.amazon.awssdk.crt.http.HttpStreamBase; +import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; +import software.amazon.awssdk.http.crt.internal.CrtStreamHandler; +import software.amazon.awssdk.http.crt.internal.ResponseHandlerErrorNotifier; + +/** + * TCK black-box verification for {@link CrtRequestBodyPublisherSubscriber}. + * + *

Black-box is appropriate because the subscriber's behavior depends on its constructor-injected + * collaborators ({@link CrtStreamHandler}, {@link CompletableFuture}, {@link SdkAsyncHttpResponseHandler}). + * The test wires a real {@code CrtStreamHandler} backed by a mocked {@link HttpStreamBase} so no JNI + * code is exercised. + */ +public class CrtRequestBodyPublisherSubscriberTckTest extends SubscriberBlackboxVerification { + + public CrtRequestBodyPublisherSubscriberTckTest() { + super(new TestEnvironment()); + } + + @Override + public Subscriber createSubscriber() { + HttpStreamBase mockStream = mock(HttpStreamBase.class); + when(mockStream.writeData(any(byte[].class), anyBoolean())) + .thenReturn(CompletableFuture.completedFuture(null)); + // writeData(null, true) is the end-of-stream signal sent on onComplete; stub it too. + when(mockStream.writeData(null, true)) + .thenReturn(CompletableFuture.completedFuture(null)); + + CrtStreamHandler streamHandler = new CrtStreamHandler(CompletableFuture.completedFuture(mockStream)); + + return new CrtRequestBodyPublisherSubscriber( + streamHandler, + new CompletableFuture<>(), + new ResponseHandlerErrorNotifier(mock(SdkAsyncHttpResponseHandler.class))); + } + + @Override + public ByteBuffer createElement(int element) { + return ByteBuffer.wrap(new byte[]{(byte) element}); + } +} diff --git a/pom.xml b/pom.xml index 9e544a097880..35605a9ba553 100644 --- a/pom.xml +++ b/pom.xml @@ -131,7 +131,7 @@ 3.1.5 1.17.1 1.37 - 0.46.0 + 0.46.1 5.10.3 From 994989e920f8fb42ba86381550e1b681c283f3c4 Mon Sep 17 00:00:00 2001 From: Zoe Wang <33073555+zoewangg@users.noreply.github.com> Date: Thu, 28 May 2026 17:08:08 -0700 Subject: [PATCH 6/7] refactor code and address feedback --- .../bugfix-AWSCRTHTTPClient-aee08c2.json | 2 +- .../awssdk/http/crt/AwsCrtHttpClient.java | 94 ++++++++++--------- .../crt/internal/CrtAsyncRequestExecutor.java | 13 ++- .../http/crt/internal/CrtRequestExecutor.java | 8 +- .../http/crt/internal/CrtStreamHandler.java | 18 +++- .../crt/AwsCrtHttpClientWireMockTest.java | 30 ++++++ .../BaseHttpStreamResponseHandlerTest.java | 20 +--- .../internal/CrtAsyncRequestExecutorTest.java | 6 ++ .../crt/internal/CrtRequestExecutorTest.java | 3 + .../crt/internal/CrtStreamHandlerTest.java | 9 -- 10 files changed, 128 insertions(+), 75 deletions(-) diff --git a/.changes/next-release/bugfix-AWSCRTHTTPClient-aee08c2.json b/.changes/next-release/bugfix-AWSCRTHTTPClient-aee08c2.json index f0cc6298ec3c..1eeb2a43c480 100644 --- a/.changes/next-release/bugfix-AWSCRTHTTPClient-aee08c2.json +++ b/.changes/next-release/bugfix-AWSCRTHTTPClient-aee08c2.json @@ -2,5 +2,5 @@ "type": "bugfix", "category": "AWS CRT HTTP Client", "contributor": "", - "description": "Fixed a potential deadlock in `AwsCrtHttpClient` that could occur when the request body `InputStream` blocked waiting for data on the CRT event loop thread. This could happen when a blocking stream (e.g., a `BufferedInputStream` wrapping a `ResponseInputStream`) was used as a request body and the read depended on the same event loop thread to deliver data. Request body writing now happens on the caller thread." + "description": "Fixed a potential deadlock in both `AwsCrtHttpClient` and `AwsCrtAsyncHttpClient` that could occur when the request body source delivered data on the CRT event loop thread. For sync, this happened when a blocking `InputStream` (e.g., a `BufferedInputStream` wrapping a `ResponseInputStream`) was used as a request body. For async, this happened when the user-supplied `Publisher` scheduled `onNext` back onto the same event loop. Request body data is now pushed via the CRT push-based `writeData` API: sync writes from the caller thread, async writes from a Reactive Streams subscriber outside the event loop." } 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 87bb65cf05bd..adbe3dcf4462 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 @@ -69,7 +69,7 @@ private AwsCrtHttpClient(DefaultBuilder builder, AttributeMap config) { } } - public static AwsCrtHttpClient.Builder builder() { + public static Builder builder() { return new DefaultBuilder(); } @@ -124,45 +124,53 @@ private CrtHttpRequest(CrtRequestContext context) { @Override public HttpExecuteResponse call() throws IOException { HttpExecuteResponse.Builder builder = HttpExecuteResponse.builder(); + CrtRequestExecutor.Result result = new CrtRequestExecutor().execute(context); + responseFuture = result.responseFuture(); try { - CrtRequestExecutor.Result result = new CrtRequestExecutor().execute(context); - responseFuture = result.responseFuture(); writeRequestBody(result.streamHandler()); SdkHttpFullResponse response = CompletableFutureUtils.joinInterruptibly(responseFuture); builder.response(response); builder.responseBody(response.content().orElse(null)); return builder.build(); - } catch (CompletionException e) { - Throwable cause = e.getCause(); - - // Complete the future exceptionally to trigger connection cleanup in the response handler. - // Handles thread-interrupt case where joinInterruptibly throws due to - // InterruptedException. Without this, the - // Ensures that closeConnection() is invoked to prevent leaking the connection from the pool. - if (responseFuture != null) { - responseFuture.completeExceptionally(cause != null ? cause : e); - } - - if (cause instanceof IOException) { - throw (IOException) cause; - } - - if (cause instanceof HttpException) { - Throwable wrapped = CrtUtils.wrapCrtException(cause); - if (wrapped instanceof IOException) { - throw (IOException) wrapped; - } - throw (HttpException) cause; - } + } catch (Throwable t) { + // CompletionException is the wrapper from joinInterruptibly; direct throws + // (e.g., IOException from inputStream.read in writeRequestBody) arrive unwrapped. + Throwable cause = (t instanceof CompletionException && t.getCause() != null) ? t.getCause() : t; + + // Tear down the stream so the connection is not leaked back to the pool. + // closeConnection() is idempotent and a no-op if the stream is not yet acquired + // or is already closed. + result.streamHandler().closeConnection(); + responseFuture.completeExceptionally(cause); + + throw mapToIoExceptionOrRethrow(cause); + } + } - if (cause instanceof InterruptedException) { - Thread.currentThread().interrupt(); - throw new IOException("Request was cancelled", cause); + private static IOException mapToIoExceptionOrRethrow(Throwable cause) { + if (cause instanceof IOException) { + return (IOException) cause; + } + if (cause instanceof HttpException) { + Throwable wrapped = CrtUtils.wrapCrtException(cause); + if (wrapped instanceof IOException) { + return (IOException) wrapped; } - throw new RuntimeException(e.getCause()); + throw (HttpException) cause; + } + if (cause instanceof InterruptedException) { + Thread.currentThread().interrupt(); + return new IOException("Request was cancelled", cause); + } + if (cause instanceof RuntimeException) { + throw (RuntimeException) cause; + } + if (cause instanceof Error) { + throw (Error) cause; } + return new IOException(cause); } private void writeRequestBody(CrtStreamHandler streamHandler) throws IOException { @@ -194,14 +202,14 @@ public void abort() { /** * Builder that allows configuration of the AWS CRT HTTP implementation. */ - public interface Builder extends SdkHttpClient.Builder { + public interface Builder extends SdkHttpClient.Builder { /** * The Maximum number of allowed concurrent requests. For HTTP/1.1 this is the same as max connections. * @param maxConcurrency maximum concurrency per endpoint * @return The builder of the method chaining. */ - AwsCrtHttpClient.Builder maxConcurrency(Integer maxConcurrency); + Builder maxConcurrency(Integer maxConcurrency); /** * Configures the number of unread bytes that can be buffered in the @@ -211,14 +219,14 @@ public interface Builder extends SdkHttpClient.Builder * @param readBufferSize The number of bytes that can be buffered. * @return The builder of the method chaining. */ - AwsCrtHttpClient.Builder readBufferSizeInBytes(Long readBufferSize); + Builder readBufferSizeInBytes(Long readBufferSize); /** * Sets the http proxy configuration to use for this client. * @param proxyConfiguration The http proxy configuration to use * @return The builder of the method chaining. */ - AwsCrtHttpClient.Builder proxyConfiguration(ProxyConfiguration proxyConfiguration); + Builder proxyConfiguration(ProxyConfiguration proxyConfiguration); /** * Sets the http proxy configuration to use for this client. @@ -226,7 +234,7 @@ public interface Builder extends SdkHttpClient.Builder * @param proxyConfigurationBuilderConsumer The consumer of the proxy configuration builder object. * @return the builder for method chaining. */ - AwsCrtHttpClient.Builder proxyConfiguration(Consumer proxyConfigurationBuilderConsumer); + Builder proxyConfiguration(Consumer proxyConfigurationBuilderConsumer); /** * Configure the health checks for all connections established by this client. @@ -246,7 +254,7 @@ public interface Builder extends SdkHttpClient.Builder * @param healthChecksConfiguration The health checks config to use * @return The builder of the method chaining. */ - AwsCrtHttpClient.Builder connectionHealthConfiguration(ConnectionHealthConfiguration healthChecksConfiguration); + Builder connectionHealthConfiguration(ConnectionHealthConfiguration healthChecksConfiguration); /** * A convenience method that creates an instance of the {@link ConnectionHealthConfiguration} builder, avoiding the @@ -256,7 +264,7 @@ public interface Builder extends SdkHttpClient.Builder * @return The builder of the method chaining. * @see #connectionHealthConfiguration(ConnectionHealthConfiguration) */ - AwsCrtHttpClient.Builder connectionHealthConfiguration(Consumer + Builder connectionHealthConfiguration(Consumer healthChecksConfigurationBuilder); /** @@ -264,21 +272,21 @@ AwsCrtHttpClient.Builder connectionHealthConfiguration(Consumer + Builder tcpKeepAliveConfiguration(Consumer tcpKeepAliveConfigurationBuilder); /** @@ -325,7 +333,7 @@ AwsCrtHttpClient.Builder tcpKeepAliveConfiguration(Consumer implements AwsCrtHttpClient.Builder { + extends AwsCrtClientBuilderBase implements Builder { @Override diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java index 91cbbaff243b..35f793740988 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java @@ -89,13 +89,22 @@ private void doExecute(CrtAsyncRequestContext executionContext, } try { stream.activate(); - streamFuture.complete(stream); - asyncRequest.requestContentPublisher().subscribe(bodySubscriber); } catch (Throwable t) { + // Stream is acquired but not activated and not yet published via + // streamFuture. No other path can reach it, so clean it up directly. + stream.cancel(); + stream.close(); handleAcquireFailure(t, streamFuture, requestFuture, errorNotifier); + return null; } + streamFuture.complete(stream); + asyncRequest.requestContentPublisher().subscribe(bodySubscriber); return null; }).exceptionally(t -> { + // Reached when the handle lambda throws (e.g., publisher.subscribe). + // closeConnection is a no-op if the stream isn't in streamFuture yet; + // otherwise it tears down the published stream. + streamHandler.closeConnection(); handleAcquireFailure(t, streamFuture, requestFuture, errorNotifier); return null; }); diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java index aff574635ff8..0f34c31967f3 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutor.java @@ -81,12 +81,18 @@ private void doExecute(CrtRequestContext executionContext, } try { streamBase.activate(); - streamFuture.complete(streamBase); } catch (Throwable t) { + // Stream is acquired but not activated and not yet published via + // streamFuture. No other path can reach it, so clean it up directly. + streamBase.cancel(); + streamBase.close(); handleAcquireFailure(t, streamFuture, responseFuture); + return null; } + streamFuture.complete(streamBase); return null; }).exceptionally(t -> { + // Defensive: only reached if the handle lambda itself throws. handleAcquireFailure(t, streamFuture, responseFuture); return null; }); diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandler.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandler.java index ab4fd2d6040c..f1fc932303fd 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandler.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandler.java @@ -106,7 +106,7 @@ private static Throwable unwrap(Throwable t) { public void incrementWindow(int windowSize) { synchronized (streamLock) { - HttpStreamBase s = streamFuture.getNow(null); + HttpStreamBase s = streamIfAvailable(); if (!streamClosed && s != null) { s.incrementWindow(windowSize); } @@ -119,7 +119,7 @@ public void incrementWindow(int windowSize) { */ public void releaseConnection() { synchronized (streamLock) { - HttpStreamBase s = streamFuture.getNow(null); + HttpStreamBase s = streamIfAvailable(); if (!streamClosed && s != null) { streamClosed = true; s.close(); @@ -134,7 +134,7 @@ public void releaseConnection() { */ public void closeConnection() { synchronized (streamLock) { - HttpStreamBase s = streamFuture.getNow(null); + HttpStreamBase s = streamIfAvailable(); if (!streamClosed && s != null) { streamClosed = true; s.cancel(); @@ -142,4 +142,16 @@ public void closeConnection() { } } } + + /** + * Returns the acquired stream if {@link #streamFuture} completed normally, otherwise {@code null}. + * Tolerates exceptional or pending completion (in contrast to {@link CompletableFuture#getNow}, which + * throws {@link CompletionException} when the future is exceptional). + */ + private HttpStreamBase streamIfAvailable() { + if (!streamFuture.isDone() || streamFuture.isCompletedExceptionally()) { + return null; + } + return streamFuture.getNow(null); + } } 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 ce5d778f06a1..7d45b7ef2aed 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 @@ -113,6 +113,36 @@ public void sharedEventLoopGroup_closeOneClient_shouldNotAffectOtherClients() th } } + @Test + public void contentStreamReadThrows_propagatesIoExceptionAndDoesNotLeakConnection() throws Exception { + try (SdkHttpClient client = AwsCrtHttpClient.builder().maxConcurrency(1).build()) { + URI uri = URI.create("http://localhost:" + mockServer.port()); + stubFor(any(urlPathEqualTo("/")).willReturn(aResponse().withBody(randomAlphabetic(10)))); + SdkHttpRequest request = createRequest(uri); + + IOException readError = new IOException("simulated read failure"); + ExecutableHttpRequest failing = client.prepareRequest( + HttpExecuteRequest.builder() + .request(request) + .contentStreamProvider(() -> new java.io.InputStream() { + @Override + public int read() throws IOException { + throw readError; + } + }) + .build()); + + assertThatThrownBy(failing::call) + .isInstanceOf(IOException.class) + .isSameAs(readError); + + // If the connection leaked, this second request would hang since maxConcurrency=1. + // Use a short overall test timeout to fail fast if the leak regresses. + HttpExecuteResponse second = makeSimpleRequest(client, null); + assertThat(second.httpResponse().statusCode()).isEqualTo(200); + } + } + @Test public void abortRequest_shouldFailTheExceptionWithIOException() throws Exception { try (SdkHttpClient client = AwsCrtHttpClient.create()) { diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/BaseHttpStreamResponseHandlerTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/BaseHttpStreamResponseHandlerTest.java index e8dcaf7ee6b4..110ad2c97cc5 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/BaseHttpStreamResponseHandlerTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/BaseHttpStreamResponseHandlerTest.java @@ -25,7 +25,6 @@ import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.atomic.AtomicBoolean; import org.apache.commons.lang3.RandomStringUtils; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -35,14 +34,12 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; -import org.mockito.stubbing.Answer; import software.amazon.awssdk.crt.http.HttpException; import software.amazon.awssdk.crt.http.HttpHeader; import software.amazon.awssdk.crt.http.HttpHeaderBlock; import software.amazon.awssdk.crt.http.HttpStream; import software.amazon.awssdk.crt.http.HttpStreamBaseResponseHandler; import software.amazon.awssdk.utils.async.SimplePublisher; -import software.amazon.awssdk.utils.async.SimplePublisher; @ExtendWith(MockitoExtension.class) public abstract class BaseHttpStreamResponseHandlerTest { @@ -66,15 +63,6 @@ abstract HttpStreamBaseResponseHandler responseHandlerWithMockedPublisher(Simple @BeforeEach public void setUp() { requestFuture = new CompletableFuture<>(); - - // Simulate CRT refcount behavior: isNull() returns true after close() - AtomicBoolean closed = new AtomicBoolean(false); - Mockito.lenient().when(httpStream.isNull()).thenAnswer(invocation -> closed.get()); - Mockito.lenient().doAnswer((Answer) invocation -> { - closed.set(true); - return null; - }).when(httpStream).close(); - streamHandler = new CrtStreamHandler(CompletableFuture.completedFuture(httpStream)); responseHandler = responseHandler(streamHandler); } @@ -114,7 +102,7 @@ void failedToGetResponse_shouldCancelAndCloseStream() { responseHandler.onResponseComplete(httpStream, 1); assertThatThrownBy(() -> requestFuture.join()).hasRootCauseInstanceOf(HttpException.class); verify(httpStream).cancel(); - verify(httpStream, Mockito.atLeastOnce()).close(); + verify(httpStream).close(); } @Test @@ -155,7 +143,7 @@ void publisherWritesFutureFails_shouldCancelAndCloseStream() { } verify(httpStream).cancel(); - verify(httpStream, Mockito.atLeastOnce()).close(); + verify(httpStream).close(); verify(httpStream, never()).incrementWindow(anyInt()); } @@ -179,7 +167,7 @@ void publisherWritesFutureCompletesAfterStreamClosed_shouldNotInvokeIncrementWin future.complete(null); requestFuture.join(); - verify(httpStream, Mockito.atLeastOnce()).close(); + verify(httpStream).close(); verify(httpStream, never()).incrementWindow(anyInt()); } @@ -204,7 +192,7 @@ void publisherWritesFutureCompletesBeforeStreamClosed_shouldInvokeIncrementWindo handler.onResponseComplete(httpStream, 0); requestFuture.join(); verify(httpStream).incrementWindow(anyInt()); - verify(httpStream, Mockito.atLeastOnce()).close(); + verify(httpStream).close(); } static HttpHeader[] getHttpHeaders() { diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutorTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutorTest.java index 98b572d126be..e5355c047072 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutorTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutorTest.java @@ -246,6 +246,9 @@ public void execute_streamActivateThrows_failsRequestFutureWithIoExceptionAndInv Mockito.verify(responseHandler, Mockito.times(1)).onError(errorCaptor.capture()); assertThat(errorCaptor.getValue()).isInstanceOf(IOException.class) .hasCauseInstanceOf(CrtRuntimeException.class); + // Verify the acquired stream was cancelled and closed so the connection is not leaked. + Mockito.verify(httpStream).cancel(); + Mockito.verify(httpStream).close(); } @Test @@ -273,6 +276,9 @@ public void subscribe(Subscriber s) { assertThat(executeFuture).hasFailedWithThrowableThat().isSameAs(subscribeError); Mockito.verify(responseHandler, Mockito.times(1)).onError(subscribeError); + // Verify the acquired stream was cancelled and closed so the connection is not leaked. + Mockito.verify(httpStream).cancel(); + Mockito.verify(httpStream).close(); } @Test diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java index 74f8c5c06069..3ca81a08b245 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtRequestExecutorTest.java @@ -173,6 +173,9 @@ public void execute_streamActivateThrows_failsBothFuturesWithIoExceptionWrapping .isInstanceOf(CompletionException.class) .hasCauseInstanceOf(IOException.class) .hasRootCauseInstanceOf(CrtRuntimeException.class); + // Verify the acquired stream was cancelled and closed so the connection is not leaked. + Mockito.verify(httpStream).cancel(); + Mockito.verify(httpStream).close(); } @Test diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandlerTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandlerTest.java index 8a0e21c5a291..bc6b38296e01 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandlerTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtStreamHandlerTest.java @@ -24,14 +24,12 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; -import java.util.concurrent.atomic.AtomicBoolean; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; -import org.mockito.stubbing.Answer; import software.amazon.awssdk.crt.CrtRuntimeException; import software.amazon.awssdk.crt.http.HttpStreamBase; import software.amazon.awssdk.utils.CompletableFutureUtils; @@ -46,13 +44,6 @@ class CrtStreamHandlerTest { @BeforeEach void setUp() { - AtomicBoolean closed = new AtomicBoolean(false); - Mockito.lenient().when(stream.isNull()).thenAnswer(invocation -> closed.get()); - Mockito.lenient().doAnswer((Answer) invocation -> { - closed.set(true); - return null; - }).when(stream).close(); - streamHandler = new CrtStreamHandler(CompletableFuture.completedFuture(stream)); } From 899217f72dec85a64313b2a8182c495cc3a851b4 Mon Sep 17 00:00:00 2001 From: Zoe Wang <33073555+zoewangg@users.noreply.github.com> Date: Thu, 28 May 2026 19:19:50 -0700 Subject: [PATCH 7/7] Remove async change to reduce blast radius --- .../bugfix-AWSCRTHTTPClient-aee08c2.json | 2 +- http-clients/aws-crt-client/pom.xml | 26 -- .../crt/internal/CrtAsyncRequestExecutor.java | 70 ++-- .../ResponseHandlerErrorNotifier.java | 57 --- .../internal/request/CrtRequestAdapter.java | 4 +- .../request/CrtRequestBodyAdapter.java | 50 +++ .../CrtRequestBodyPublisherSubscriber.java | 120 ------ .../internal/response/CrtResponseAdapter.java | 26 +- .../internal/CrtAsyncRequestExecutorTest.java | 367 +----------------- .../crt/internal/CrtResponseHandlerTest.java | 10 +- ...RequestBodyPublisherSubscriberTckTest.java | 68 ---- 11 files changed, 105 insertions(+), 695 deletions(-) delete mode 100644 http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/ResponseHandlerErrorNotifier.java create mode 100644 http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyAdapter.java delete mode 100644 http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriber.java delete mode 100644 http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriberTckTest.java diff --git a/.changes/next-release/bugfix-AWSCRTHTTPClient-aee08c2.json b/.changes/next-release/bugfix-AWSCRTHTTPClient-aee08c2.json index 1eeb2a43c480..f0cc6298ec3c 100644 --- a/.changes/next-release/bugfix-AWSCRTHTTPClient-aee08c2.json +++ b/.changes/next-release/bugfix-AWSCRTHTTPClient-aee08c2.json @@ -2,5 +2,5 @@ "type": "bugfix", "category": "AWS CRT HTTP Client", "contributor": "", - "description": "Fixed a potential deadlock in both `AwsCrtHttpClient` and `AwsCrtAsyncHttpClient` that could occur when the request body source delivered data on the CRT event loop thread. For sync, this happened when a blocking `InputStream` (e.g., a `BufferedInputStream` wrapping a `ResponseInputStream`) was used as a request body. For async, this happened when the user-supplied `Publisher` scheduled `onNext` back onto the same event loop. Request body data is now pushed via the CRT push-based `writeData` API: sync writes from the caller thread, async writes from a Reactive Streams subscriber outside the event loop." + "description": "Fixed a potential deadlock in `AwsCrtHttpClient` that could occur when the request body `InputStream` blocked waiting for data on the CRT event loop thread. This could happen when a blocking stream (e.g., a `BufferedInputStream` wrapping a `ResponseInputStream`) was used as a request body and the read depended on the same event loop thread to deliver data. Request body writing now happens on the caller thread." } diff --git a/http-clients/aws-crt-client/pom.xml b/http-clients/aws-crt-client/pom.xml index 47516d1140c8..8f22dcb96e31 100644 --- a/http-clients/aws-crt-client/pom.xml +++ b/http-clients/aws-crt-client/pom.xml @@ -213,32 +213,6 @@ - - - org.apache.maven.plugins - maven-surefire-plugin - ${maven.surefire.version} - - - - junit - false - - - - - - org.apache.maven.surefire - surefire-junit-platform - ${maven.surefire.version} - - - org.apache.maven.surefire - surefire-testng - ${maven.surefire.version} - - - diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java index 35f793740988..b4901c6d9cb8 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutor.java @@ -26,23 +26,25 @@ import software.amazon.awssdk.crt.http.HttpStreamBaseResponseHandler; import software.amazon.awssdk.http.SdkCancellationException; import software.amazon.awssdk.http.async.AsyncExecuteRequest; -import software.amazon.awssdk.http.crt.internal.request.CrtRequestBodyPublisherSubscriber; +import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; import software.amazon.awssdk.http.crt.internal.response.CrtResponseAdapter; import software.amazon.awssdk.metrics.MetricCollector; import software.amazon.awssdk.metrics.NoOpMetricCollector; +import software.amazon.awssdk.utils.Logger; @SdkInternalApi public final class CrtAsyncRequestExecutor { + private static final Logger log = Logger.loggerFor(CrtAsyncRequestExecutor.class); + public CompletableFuture execute(CrtAsyncRequestContext executionContext) { AsyncExecuteRequest asyncRequest = executionContext.sdkRequest(); CompletableFuture requestFuture = createAsyncExecutionFuture(asyncRequest); - ResponseHandlerErrorNotifier errorNotifier = new ResponseHandlerErrorNotifier(asyncRequest.responseHandler()); try { - doExecute(executionContext, asyncRequest, requestFuture, errorNotifier); + doExecute(executionContext, asyncRequest, requestFuture); } catch (Throwable t) { - reportAsyncFailure(t, requestFuture, errorNotifier); + reportAsyncFailure(t, requestFuture, asyncRequest.responseHandler()); } return requestFuture; @@ -50,8 +52,7 @@ public CompletableFuture execute(CrtAsyncRequestContext executionContext) private void doExecute(CrtAsyncRequestContext executionContext, AsyncExecuteRequest asyncRequest, - CompletableFuture requestFuture, - ResponseHandlerErrorNotifier errorNotifier) { + CompletableFuture requestFuture) { MetricCollector metricCollector = executionContext.metricCollector(); boolean shouldPublishMetrics = metricCollector != null && !(metricCollector instanceof NoOpMetricCollector); @@ -70,55 +71,26 @@ private void doExecute(CrtAsyncRequestContext executionContext, CrtStreamHandler streamHandler = new CrtStreamHandler(streamFuture); HttpStreamBaseResponseHandler crtResponseHandler = - CrtResponseAdapter.toCrtResponseHandler(requestFuture, asyncRequest.responseHandler(), streamHandler, errorNotifier); - - CrtRequestBodyPublisherSubscriber bodySubscriber = - new CrtRequestBodyPublisherSubscriber(streamHandler, requestFuture, errorNotifier); + CrtResponseAdapter.toCrtResponseHandler(requestFuture, asyncRequest.responseHandler(), streamHandler); long finalAcquireStartTime = acquireStartTime; - executionContext.streamManager().acquireStream(crtRequest, crtResponseHandler, true) - .handle((stream, throwable) -> { + executionContext.streamManager().acquireStream(crtRequest, crtResponseHandler) + .whenComplete((stream, throwable) -> { if (shouldPublishMetrics) { reportMetrics(executionContext.streamManager(), metricCollector, finalAcquireStartTime); } if (throwable != null) { - handleAcquireFailure(throwable, streamFuture, requestFuture, errorNotifier); - return null; - } - try { - stream.activate(); - } catch (Throwable t) { - // Stream is acquired but not activated and not yet published via - // streamFuture. No other path can reach it, so clean it up directly. - stream.cancel(); - stream.close(); - handleAcquireFailure(t, streamFuture, requestFuture, errorNotifier); - return null; + Throwable toThrow = wrapCrtException(throwable); + streamFuture.completeExceptionally(toThrow); + reportAsyncFailure(toThrow, requestFuture, asyncRequest.responseHandler()); + } else { + streamFuture.complete(stream); } - streamFuture.complete(stream); - asyncRequest.requestContentPublisher().subscribe(bodySubscriber); - return null; - }).exceptionally(t -> { - // Reached when the handle lambda throws (e.g., publisher.subscribe). - // closeConnection is a no-op if the stream isn't in streamFuture yet; - // otherwise it tears down the published stream. - streamHandler.closeConnection(); - handleAcquireFailure(t, streamFuture, requestFuture, errorNotifier); - return null; }); } - private void handleAcquireFailure(Throwable t, - CompletableFuture streamFuture, - CompletableFuture requestFuture, - ResponseHandlerErrorNotifier errorNotifier) { - Throwable toThrow = wrapCrtException(t); - streamFuture.completeExceptionally(toThrow); - reportAsyncFailure(toThrow, requestFuture, errorNotifier); - } - /** * Create the execution future and set up the cancellation logic. * @@ -141,10 +113,18 @@ private CompletableFuture createAsyncExecutionFuture(AsyncExecuteRequest r return future; } + /** + * Notify the provided response handler and future of the failure. + */ private void reportAsyncFailure(Throwable cause, CompletableFuture executeFuture, - ResponseHandlerErrorNotifier errorNotifier) { - errorNotifier.tryNotifyError(cause); + SdkAsyncHttpResponseHandler responseHandler) { + try { + responseHandler.onError(cause); + } catch (Exception e) { + log.error(() -> "SdkAsyncHttpResponseHandler " + responseHandler + " threw an exception in onError. It will be " + + "ignored.", e); + } executeFuture.completeExceptionally(cause); } } diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/ResponseHandlerErrorNotifier.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/ResponseHandlerErrorNotifier.java deleted file mode 100644 index 94651087c0b8..000000000000 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/ResponseHandlerErrorNotifier.java +++ /dev/null @@ -1,57 +0,0 @@ -/* - * 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.internal; - -import java.util.concurrent.atomic.AtomicBoolean; -import software.amazon.awssdk.annotations.SdkInternalApi; -import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; -import software.amazon.awssdk.utils.Logger; - -/** - * Coordinates at-most-once delivery of {@link SdkAsyncHttpResponseHandler#onError(Throwable)} across - * the multiple paths (response adapter, request body subscriber, executor failure paths) that may - * all attempt to notify the same handler. - */ -@SdkInternalApi -public final class ResponseHandlerErrorNotifier { - - private static final Logger log = Logger.loggerFor(ResponseHandlerErrorNotifier.class); - - private final SdkAsyncHttpResponseHandler responseHandler; - private final AtomicBoolean notified = new AtomicBoolean(false); - - public ResponseHandlerErrorNotifier(SdkAsyncHttpResponseHandler responseHandler) { - this.responseHandler = responseHandler; - } - - /** - * Atomically delivers {@code onError(t)} to the response handler at most once. Returns - * {@code true} if this caller delivered the notification, {@code false} if another caller - * already did. Exceptions thrown by the handler are caught and logged. - */ - public boolean tryNotifyError(Throwable t) { - if (!notified.compareAndSet(false, true)) { - return false; - } - try { - responseHandler.onError(t); - } catch (Exception e) { - log.error(() -> "SdkAsyncHttpResponseHandler " + responseHandler + " threw an exception in onError. It will be " - + "ignored.", e); - } - return true; - } -} diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestAdapter.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestAdapter.java index f7fef41cfc4e..e97b1adb514e 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestAdapter.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestAdapter.java @@ -50,12 +50,14 @@ public static HttpRequestBase toAsyncCrtRequest(CrtAsyncRequestContext request) .map(value -> "?" + value) .orElse(""); String path = encodedPath + encodedQueryString; + CrtRequestBodyAdapter crtRequestBodyAdapter = new CrtRequestBodyAdapter(sdkExecuteRequest.requestContentPublisher(), + request.readBufferSize()); HttpHeader[] crtHeaderArray = asArray(createAsyncHttpHeaderList(sdkRequest.getUri(), sdkExecuteRequest, request.protocol())); return new HttpRequest(method, path, crtHeaderArray, - null); + crtRequestBodyAdapter); } public static HttpRequest toCrtRequest(CrtRequestContext request) { diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyAdapter.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyAdapter.java new file mode 100644 index 000000000000..6fa64d8a011d --- /dev/null +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyAdapter.java @@ -0,0 +1,50 @@ +/* + * 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.internal.request; + +import java.nio.ByteBuffer; +import java.util.concurrent.atomic.AtomicBoolean; +import software.amazon.awssdk.annotations.SdkInternalApi; +import software.amazon.awssdk.crt.http.HttpRequestBodyStream; +import software.amazon.awssdk.http.async.SdkHttpContentPublisher; +import software.amazon.awssdk.utils.async.ByteBufferStoringSubscriber; +import software.amazon.awssdk.utils.async.ByteBufferStoringSubscriber.TransferResult; + +@SdkInternalApi +final class CrtRequestBodyAdapter implements HttpRequestBodyStream { + private final SdkHttpContentPublisher requestPublisher; + private final ByteBufferStoringSubscriber requestBodySubscriber; + private final AtomicBoolean subscribed = new AtomicBoolean(false); + + CrtRequestBodyAdapter(SdkHttpContentPublisher requestPublisher, long readLimit) { + this.requestPublisher = requestPublisher; + this.requestBodySubscriber = new ByteBufferStoringSubscriber(readLimit); + } + + @Override + public boolean sendRequestBody(ByteBuffer bodyBytesOut) { + if (subscribed.compareAndSet(false, true)) { + requestPublisher.subscribe(requestBodySubscriber); + } + + return requestBodySubscriber.transferTo(bodyBytesOut) == TransferResult.END_OF_STREAM; + } + + @Override + public long getLength() { + return requestPublisher.contentLength().orElse(0L); + } +} diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriber.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriber.java deleted file mode 100644 index 5bfcda2a0499..000000000000 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriber.java +++ /dev/null @@ -1,120 +0,0 @@ -/* - * 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.internal.request; - -import java.nio.ByteBuffer; -import java.util.concurrent.CompletableFuture; -import java.util.concurrent.atomic.AtomicBoolean; -import org.reactivestreams.Subscriber; -import org.reactivestreams.Subscription; -import software.amazon.awssdk.annotations.SdkInternalApi; -import software.amazon.awssdk.http.crt.internal.CrtStreamHandler; -import software.amazon.awssdk.http.crt.internal.ResponseHandlerErrorNotifier; -import software.amazon.awssdk.utils.Validate; - -/** - * Subscriber that consumes the SDK request body publisher and pushes each chunk to the CRT stream - * via {@link CrtStreamHandler#writeData(byte[], boolean)}. Signals end-of-stream on {@code onComplete}, - * and tears down the stream on errors from either the publisher or CRT. - */ -@SdkInternalApi -public final class CrtRequestBodyPublisherSubscriber implements Subscriber { - - private final CrtStreamHandler streamHandler; - private final CompletableFuture executeFuture; - private final ResponseHandlerErrorNotifier errorNotifier; - private final AtomicBoolean terminated = new AtomicBoolean(false); - // Per Reactive Streams rule 2.7, calls to subscription.request and subscription.cancel must - // be performed serially. This lock provides the happens-before edge between calls made on - // different threads (publisher thread for onSubscribe; CRT event loop thread for whenComplete - // callbacks; either for handleError). - private final Object subscriptionLock = new Object(); - - private Subscription subscription; - - public CrtRequestBodyPublisherSubscriber(CrtStreamHandler streamHandler, - CompletableFuture executeFuture, - ResponseHandlerErrorNotifier errorNotifier) { - this.streamHandler = streamHandler; - this.executeFuture = executeFuture; - this.errorNotifier = errorNotifier; - } - - @Override - public void onSubscribe(Subscription s) { - Validate.paramNotNull(s, "s"); - synchronized (subscriptionLock) { - if (this.subscription != null) { - s.cancel(); - return; - } - this.subscription = s; - s.request(1); - } - } - - @Override - public void onNext(ByteBuffer buf) { - Validate.paramNotNull(buf, "buf"); - byte[] bytes = new byte[buf.remaining()]; - buf.get(bytes); - CompletableFuture writeFuture = streamHandler.writeData(bytes, false); - writeFuture.whenComplete((v, err) -> { - if (err != null) { - handleError(err, true); - } else { - requestNext(); - } - }); - } - - @Override - public void onComplete() { - streamHandler.writeData(null, true).whenComplete((v, err) -> { - if (err != null) { - handleError(err, false); - } - }); - } - - @Override - public void onError(Throwable t) { - Validate.paramNotNull(t, "t"); - handleError(t, false); - } - - private void requestNext() { - synchronized (subscriptionLock) { - subscription.request(1); - } - } - - private void handleError(Throwable t, boolean cancelSubscription) { - if (!terminated.compareAndSet(false, true)) { - return; - } - streamHandler.closeConnection(); - errorNotifier.tryNotifyError(t); - executeFuture.completeExceptionally(t); - if (cancelSubscription) { - synchronized (subscriptionLock) { - if (subscription != null) { - subscription.cancel(); - } - } - } - } -} diff --git a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/CrtResponseAdapter.java b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/CrtResponseAdapter.java index 4023ae24372d..250edb9733f5 100644 --- a/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/CrtResponseAdapter.java +++ b/http-clients/aws-crt-client/src/main/java/software/amazon/awssdk/http/crt/internal/response/CrtResponseAdapter.java @@ -31,7 +31,6 @@ import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; import software.amazon.awssdk.http.crt.AwsCrtAsyncHttpClient; import software.amazon.awssdk.http.crt.internal.CrtStreamHandler; -import software.amazon.awssdk.http.crt.internal.ResponseHandlerErrorNotifier; import software.amazon.awssdk.utils.Logger; import software.amazon.awssdk.utils.Validate; import software.amazon.awssdk.utils.async.SimplePublisher; @@ -51,36 +50,31 @@ public final class CrtResponseAdapter implements HttpStreamBaseResponseHandler { private final SdkHttpResponse.Builder responseBuilder; private final ResponseHandlerHelper responseHandlerHelper; private final CrtStreamHandler streamHandler; - private final ResponseHandlerErrorNotifier errorNotifier; private CrtResponseAdapter(CompletableFuture completionFuture, SdkAsyncHttpResponseHandler responseHandler, - CrtStreamHandler streamHandler, - ResponseHandlerErrorNotifier errorNotifier) { - this(completionFuture, responseHandler, new SimplePublisher<>(), streamHandler, errorNotifier); + CrtStreamHandler streamHandler) { + this(completionFuture, responseHandler, new SimplePublisher<>(), streamHandler); } @SdkTestInternalApi public CrtResponseAdapter(CompletableFuture completionFuture, SdkAsyncHttpResponseHandler responseHandler, SimplePublisher simplePublisher, - CrtStreamHandler streamHandler, - ResponseHandlerErrorNotifier errorNotifier) { + CrtStreamHandler streamHandler) { this.completionFuture = Validate.paramNotNull(completionFuture, "completionFuture"); this.responseHandler = Validate.paramNotNull(responseHandler, "responseHandler"); this.responseBuilder = SdkHttpResponse.builder(); this.responsePublisher = simplePublisher; this.streamHandler = streamHandler; - this.errorNotifier = Validate.paramNotNull(errorNotifier, "errorNotifier"); this.responseHandlerHelper = new ResponseHandlerHelper(responseBuilder); } public static HttpStreamBaseResponseHandler toCrtResponseHandler( CompletableFuture requestFuture, SdkAsyncHttpResponseHandler responseHandler, - CrtStreamHandler streamHandler, - ResponseHandlerErrorNotifier errorNotifier) { - return new CrtResponseAdapter(requestFuture, responseHandler, streamHandler, errorNotifier); + CrtStreamHandler streamHandler) { + return new CrtResponseAdapter(requestFuture, responseHandler, streamHandler); } @Override @@ -146,7 +140,15 @@ private void onFailedResponseComplete(HttpException error) { } private void failResponseHandlerAndFuture(Throwable error) { - errorNotifier.tryNotifyError(error); + callResponseHandlerOnError(error); completionFuture.completeExceptionally(error); } + + private void callResponseHandlerOnError(Throwable error) { + try { + responseHandler.onError(error); + } catch (RuntimeException e) { + log.warn(() -> "Exception raised from SdkAsyncHttpResponseHandler#onError.", e); + } + } } diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutorTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutorTest.java index e5355c047072..89d5e2fb2f65 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutorTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtAsyncRequestExecutorTest.java @@ -23,16 +23,11 @@ import java.io.IOException; import java.net.ConnectException; import java.net.URI; -import java.nio.ByteBuffer; -import java.nio.charset.StandardCharsets; -import java.util.AbstractMap.SimpleEntry; -import java.util.ArrayList; -import java.util.List; -import java.util.Map.Entry; -import java.util.Optional; +import javax.net.ssl.SSLHandshakeException; import java.util.concurrent.CompletableFuture; import java.util.stream.Stream; -import javax.net.ssl.SSLHandshakeException; +import java.util.AbstractMap.SimpleEntry; +import java.util.Map.Entry; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -43,8 +38,6 @@ import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; -import org.reactivestreams.Subscriber; -import org.reactivestreams.Subscription; import software.amazon.awssdk.crt.CrtRuntimeException; import software.amazon.awssdk.crt.http.HttpException; import software.amazon.awssdk.crt.http.HttpRequestBase; @@ -55,7 +48,6 @@ import software.amazon.awssdk.http.SdkHttpFullRequest; import software.amazon.awssdk.http.async.AsyncExecuteRequest; import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; -import software.amazon.awssdk.http.async.SdkHttpContentPublisher; import software.amazon.awssdk.utils.CompletableFutureUtils; @ExtendWith(MockitoExtension.class) @@ -113,9 +105,7 @@ public void execute_acquireStreamFails_invokesOnErrorAndWrapsWithIOException() { CrtAsyncRequestContext context = crtAsyncRequestContext(); CompletableFuture completableFuture = new CompletableFuture<>(); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), Mockito.any(HttpStreamBaseResponseHandler.class))) .thenReturn(completableFuture); completableFuture.completeExceptionally(exception); @@ -135,9 +125,7 @@ public void execute_crtRuntimeException_invokesOnError() { CrtAsyncRequestContext context = crtAsyncRequestContext(); CompletableFuture completableFuture = CompletableFutureUtils.failedFuture(exception); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), Mockito.any(HttpStreamBaseResponseHandler.class))) .thenReturn(completableFuture); CompletableFuture executeFuture = requestExecutor.execute(context); @@ -155,9 +143,7 @@ public void execute_requestCancelled_invokesOnError() { CrtAsyncRequestContext context = crtAsyncRequestContext(); CompletableFuture completableFuture = new CompletableFuture<>(); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), Mockito.any(HttpStreamBaseResponseHandler.class))) .thenReturn(completableFuture); CompletableFuture executeFuture = requestExecutor.execute(context); @@ -177,9 +163,7 @@ public void execute_retryableHttpException_wrapsWithIOException() { CrtAsyncRequestContext context = crtAsyncRequestContext(); CompletableFuture completableFuture = CompletableFutureUtils.failedFuture(exception); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), Mockito.any(HttpStreamBaseResponseHandler.class))) .thenReturn(completableFuture); CompletableFuture executeFuture = requestExecutor.execute(context); @@ -196,9 +180,7 @@ public void execute_httpException_mapsToCorrectException(Entry completableFuture = CompletableFutureUtils.failedFuture(exception); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), Mockito.any(HttpStreamBaseResponseHandler.class))) .thenReturn(completableFuture); CompletableFuture executeFuture = requestExecutor.execute(context); @@ -211,9 +193,7 @@ public void execute_nonRetryableHttpException_doesNotWrapWithIOException() { CrtAsyncRequestContext context = crtAsyncRequestContext(); CompletableFuture completableFuture = CompletableFutureUtils.failedFuture(exception); - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) + Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), Mockito.any(HttpStreamBaseResponseHandler.class))) .thenReturn(completableFuture); CompletableFuture executeFuture = requestExecutor.execute(context); @@ -226,335 +206,6 @@ public void execute_nonRetryableHttpException_doesNotWrapWithIOException() { assertThatThrownBy(executeFuture::join).hasCause(exception); } - @Test - public void execute_streamActivateThrows_failsRequestFutureWithIoExceptionAndInvokesOnError() { - CrtRuntimeException activateError = new CrtRuntimeException("activate failed"); - CrtAsyncRequestContext context = crtAsyncRequestContext(); - - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) - .thenReturn(CompletableFuture.completedFuture(httpStream)); - Mockito.doThrow(activateError).when(httpStream).activate(); - - CompletableFuture executeFuture = requestExecutor.execute(context); - - assertThat(executeFuture).hasFailedWithThrowableThat() - .isInstanceOf(IOException.class) - .hasCauseInstanceOf(CrtRuntimeException.class); - ArgumentCaptor errorCaptor = ArgumentCaptor.forClass(Throwable.class); - Mockito.verify(responseHandler, Mockito.times(1)).onError(errorCaptor.capture()); - assertThat(errorCaptor.getValue()).isInstanceOf(IOException.class) - .hasCauseInstanceOf(CrtRuntimeException.class); - // Verify the acquired stream was cancelled and closed so the connection is not leaked. - Mockito.verify(httpStream).cancel(); - Mockito.verify(httpStream).close(); - } - - @Test - public void execute_publisherSubscribeThrowsSynchronously_failsRequestFutureAndInvokesOnError() { - RuntimeException subscribeError = new RuntimeException("subscribe failure"); - SdkHttpContentPublisher throwingPublisher = new SdkHttpContentPublisher() { - @Override - public Optional contentLength() { - return Optional.of(0L); - } - - @Override - public void subscribe(Subscriber s) { - throw subscribeError; - } - }; - CrtAsyncRequestContext context = crtAsyncRequestContext(throwingPublisher); - - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) - .thenReturn(CompletableFuture.completedFuture(httpStream)); - - CompletableFuture executeFuture = requestExecutor.execute(context); - - assertThat(executeFuture).hasFailedWithThrowableThat().isSameAs(subscribeError); - Mockito.verify(responseHandler, Mockito.times(1)).onError(subscribeError); - // Verify the acquired stream was cancelled and closed so the connection is not leaked. - Mockito.verify(httpStream).cancel(); - Mockito.verify(httpStream).close(); - } - - @Test - public void execute_publisherDeliversBody_writesAllChunksAndEndOfStream() { - byte[] chunk1 = "hello ".getBytes(StandardCharsets.UTF_8); - byte[] chunk2 = "world".getBytes(StandardCharsets.UTF_8); - TestPublisher publisher = TestPublisher.builder() - .contentLength(chunk1.length + chunk2.length) - .emit(ByteBuffer.wrap(chunk1)) - .emit(ByteBuffer.wrap(chunk2)) - .complete() - .build(); - CrtAsyncRequestContext context = crtAsyncRequestContext(publisher); - - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) - .thenReturn(CompletableFuture.completedFuture(httpStream)); - Mockito.when(httpStream.writeData(Mockito.any(byte[].class), Mockito.eq(false))) - .thenReturn(CompletableFuture.completedFuture(null)); - Mockito.when(httpStream.writeData(Mockito.isNull(), Mockito.eq(true))) - .thenReturn(CompletableFuture.completedFuture(null)); - - requestExecutor.execute(context); - - ArgumentCaptor dataCaptor = ArgumentCaptor.forClass(byte[].class); - Mockito.verify(httpStream, Mockito.times(2)).writeData(dataCaptor.capture(), Mockito.eq(false)); - assertThat(dataCaptor.getAllValues().get(0)).isEqualTo(chunk1); - assertThat(dataCaptor.getAllValues().get(1)).isEqualTo(chunk2); - Mockito.verify(httpStream, Mockito.times(1)).writeData(Mockito.isNull(), Mockito.eq(true)); - } - - @Test - public void execute_publisherSignalsError_failsExecuteFutureAndClosesConnection() { - RuntimeException publisherError = new RuntimeException("publisher failure"); - byte[] chunk = "data".getBytes(StandardCharsets.UTF_8); - TestPublisher publisher = TestPublisher.builder() - .contentLength(chunk.length) - .emit(ByteBuffer.wrap(chunk)) - .error(publisherError) - .build(); - CrtAsyncRequestContext context = crtAsyncRequestContext(publisher); - - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) - .thenReturn(CompletableFuture.completedFuture(httpStream)); - Mockito.when(httpStream.writeData(Mockito.any(byte[].class), Mockito.eq(false))) - .thenReturn(CompletableFuture.completedFuture(null)); - - CompletableFuture executeFuture = requestExecutor.execute(context); - - Mockito.verify(responseHandler).onError(publisherError); - Mockito.verify(httpStream).cancel(); - Mockito.verify(httpStream).close(); - assertThat(executeFuture).hasFailedWithThrowableThat().isSameAs(publisherError); - } - - @Test - public void execute_writeDataFutureFails_failsExecuteFutureAndClosesConnection() { - RuntimeException writeError = new RuntimeException("write failure"); - byte[] chunk = "data".getBytes(StandardCharsets.UTF_8); - TestPublisher publisher = TestPublisher.builder() - .contentLength(chunk.length) - .emit(ByteBuffer.wrap(chunk)) - .complete() - .build(); - CrtAsyncRequestContext context = crtAsyncRequestContext(publisher); - - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) - .thenReturn(CompletableFuture.completedFuture(httpStream)); - Mockito.when(httpStream.writeData(Mockito.any(byte[].class), Mockito.eq(false))) - .thenReturn(CompletableFutureUtils.failedFuture(writeError)); - // After the write failure cancels the subscription, the publisher must not emit further - // signals per Reactive Streams rule 1.6, but stub the EOS write defensively in case the - // subscriber's onComplete still fires before cancellation propagates. - Mockito.when(httpStream.writeData(Mockito.isNull(), Mockito.eq(true))) - .thenReturn(CompletableFuture.completedFuture(null)); - - CompletableFuture executeFuture = requestExecutor.execute(context); - - Mockito.verify(responseHandler).onError(writeError); - Mockito.verify(httpStream).cancel(); - Mockito.verify(httpStream).close(); - assertThat(executeFuture).hasFailedWithThrowableThat().isSameAs(writeError); - } - - @Test - public void execute_emptyBodyPublisher_subscribesAndSendsEndOfStreamMarker() { - // createProvider("") returns a publisher with contentLength = Optional.of(0L) that emits - // onComplete with no onNext. The subscriber should send writeData(null, true) as the EOS - // marker and never call writeData with body bytes. - CrtAsyncRequestContext context = crtAsyncRequestContext(); - - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) - .thenReturn(CompletableFuture.completedFuture(httpStream)); - Mockito.when(httpStream.writeData(Mockito.isNull(), Mockito.eq(true))) - .thenReturn(CompletableFuture.completedFuture(null)); - - requestExecutor.execute(context); - - ArgumentCaptor manualWritesCaptor = ArgumentCaptor.forClass(Boolean.class); - Mockito.verify(streamManager).acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - manualWritesCaptor.capture()); - assertThat(manualWritesCaptor.getValue()).isTrue(); - Mockito.verify(httpStream, Mockito.never()).writeData(Mockito.any(byte[].class), Mockito.eq(false)); - Mockito.verify(httpStream).writeData(Mockito.isNull(), Mockito.eq(true)); - } - - @Test - public void execute_chunkedBodyPublisher_useManualDataWritesIsTrue() { - byte[] chunk = "data".getBytes(StandardCharsets.UTF_8); - TestPublisher publisher = TestPublisher.builder() - .unknownContentLength() - .emit(ByteBuffer.wrap(chunk)) - .complete() - .build(); - CrtAsyncRequestContext context = crtAsyncRequestContext(publisher); - - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) - .thenReturn(CompletableFuture.completedFuture(httpStream)); - Mockito.when(httpStream.writeData(Mockito.any(byte[].class), Mockito.eq(false))) - .thenReturn(CompletableFuture.completedFuture(null)); - Mockito.when(httpStream.writeData(Mockito.isNull(), Mockito.eq(true))) - .thenReturn(CompletableFuture.completedFuture(null)); - - requestExecutor.execute(context); - - ArgumentCaptor manualWritesCaptor = ArgumentCaptor.forClass(Boolean.class); - Mockito.verify(streamManager).acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - manualWritesCaptor.capture()); - assertThat(manualWritesCaptor.getValue()).isTrue(); - Mockito.verify(httpStream).writeData(Mockito.eq(chunk), Mockito.eq(false)); - Mockito.verify(httpStream).writeData(Mockito.isNull(), Mockito.eq(true)); - } - - @Test - public void execute_nonEmptyBodyPublisher_useManualDataWritesIsTrue() { - byte[] chunk = "data".getBytes(StandardCharsets.UTF_8); - TestPublisher publisher = TestPublisher.builder() - .contentLength(chunk.length) - .emit(ByteBuffer.wrap(chunk)) - .complete() - .build(); - CrtAsyncRequestContext context = crtAsyncRequestContext(publisher); - - Mockito.when(streamManager.acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - Mockito.anyBoolean())) - .thenReturn(CompletableFuture.completedFuture(httpStream)); - Mockito.when(httpStream.writeData(Mockito.any(byte[].class), Mockito.eq(false))) - .thenReturn(CompletableFuture.completedFuture(null)); - Mockito.when(httpStream.writeData(Mockito.isNull(), Mockito.eq(true))) - .thenReturn(CompletableFuture.completedFuture(null)); - - requestExecutor.execute(context); - - ArgumentCaptor manualWritesCaptor = ArgumentCaptor.forClass(Boolean.class); - Mockito.verify(streamManager).acquireStream(Mockito.any(HttpRequestBase.class), - Mockito.any(HttpStreamBaseResponseHandler.class), - manualWritesCaptor.capture()); - assertThat(manualWritesCaptor.getValue()).isTrue(); - } - - private CrtAsyncRequestContext crtAsyncRequestContext(SdkHttpContentPublisher publisher) { - SdkHttpFullRequest request = createRequest(URI.create("http://localhost")); - return CrtAsyncRequestContext.builder() - .readBufferSize(2000) - .streamManager(streamManager) - .request(AsyncExecuteRequest.builder() - .request(request) - .requestContentPublisher(publisher) - .responseHandler(responseHandler) - .build()) - .build(); - } - - /** - * A test {@link SdkHttpContentPublisher} that emits a fixed sequence of buffers, then either - * completes or signals an error. All emissions are delivered synchronously inside the first - * {@code request(n)} call. - */ - private static final class TestPublisher implements SdkHttpContentPublisher { - private final List buffers; - private final boolean complete; - private final Throwable error; - private final Optional contentLength; - - private TestPublisher(Builder b) { - this.buffers = b.buffers; - this.complete = b.complete; - this.error = b.error; - this.contentLength = b.contentLength; - } - - static Builder builder() { - return new Builder(); - } - - @Override - public Optional contentLength() { - return contentLength; - } - - @Override - public void subscribe(Subscriber s) { - s.onSubscribe(new Subscription() { - private boolean delivered; - - @Override - public void request(long n) { - if (delivered) { - return; - } - delivered = true; - for (ByteBuffer b : buffers) { - s.onNext(b); - } - if (error != null) { - s.onError(error); - } else if (complete) { - s.onComplete(); - } - } - - @Override - public void cancel() { - } - }); - } - - static final class Builder { - private final List buffers = new ArrayList<>(); - private boolean complete; - private Throwable error; - private Optional contentLength = Optional.of(0L); - - Builder emit(ByteBuffer buf) { - buffers.add(buf); - return this; - } - - Builder complete() { - this.complete = true; - return this; - } - - Builder error(Throwable t) { - this.error = t; - return this; - } - - Builder contentLength(long len) { - this.contentLength = Optional.of(len); - return this; - } - - Builder unknownContentLength() { - this.contentLength = Optional.empty(); - return this; - } - - TestPublisher build() { - return new TestPublisher(this); - } - } - } - private CrtAsyncRequestContext crtAsyncRequestContext() { SdkHttpFullRequest request = createRequest(URI.create("http://localhost")); return CrtAsyncRequestContext.builder() diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtResponseHandlerTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtResponseHandlerTest.java index 5068af83d699..aba67504c5a0 100644 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtResponseHandlerTest.java +++ b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/CrtResponseHandlerTest.java @@ -34,7 +34,6 @@ import software.amazon.awssdk.http.SdkHttpResponse; import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; import software.amazon.awssdk.http.crt.internal.CrtStreamHandler; -import software.amazon.awssdk.http.crt.internal.ResponseHandlerErrorNotifier; import software.amazon.awssdk.http.crt.internal.response.CrtResponseAdapter; import software.amazon.awssdk.utils.async.SimplePublisher; @@ -46,8 +45,7 @@ HttpStreamBaseResponseHandler responseHandler(CrtStreamHandler streamHandler) { executionAttributes) -> null, Function.identity(), new ExecutionAttributes()); responseHandler.prepare(); - return CrtResponseAdapter.toCrtResponseHandler(requestFuture, responseHandler, streamHandler, - new ResponseHandlerErrorNotifier(responseHandler)); + return CrtResponseAdapter.toCrtResponseHandler(requestFuture, responseHandler, streamHandler); } @Override @@ -57,16 +55,14 @@ HttpStreamBaseResponseHandler responseHandlerWithMockedPublisher(SimplePublisher executionAttributes) -> null, Function.identity(), new ExecutionAttributes()); responseHandler.prepare(); - return new CrtResponseAdapter(requestFuture, responseHandler, simplePublisher, streamHandler, - new ResponseHandlerErrorNotifier(responseHandler)); + return new CrtResponseAdapter(requestFuture, responseHandler, simplePublisher, streamHandler); } @Test void onResponseComplete_publisherCancelled_closesStream() { SdkAsyncHttpResponseHandler responseHandler = new TestAsyncHttpResponseHandler(); - HttpStreamBaseResponseHandler crtResponseHandler = CrtResponseAdapter.toCrtResponseHandler(requestFuture, responseHandler, streamHandler, - new ResponseHandlerErrorNotifier(responseHandler)); + HttpStreamBaseResponseHandler crtResponseHandler = CrtResponseAdapter.toCrtResponseHandler(requestFuture, responseHandler, streamHandler); HttpHeader[] httpHeaders = getHttpHeaders(); crtResponseHandler.onResponseHeaders(httpStream, 200, HttpHeaderBlock.MAIN.getValue(), httpHeaders); diff --git a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriberTckTest.java b/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriberTckTest.java deleted file mode 100644 index ffd98e90802b..000000000000 --- a/http-clients/aws-crt-client/src/test/java/software/amazon/awssdk/http/crt/internal/request/CrtRequestBodyPublisherSubscriberTckTest.java +++ /dev/null @@ -1,68 +0,0 @@ -/* - * 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.internal.request; - -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyBoolean; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; - -import java.nio.ByteBuffer; -import java.util.concurrent.CompletableFuture; -import org.reactivestreams.Subscriber; -import org.reactivestreams.tck.SubscriberBlackboxVerification; -import org.reactivestreams.tck.TestEnvironment; -import software.amazon.awssdk.crt.http.HttpStreamBase; -import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler; -import software.amazon.awssdk.http.crt.internal.CrtStreamHandler; -import software.amazon.awssdk.http.crt.internal.ResponseHandlerErrorNotifier; - -/** - * TCK black-box verification for {@link CrtRequestBodyPublisherSubscriber}. - * - *

Black-box is appropriate because the subscriber's behavior depends on its constructor-injected - * collaborators ({@link CrtStreamHandler}, {@link CompletableFuture}, {@link SdkAsyncHttpResponseHandler}). - * The test wires a real {@code CrtStreamHandler} backed by a mocked {@link HttpStreamBase} so no JNI - * code is exercised. - */ -public class CrtRequestBodyPublisherSubscriberTckTest extends SubscriberBlackboxVerification { - - public CrtRequestBodyPublisherSubscriberTckTest() { - super(new TestEnvironment()); - } - - @Override - public Subscriber createSubscriber() { - HttpStreamBase mockStream = mock(HttpStreamBase.class); - when(mockStream.writeData(any(byte[].class), anyBoolean())) - .thenReturn(CompletableFuture.completedFuture(null)); - // writeData(null, true) is the end-of-stream signal sent on onComplete; stub it too. - when(mockStream.writeData(null, true)) - .thenReturn(CompletableFuture.completedFuture(null)); - - CrtStreamHandler streamHandler = new CrtStreamHandler(CompletableFuture.completedFuture(mockStream)); - - return new CrtRequestBodyPublisherSubscriber( - streamHandler, - new CompletableFuture<>(), - new ResponseHandlerErrorNotifier(mock(SdkAsyncHttpResponseHandler.class))); - } - - @Override - public ByteBuffer createElement(int element) { - return ByteBuffer.wrap(new byte[]{(byte) element}); - } -}