From 7c279ef1acbea287377a80758903d751826e63ac Mon Sep 17 00:00:00 2001 From: Zoe Wang <33073555+zoewangg@users.noreply.github.com> Date: Wed, 6 Aug 2025 16:50:59 -0700 Subject: [PATCH 01/16] Override split method in byte array based response transformer --- .../ByteArrayAsyncResponseTransformer.java | 14 + .../async/ByteArraySplittingTransformer.java | 242 ++++++++ .../src/test/resources/log4j2.properties | 4 +- .../multipart/DownloadObjectHelper.java | 5 +- .../multipart/MultipartDownloadTestUtil.java | 67 ++ ...artClientGetObjectToBytesWiremockTest.java | 581 ++++++++++++++++++ ...MultipartClientGetObjectWiremockTest.java} | 96 ++- 7 files changed, 985 insertions(+), 24 deletions(-) create mode 100644 core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java create mode 100644 services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java rename services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/{MultipartDownloaderSubscriberWiremockTest.java => S3MultipartClientGetObjectWiremockTest.java} (63%) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java index d1103ea2a2de..c86bcb50395e 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java @@ -20,10 +20,12 @@ import java.io.ByteArrayOutputStream; import java.nio.ByteBuffer; import java.util.concurrent.CompletableFuture; +import java.util.function.Consumer; import org.reactivestreams.Subscriber; import org.reactivestreams.Subscription; import software.amazon.awssdk.annotations.SdkInternalApi; import software.amazon.awssdk.core.ResponseBytes; +import software.amazon.awssdk.core.SplittingTransformerConfiguration; import software.amazon.awssdk.core.async.AsyncResponseTransformer; import software.amazon.awssdk.core.async.SdkPublisher; import software.amazon.awssdk.utils.BinaryUtils; @@ -65,6 +67,17 @@ public void exceptionOccurred(Throwable throwable) { cf.completeExceptionally(throwable); } + @Override + public SplitResult> split(SplittingTransformerConfiguration splitConfig) { + CompletableFuture> future = new CompletableFuture<>(); + SdkPublisher> transformer = + new ByteArraySplittingTransformer(this, future); + return AsyncResponseTransformer.SplitResult.>builder() + .publisher(transformer) + .resultFuture(future) + .build(); + } + @Override public String name() { return TransformerType.BYTES.getName(); @@ -108,4 +121,5 @@ public void onComplete() { resultFuture.complete(baos.toByteArray()); } } + } diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java new file mode 100644 index 000000000000..9ef959ece266 --- /dev/null +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java @@ -0,0 +1,242 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.core.internal.async; + +import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; +import org.reactivestreams.Subscriber; +import org.reactivestreams.Subscription; +import software.amazon.awssdk.core.ResponseBytes; +import software.amazon.awssdk.core.async.AsyncResponseTransformer; +import software.amazon.awssdk.core.async.SdkPublisher; +import software.amazon.awssdk.core.exception.SdkClientException; +import software.amazon.awssdk.utils.CompletableFutureUtils; +import software.amazon.awssdk.utils.Logger; +import software.amazon.awssdk.utils.async.DelegatingBufferingSubscriber; +import software.amazon.awssdk.utils.async.SimplePublisher; + +public class ByteArraySplittingTransformer implements SdkPublisher> { + private static final Logger log = Logger.loggerFor(ByteArraySplittingTransformer.class); + private final AsyncResponseTransformer> upstreamResponseTransformer; + private final SimplePublisher> simplePublisher = new SimplePublisher<>(); + private final CompletableFuture> resultFuture; + private Subscriber> downstreamSubscriber; + private final AtomicInteger partNumber = new AtomicInteger(0); + private final AtomicReference responseT = new AtomicReference<>(); + + /** + * This publisher is used to send the bytes received from the downstream subscriber's transformers to a + * {@link DelegatingBufferingSubscriber} that will buffer a number of bytes up to {@code maximumBufferSize}. + */ + private final SimplePublisher publisherToUpstream = new SimplePublisher<>(); + /** + * The amount requested by the downstream subscriber that is still left to fulfill. Updated. when the + * {@link Subscription#request(long) request} method is called on the downstream subscriber's subscription. Corresponds to the + * number of {@code AsyncResponseTransformer} that will be published to the downstream subscriber. + */ + private final AtomicLong outstandingDemand = new AtomicLong(0); + + /** + * This flag stops the current thread from publishing transformers while another thread is already publishing. + */ + private final AtomicBoolean emitting = new AtomicBoolean(false); + + private final Object lock = new Object(); + + /** + * Set to true once {@code .onStream()} is called on the upstreamResponseTransformer + */ + private boolean onStreamCalled; + + /** + * Set to true once {@code .cancel()} is called in the subscription of the downstream subscriber, or if the + * {@code resultFuture} is cancelled. + */ + private final AtomicBoolean isCancelled = new AtomicBoolean(false); + + private final Map buffers; + + public ByteArraySplittingTransformer(AsyncResponseTransformer> upstreamResponseTransformer, + CompletableFuture> resultFuture) { + this.upstreamResponseTransformer = upstreamResponseTransformer; + this.resultFuture = resultFuture; + this.buffers = new ConcurrentHashMap<>(); + } + + @Override + public void subscribe(Subscriber> subscriber) { + this.downstreamSubscriber = subscriber; + subscriber.onSubscribe(new DownstreamSubscription()); + } + + private final class DownstreamSubscription implements Subscription { + + @Override + public void request(long n) { + if (n <= 0) { + downstreamSubscriber.onError(new IllegalArgumentException("Amount requested must be positive")); + return; + } + long newDemand = outstandingDemand.updateAndGet(current -> { + if (Long.MAX_VALUE - current < n) { + return Long.MAX_VALUE; + } + return current + n; + }); + log.trace(() -> String.format("new outstanding demand: %s", newDemand)); + emit(); + } + + @Override + public void cancel() { + log.trace(() -> String.format("received cancel signal. Current cancel state is 'isCancelled=%s'", isCancelled.get())); + if (isCancelled.compareAndSet(false, true)) { + handleSubscriptionCancel(); + } + } + } + + private void emit() { + do { + if (!emitting.compareAndSet(false, true)) { + return; + } + try { + if (doEmit()) { + return; + } + } finally { + emitting.compareAndSet(true, false); + } + } while (outstandingDemand.get() > 0); + } + + private boolean doEmit() { + long demand = outstandingDemand.get(); + + while (demand > 0) { + if (isCancelled.get()) { + return true; + } + if (outstandingDemand.get() > 0) { + demand = outstandingDemand.decrementAndGet(); + downstreamSubscriber.onNext(new IndividualTransformer(partNumber.incrementAndGet())); + } + } + return false; + } + + /** + * Handle the {@code .cancel()} signal received from the downstream subscription. Data that is being sent to the upstream + * transformer need to finish processing before we complete. One typical use case for this is completing the multipart + * download, the subscriber having reached the final part will signal that it doesn't need more + * {@link AsyncResponseTransformer}s by calling {@code .cancel()} on the subscription. + */ + private void handleSubscriptionCancel() { + synchronized (lock) { + if (downstreamSubscriber == null) { + log.trace(() -> "downstreamSubscriber already null, skipping downstreamSubscriber.onComplete()"); + return; + } + if (!onStreamCalled) { + // we never subscribe publisherToUpstream to the upstream, it would not complete + downstreamSubscriber = null; + return; + } + + // if result future is already complete (likely by exception propagation), skip. + if (resultFuture.isDone()) { + return; + } + + CompletableFuture> upstreamPrepareFuture = upstreamResponseTransformer.prepare(); + CompletableFutureUtils.forwardResultTo(upstreamPrepareFuture, resultFuture); + + upstreamResponseTransformer.onResponse(responseT.get()); + + try { + buffers.keySet().stream().sorted().forEach(index -> { + publisherToUpstream.send(buffers.get(index)).exceptionally(ex -> { + resultFuture.completeExceptionally(SdkClientException.create("unexpected error occurred", ex)); + return null; + }); + }); + + publisherToUpstream.complete().exceptionally(ex -> { + resultFuture.completeExceptionally(SdkClientException.create("unexpected error occurred", ex)); + return null; + }); + upstreamResponseTransformer.onStream(SdkPublisher.adapt(publisherToUpstream)); + + } catch (Throwable throwable) { + resultFuture.completeExceptionally(SdkClientException.create("unexpected error occurred", throwable)); + } + } + } + + private class IndividualTransformer implements AsyncResponseTransformer { + private final int partNumber; + private ByteArrayAsyncResponseTransformer delegate = new ByteArrayAsyncResponseTransformer<>(); + + private CompletableFuture future; + private List>> delegatePrepareFutures = new ArrayList<>(); + + private IndividualTransformer(int partNumber) { + this.partNumber = partNumber; + } + + @Override + public CompletableFuture prepare() { + future = new CompletableFuture<>(); + CompletableFuture> prepare = delegate.prepare(); + CompletableFutureUtils.forwardExceptionTo(prepare, future); + delegatePrepareFutures.add(prepare); + return prepare.thenApply(responseTResponseBytes -> { + buffers.put(partNumber, responseTResponseBytes.asByteBuffer()); + return responseTResponseBytes.response(); + }); + } + + @Override + public void onResponse(ResponseT response) { + responseT.compareAndSet(null, response); + delegate.onResponse(response); + } + + @Override + public void onStream(SdkPublisher publisher) { + delegate.onStream(publisher); + synchronized (lock) { + if (!onStreamCalled) { + onStreamCalled = true; + } + } + } + + @Override + public void exceptionOccurred(Throwable error) { + delegate.exceptionOccurred(error); + } + } +} diff --git a/core/sdk-core/src/test/resources/log4j2.properties b/core/sdk-core/src/test/resources/log4j2.properties index e5e68dd2faa4..0d89a7c40277 100644 --- a/core/sdk-core/src/test/resources/log4j2.properties +++ b/core/sdk-core/src/test/resources/log4j2.properties @@ -25,8 +25,8 @@ rootLogger.appenderRef.stdout.ref = ConsoleAppender # Uncomment below to enable more specific logging # -#logger.sdk.name = software.amazon.awssdk -#logger.sdk.level = debug +logger.sdk.name = software.amazon.awssdk.services.s3.internal.multipart +logger.sdk.level = debug # #logger.request.name = software.amazon.awssdk.request #logger.request.level = debug diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/DownloadObjectHelper.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/DownloadObjectHelper.java index 2d6fadc5f505..e09a065b29be 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/DownloadObjectHelper.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/DownloadObjectHelper.java @@ -23,6 +23,7 @@ import software.amazon.awssdk.services.s3.S3AsyncClient; import software.amazon.awssdk.services.s3.model.GetObjectRequest; import software.amazon.awssdk.services.s3.model.GetObjectResponse; +import software.amazon.awssdk.utils.CompletableFutureUtils; import software.amazon.awssdk.utils.Logger; @SdkInternalApi @@ -49,7 +50,9 @@ public CompletableFuture downloadObject( .build()); MultipartDownloaderSubscriber subscriber = subscriber(getObjectRequest); split.publisher().subscribe(subscriber); - return split.resultFuture(); + CompletableFuture splitFuture = split.resultFuture(); + CompletableFutureUtils.forwardExceptionTo(subscriber.future(), splitFuture); + return splitFuture; } private MultipartDownloaderSubscriber subscriber(GetObjectRequest getObjectRequest) { diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadTestUtil.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadTestUtil.java index 708972b6b0d7..ea8c51818905 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadTestUtil.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadTestUtil.java @@ -23,9 +23,12 @@ import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching; import static com.github.tomakehurst.wiremock.client.WireMock.verify; +import com.github.tomakehurst.wiremock.http.Fault; +import com.github.tomakehurst.wiremock.stubbing.Scenario; import java.util.Arrays; import java.util.List; import java.util.Random; +import java.util.UUID; import software.amazon.awssdk.services.s3.utils.AsyncResponseTransformerTestSupplier; public class MultipartDownloadTestUtil { @@ -63,6 +66,12 @@ public byte[] stubAllParts(String testBucket, String testKey, int amountOfPartTo return expectedBody; } + void stubIoError(int partNumber) { + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%s", testBucket, testKey, partNumber))) + .willReturn(aResponse() + .withFault(Fault.CONNECTION_RESET_BY_PEER))); + } + public byte[] stubForPart(String testBucket, String testKey,int part, int totalPart, int partSize) { byte[] body = new byte[partSize]; random.nextBytes(body); @@ -74,6 +83,27 @@ public byte[] stubForPart(String testBucket, String testKey,int part, int totalP return body; } + String internalErrorBody() { + return errorBody("InternalError", "We encountered an internal error. Please try again."); + } + + private String errorBody(String errorCode, String errorMessage) { + return "\n" + + "\n" + + " " + errorCode + "\n" + + " " + errorMessage + "\n" + + ""; + } + + + void stubSeverError(int partNumber, String errorBody, int totalPart) { + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%d", testBucket, testKey, partNumber))) + .willReturn(aResponse() + .withHeader("x-amz-request-id", String.valueOf(UUID.randomUUID())) + .withHeader("x-amz-mp-parts-count", String.valueOf(totalPart)) + .withStatus(500).withBody(errorBody))); + } + public void verifyCorrectAmountOfRequestsMade(int amountOfPartToTest) { String urlTemplate = ".*partNumber=%d.*"; for (int i = 1; i <= amountOfPartToTest; i++) { @@ -95,4 +125,41 @@ public byte[] stubForPartSuccess(int part, int totalPart, int partSize) { .withBody(body))); return body; } + + public byte[] stubFirst503Second200(int partNumber, int totalPart, int partSize) { + byte[] body = new byte[partSize]; + random.nextBytes(body); + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%s", testBucket, testKey, partNumber))) + .inScenario("part-retry" + partNumber) + .whenScenarioStateIs(Scenario.STARTED) + .willReturn(aResponse() + .withStatus(500) + .withHeader("x-amz-request-id", UUID.randomUUID().toString()) + .withBody(internalErrorBody())) + .willSetStateTo("retry-attempt" + partNumber)); + + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%s", testBucket, testKey, partNumber))) + .inScenario("part-retry" + partNumber) + .whenScenarioStateIs("retry-attempt" + partNumber) + .willReturn(aResponse() + .withStatus(200) + .withHeader("x-amz-mp-parts-count", String.valueOf(totalPart)) + .withHeader("x-amz-request-id", UUID.randomUUID().toString()) + .withBody(body))); + return body; + } + + public byte[] stubFirst503Second200AllParts(int totalPart, int partSize) { + + byte[] expectedBody = new byte[totalPart * partSize]; + for (int i = 0; i < totalPart; i++) { + byte[] individualBody = stubFirst503Second200(i + 1, totalPart, partSize); + System.arraycopy(individualBody, 0, expectedBody, i * partSize, individualBody.length); + } + return expectedBody; + } + + private String slowdownErrorBody() { + return errorBody("SlowDown", "Please reduce your request rate."); + } } diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java new file mode 100644 index 000000000000..14ff8c9131f4 --- /dev/null +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java @@ -0,0 +1,581 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.services.s3.internal.multipart; + +import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; +import static com.github.tomakehurst.wiremock.client.WireMock.any; +import static com.github.tomakehurst.wiremock.client.WireMock.anyUrl; +import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; +import static com.github.tomakehurst.wiremock.client.WireMock.matching; +import static com.github.tomakehurst.wiremock.client.WireMock.stubFor; +import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static com.github.tomakehurst.wiremock.client.WireMock.verify; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.github.tomakehurst.wiremock.http.Fault; +import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; +import com.github.tomakehurst.wiremock.junit5.WireMockTest; +import com.github.tomakehurst.wiremock.stubbing.Scenario; +import java.net.URI; +import java.nio.ByteBuffer; +import java.nio.charset.StandardCharsets; +import java.time.Duration; +import java.util.ArrayList; +import java.util.List; +import java.util.Random; +import java.util.UUID; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; +import org.reactivestreams.Subscriber; +import org.reactivestreams.Subscription; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.awscore.retry.AwsRetryStrategy; +import software.amazon.awssdk.core.ResponseBytes; +import software.amazon.awssdk.core.async.AsyncResponseTransformer; +import software.amazon.awssdk.core.async.SdkPublisher; +import software.amazon.awssdk.core.exception.SdkClientException; +import software.amazon.awssdk.core.interceptor.Context; +import software.amazon.awssdk.core.interceptor.ExecutionAttributes; +import software.amazon.awssdk.core.interceptor.ExecutionInterceptor; +import software.amazon.awssdk.http.SdkHttpResponse; +import software.amazon.awssdk.http.nio.netty.NettyNioAsyncHttpClient; +import software.amazon.awssdk.regions.Region; +import software.amazon.awssdk.services.s3.S3AsyncClient; +import software.amazon.awssdk.services.s3.model.GetObjectRequest; +import software.amazon.awssdk.services.s3.model.GetObjectResponse; +import software.amazon.awssdk.services.s3.model.S3Exception; + +@WireMockTest +@Timeout(value = 30, unit = TimeUnit.SECONDS) +public class S3MultipartClientGetObjectToBytesWiremockTest { + private static final CapturingInterceptor capturingInterceptor = new CapturingInterceptor(); + private static final String BUCKET = "Example-Bucket"; + private static final String KEY = "Key"; + private static final int MAX_ATTEMPTS = 7; + private static final int TOTAL_PARTS = 3; + private static final int PART_SIZE = 1024; + private static final byte[] PART_1_DATA = new byte[PART_SIZE]; + private static final byte[] PART_2_DATA = new byte[PART_SIZE]; + private static final byte[] PART_3_DATA = new byte[PART_SIZE]; + private static byte[] expectedBody; + private S3AsyncClient multipartClient; + + @BeforeAll + public static void init() { + new Random().nextBytes(PART_1_DATA); + new Random().nextBytes(PART_2_DATA); + new Random().nextBytes(PART_3_DATA); + + expectedBody = new byte[TOTAL_PARTS * PART_SIZE]; + System.arraycopy(PART_1_DATA, 0, expectedBody, 0, PART_SIZE); + System.arraycopy(PART_2_DATA, 0, expectedBody, PART_SIZE, PART_SIZE); + System.arraycopy(PART_3_DATA, 0, expectedBody, 2 * PART_SIZE, PART_SIZE); + } + + @BeforeEach + public void setup(WireMockRuntimeInfo wm) { + capturingInterceptor.clear(); + multipartClient = S3AsyncClient.builder() + .region(Region.US_EAST_1) + .endpointOverride(URI.create(wm.getHttpBaseUrl())) + .multipartEnabled(true) + .httpClientBuilder(NettyNioAsyncHttpClient.builder().maxConcurrency(100).connectionAcquisitionTimeout(Duration.ofSeconds(100))) + .credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("key", "secret"))) + .overrideConfiguration( + o -> o.retryStrategy(AwsRetryStrategy.standardRetryStrategy().toBuilder() + .maxAttempts(MAX_ATTEMPTS) + .circuitBreakerEnabled(false) + .build()) + .addExecutionInterceptor(capturingInterceptor)) + .build(); + } + + @Test + public void getObject_concurrentCallsReturn200_shouldSucceed() { + List>> futures = new ArrayList<>(); + + int numRuns = 1000; + for (int i = 0; i < numRuns; i++) { + CompletableFuture> resp = mock200Response(multipartClient, i); + futures.add(resp); + } + + CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).join(); + } + + @Test + public void getObject_single500WithinMany200s_shouldRetrySuccessfully() { + List>> futures = new ArrayList<>(); + + int numRuns = 1000; + for (int i = 0; i < numRuns; i++) { + CompletableFuture> resp = mock200Response(multipartClient, i); + futures.add(resp); + } + + CompletableFuture> requestWithRetryableError = + mockRetryableErrorThen200Response(multipartClient, 1); + futures.add(requestWithRetryableError); + + for (int i = 0; i < numRuns; i++) { + CompletableFuture> resp = mock200Response(multipartClient, i + 1000); + futures.add(resp); + } + + CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).join(); + } + + @Test + public void getObject_concurrent503s_shouldRetrySuccessfully() { + List>> futures = new ArrayList<>(); + + int numRuns = 1000; + for (int i = 0; i < numRuns; i++) { + CompletableFuture> resp = mockRetryableErrorThen200Response(multipartClient, i); + futures.add(resp); + } + + CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).join(); + } + + @Test + public void getObject_5xxErrorResponses_shouldNotReuseInitialRequestId() { + String firstRequestId = UUID.randomUUID().toString(); + String secondRequestId = UUID.randomUUID().toString(); + + stubFor(any(anyUrl()) + .inScenario("errors") + .whenScenarioStateIs(Scenario.STARTED) + .willReturn(aResponse() + .withHeader("x-amz-request-id", firstRequestId) + .withStatus(503) + .withBody(internalErrorBody())) + .willSetStateTo("SecondAttempt")); + + stubFor(any(anyUrl()) + .inScenario("errors") + .whenScenarioStateIs("SecondAttempt") + .willReturn(aResponse() + .withHeader("x-amz-request-id", secondRequestId) + .withStatus(500))); + + assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), + AsyncResponseTransformer.toBytes()).join()) + .isInstanceOf(CompletionException.class) + .hasCauseInstanceOf(S3Exception.class); + + + List responses = capturingInterceptor.getResponses(); + assertEquals(MAX_ATTEMPTS, responses.size(), () -> String.format("Expected exactly %s responses", MAX_ATTEMPTS)); + + String actualFirstRequestId = responses.get(0).firstMatchingHeader("x-amz-request-id").orElse(null); + String actualSecondRequestId = responses.get(1).firstMatchingHeader("x-amz-request-id").orElse(null); + + assertNotNull(actualFirstRequestId); + assertNotNull(actualSecondRequestId); + + assertNotEquals(actualFirstRequestId, actualSecondRequestId); + + assertEquals(firstRequestId, actualFirstRequestId); + assertEquals(secondRequestId, actualSecondRequestId); + + assertEquals(503, responses.get(0).statusCode()); + assertEquals(500, responses.get(1).statusCode()); + + verify(MAX_ATTEMPTS, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); + verify(0, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY)))); + verify(0, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); + } + + @Test + public void getObject_ioException_shouldRetryAndFail() { + String firstRequestId = UUID.randomUUID().toString(); + String secondRequestId = UUID.randomUUID().toString(); + + stubIoError(1); + assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), + AsyncResponseTransformer.toBytes()).join()) + .isInstanceOf(CompletionException.class) + .hasCauseInstanceOf(SdkClientException.class).hasMessageContaining("The connection was closed") + .hasStackTraceContaining("Error encountered during GetObjectRequest"); + + verify(MAX_ATTEMPTS, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); + verify(0, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY)))); + verify(0, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); + } + + + @Test + public void multipartDownload_200Response_shouldSucceed() { + stub200SuccessPart1(); + stub200SuccessPart2(); + stub200SuccessPart3(); + + CompletableFuture> future = + multipartClient.getObject(GetObjectRequest.builder().bucket(BUCKET).key(KEY).build(), + AsyncResponseTransformer.toBytes()); + + ResponseBytes response = future.join(); + byte[] actualBody = response.asByteArray(); + assertArrayEquals(expectedBody, actualBody, "Downloaded body should match expected combined parts"); + + // Verify that all 3 parts were requested only once + verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); + verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY)))); + verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); + } + + @Test + public void multipartDownload_secondPartNonRetryableError_shouldFail() { + stub200SuccessPart1(); + stubError(2, errorBody(String.valueOf(500), "Internal Error")); + + CompletableFuture> future = + multipartClient.getObject(GetObjectRequest.builder().bucket(BUCKET).key(KEY).build(), + AsyncResponseTransformer.toBytes()); + + assertThatThrownBy(() -> future.join()).hasCauseInstanceOf(S3Exception.class) + .hasStackTraceContaining("Error encountered " + + "during " + + "GetObjectRequest"); + + verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); + verify(MAX_ATTEMPTS, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY)))); + verify(0, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); + } + + @Test + public void multipartDownload_503OnFirstPart_shouldRetrySuccessfully() { + // Stub Part 1 - 503 on first attempt, 200 on retry + String part1Scenario = "part1-retry"; + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))) + .inScenario(part1Scenario) + .whenScenarioStateIs(Scenario.STARTED) + .willReturn(aResponse() + .withStatus(503) + .withHeader("x-amz-request-id", UUID.randomUUID().toString()) + .withBody("\n" + + "\n" + + " SlowDown\n" + + " Please reduce your request rate.\n" + + "")) + .willSetStateTo("retry-attempt")); + + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))) + .inScenario(part1Scenario) + .whenScenarioStateIs("retry-attempt") + .willReturn(aResponse() + .withStatus(200) + .withHeader("x-amz-mp-parts-count", String.valueOf(TOTAL_PARTS)) + .withHeader("x-amz-request-id", UUID.randomUUID().toString()) + .withBody(PART_1_DATA))); + + stub200SuccessPart2(); + stub200SuccessPart3(); + + CompletableFuture> future = + multipartClient.getObject(GetObjectRequest.builder().bucket(BUCKET).key(KEY).build(), + AsyncResponseTransformer.toBytes()); + + ResponseBytes response = future.join(); + byte[] actualBody = response.asByteArray(); + assertArrayEquals(expectedBody, actualBody, "Downloaded body should match expected combined parts"); + + // Verify that part 1 was requested twice (initial 503 + retry) + verify(2, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); + verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY)))); + verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); + } + + @Test + public void multipartDownload_503OnFirstPartAndSecondPart_shouldRetrySuccessfully() { + // Stub Part 1 - 503 on first attempt, 200 on retry + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))) + .inScenario("part1-retry") + .whenScenarioStateIs(Scenario.STARTED) + .willReturn(aResponse() + .withStatus(503) + .withHeader("x-amz-request-id", UUID.randomUUID().toString()) + .withBody(slowdownErrorBody())) + .willSetStateTo("retry-attempt")); + + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))) + .inScenario("part1-retry") + .whenScenarioStateIs("retry-attempt") + .willReturn(aResponse() + .withStatus(200) + .withHeader("x-amz-mp-parts-count", String.valueOf(TOTAL_PARTS)) + .withHeader("x-amz-request-id", UUID.randomUUID().toString()) + .withBody(PART_1_DATA))); + + + // Stub Part 2 - 503 on first attempt, 200 on retry + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY))) + .inScenario("part2-retry") + .whenScenarioStateIs(Scenario.STARTED) + .willReturn(aResponse() + .withStatus(500) + .withHeader("x-amz-request-id", UUID.randomUUID().toString()) + .withBody(internalErrorBody())) + .willSetStateTo("retry-attempt")); + + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY))) + .inScenario("part2-retry") + .whenScenarioStateIs("retry-attempt") + .willReturn(aResponse() + .withStatus(200) + .withHeader("x-amz-mp-parts-count", String.valueOf(TOTAL_PARTS)) + .withHeader("x-amz-request-id", UUID.randomUUID().toString()) + .withBody(PART_2_DATA))); + + stub200SuccessPart3(); + ResponseBytes response = multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), + AsyncResponseTransformer.toBytes()).join(); + + byte[] actualBody = response.asByteArray(); + assertArrayEquals(expectedBody, actualBody, "Downloaded body should match expected combined parts"); + + // Verify that part 1 was requested twice (initial 503 + retry) + verify(2, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); + // Verify that part 2 was requested once (no retry) + verify(2, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY)))); + verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); + } + + @Test + public void getObject_iOError_shouldRetrySuccessfully() { + String requestId = UUID.randomUUID().toString(); + + stubFor(any(anyUrl()) + .inScenario("io-error") + .whenScenarioStateIs(Scenario.STARTED) + .willReturn(aResponse() + .withFault(Fault.CONNECTION_RESET_BY_PEER)) + .willSetStateTo("retry")); + + stubFor(any(anyUrl()) + .inScenario("io-error") + .whenScenarioStateIs("retry") + .willReturn(aResponse() + .withStatus(200) + .withHeader("x-amz-request-id", requestId) + .withBody("Hello World"))); + + ResponseBytes response = multipartClient.getObject(GetObjectRequest.builder() + .bucket(BUCKET) + .key(KEY) + .build(), + AsyncResponseTransformer.toBytes()).join(); + + assertArrayEquals("Hello World".getBytes(StandardCharsets.UTF_8), response.asByteArray()); + + verify(2, getRequestedFor(urlEqualTo("/" + BUCKET + "/" + KEY + "?partNumber=1"))); + + List responses = capturingInterceptor.getResponses(); + String finalRequestId = responses.get(responses.size() - 1) + .firstMatchingHeader("x-amz-request-id") + .orElse(null); + + assertEquals(requestId, finalRequestId); + } + + private String errorBody(String errorCode, String errorMessage) { + return "\n" + + "\n" + + " " + errorCode + "\n" + + " " + errorMessage + "\n" + + ""; + } + + private String internalErrorBody() { + return errorBody("InternalError", "We encountered an internal error. Please try again."); + } + + private String slowdownErrorBody() { + return errorBody("SlowDown", "Please reduce your request rate."); + } + + private void stubError(int partNumber, String errorBody) { + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=" + partNumber, BUCKET, KEY))) + .willReturn(aResponse() + .withHeader("x-amz-request-id", String.valueOf(UUID.randomUUID())) + .withHeader("x-amz-mp-parts-count", String.valueOf(TOTAL_PARTS)) + .withStatus(500).withBody(errorBody))); + } + + private void stubIoError(int partNumber) { + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=" + partNumber, BUCKET, KEY))) + .willReturn(aResponse() + .withFault(Fault.CONNECTION_RESET_BY_PEER))); + } + + private void stub200SuccessPart1() { + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))) + .willReturn(aResponse() + .withHeader("x-amz-request-id", String.valueOf(UUID.randomUUID())) + .withHeader("x-amz-mp-parts-count", String.valueOf(TOTAL_PARTS)) + .withStatus(200).withBody(PART_1_DATA))); + } + + private void stub200SuccessPart2() { + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY))) + .willReturn(aResponse() + .withStatus(200) + .withHeader("x-amz-mp-parts-count", String.valueOf(TOTAL_PARTS)) + .withHeader("x-amz-request-id", UUID.randomUUID().toString()) + .withBody(PART_2_DATA))); + } + + private void stub200SuccessPart3() { + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY))) + .willReturn(aResponse() + .withStatus(200) + .withHeader("x-amz-mp-parts-count", String.valueOf(TOTAL_PARTS)) + .withHeader("x-amz-request-id", UUID.randomUUID().toString()) + .withBody(PART_3_DATA))); + } + + private CompletableFuture> mock200Response(S3AsyncClient s3Client, int runNumber) { + String runId = runNumber + " success"; + + stubFor(any(anyUrl()) + .withHeader("RunNum", matching(runId)) + .inScenario(runId) + .whenScenarioStateIs(Scenario.STARTED) + .willReturn(aResponse().withStatus(200) + .withHeader("x-amz-request-id", String.valueOf(UUID.randomUUID())) + .withBody("Hello World"))); + + return s3Client.getObject(r -> r.bucket(BUCKET).key("key") + .overrideConfiguration(c -> c.putHeader("RunNum", runId)), + AsyncResponseTransformer.toBytes()); + } + + private CompletableFuture> mockRetryableErrorThen200Response(S3AsyncClient s3Client, int runNumber) { + String runId = String.valueOf(runNumber); + + stubFor(any(anyUrl()) + .withHeader("RunNum", matching(runId)) + .inScenario(runId) + .whenScenarioStateIs(Scenario.STARTED) + .willReturn(aResponse() + .withHeader("x-amz-request-id", String.valueOf(UUID.randomUUID())) + .withStatus(500) + .withBody(internalErrorBody()) + ) + .willSetStateTo("SecondAttempt" + runId)); + + stubFor(any(anyUrl()) + .inScenario(runId) + .withHeader("RunNum", matching(runId)) + .whenScenarioStateIs("SecondAttempt" + runId) + .willReturn(aResponse().withStatus(200) + .withHeader("x-amz-request-id", String.valueOf(UUID.randomUUID())) + .withBody("Hello World"))); + + return s3Client.getObject(r -> r.bucket(BUCKET).key("key") + .overrideConfiguration(c -> c.putHeader("RunNum", runId)), + AsyncResponseTransformer.toBytes()); + } + + static class CapturingInterceptor implements ExecutionInterceptor { + private final List responses = new ArrayList<>(); + + @Override + public void afterTransmission(Context.AfterTransmission context, ExecutionAttributes executionAttributes) { + responses.add(context.httpResponse()); + } + + public List getResponses() { + return new ArrayList<>(responses); + } + + public void clear() { + responses.clear(); + } + } + + /** + * Custom AsyncResponseTransformer that simulates an error occurring after onStream() has been called + */ + private static final class StreamingErrorTransformer + implements AsyncResponseTransformer> { + + private final CompletableFuture> future = new CompletableFuture<>(); + private final AtomicBoolean errorThrown = new AtomicBoolean(); + private final AtomicBoolean onStreamCalled = new AtomicBoolean(); + + @Override + public CompletableFuture> prepare() { + return future; + } + + @Override + public void onResponse(GetObjectResponse response) { + // + } + + @Override + public void onStream(SdkPublisher publisher) { + onStreamCalled.set(true); + publisher.subscribe(new Subscriber() { + private Subscription subscription; + + @Override + public void onSubscribe(Subscription s) { + this.subscription = s; + s.request(1); + } + + @Override + public void onNext(ByteBuffer byteBuffer) { + if (errorThrown.compareAndSet(false, true)) { + future.completeExceptionally(new RuntimeException()); + subscription.cancel(); + } + } + + @Override + public void onError(Throwable t) { + future.completeExceptionally(t); + } + + @Override + public void onComplete() { + // + } + }); + } + + @Override + public void exceptionOccurred(Throwable throwable) { + future.completeExceptionally(throwable); + } + } +} diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriberWiremockTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java similarity index 63% rename from services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriberWiremockTest.java rename to services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java index 1c6eb666a9c2..0186101dd63a 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriberWiremockTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java @@ -17,8 +17,10 @@ import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; import static com.github.tomakehurst.wiremock.client.WireMock.get; +import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; import static com.github.tomakehurst.wiremock.client.WireMock.stubFor; import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; +import static com.github.tomakehurst.wiremock.client.WireMock.verify; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -31,7 +33,10 @@ import java.util.Arrays; import java.util.List; import java.util.UUID; +import java.util.stream.IntStream; import java.util.stream.Stream; +import org.apache.commons.lang3.RandomStringUtils; +import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; @@ -41,6 +46,9 @@ import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; import software.amazon.awssdk.core.SplittingTransformerConfiguration; import software.amazon.awssdk.core.async.AsyncResponseTransformer; +import software.amazon.awssdk.core.exception.SdkClientException; +import software.amazon.awssdk.core.internal.async.ByteArrayAsyncResponseTransformer; +import software.amazon.awssdk.core.retry.RetryMode; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.s3.S3AsyncClient; import software.amazon.awssdk.services.s3.S3Configuration; @@ -50,21 +58,25 @@ import software.amazon.awssdk.utils.Pair; @WireMockTest -class MultipartDownloaderSubscriberWiremockTest { +class S3MultipartClientGetObjectWiremockTest { private final String testBucket = "test-bucket"; private final String testKey = "test-key"; + private final static int MAX_ATTEMPT = 3; private S3AsyncClient s3AsyncClient; private MultipartDownloadTestUtil util; @BeforeEach public void init(WireMockRuntimeInfo wiremock) { + wiremock.getWireMock().resetMappings(); s3AsyncClient = S3AsyncClient.builder() .credentialsProvider(StaticCredentialsProvider.create( AwsBasicCredentials.create("key", "secret"))) .region(Region.US_WEST_2) .endpointOverride(URI.create("http://localhost:" + wiremock.getHttpPort())) + .multipartEnabled(true) + .overrideConfiguration(o -> o.retryStrategy(b -> b.maxAttempts(MAX_ATTEMPT))) .serviceConfiguration(S3Configuration.builder() .pathStyleAccessEnabled(true) .build()) @@ -83,15 +95,8 @@ void happyPath_shouldReceiveAllBodyPartInCorrectOrder(AsyncResponseTransform SplittingTransformerConfiguration.builder() .bufferSizeInBytes(1024 * 32L) .build()); - Subscriber> subscriber = new MultipartDownloaderSubscriber( - s3AsyncClient, - GetObjectRequest.builder() - .bucket(testBucket) - .key(testKey) - .build()); - split.publisher().subscribe(subscriber); - T response = split.resultFuture().join(); + T response = s3AsyncClient.getObject(b -> b.bucket(testBucket).key(testKey), transformer).join(); byte[] body = supplier.body(response); assertArrayEquals(expectedBody, body); @@ -100,7 +105,7 @@ void happyPath_shouldReceiveAllBodyPartInCorrectOrder(AsyncResponseTransform @ParameterizedTest @MethodSource("argumentsProvider") - void errorOnFirstRequest_shouldCompleteExceptionally(AsyncResponseTransformerTestSupplier supplier, + void nonRetryableErrorOnFirstPart_shouldFail(AsyncResponseTransformerTestSupplier supplier, int amountOfPartToTest, int partSize) { stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", testBucket, testKey))).willReturn( @@ -112,21 +117,14 @@ void errorOnFirstRequest_shouldCompleteExceptionally(AsyncResponseTransforme SplittingTransformerConfiguration.builder() .bufferSizeInBytes(1024 * 32L) .build()); - Subscriber> subscriber = new MultipartDownloaderSubscriber( - s3AsyncClient, - GetObjectRequest.builder() - .bucket(testBucket) - .key(testKey) - .build()); - split.publisher().subscribe(subscriber); - assertThatThrownBy(() -> split.resultFuture().join()) + assertThatThrownBy(() -> s3AsyncClient.getObject(b -> b.bucket(testBucket).key(testKey), transformer).join()) .hasMessageContaining("test error message"); } @ParameterizedTest @MethodSource("argumentsProvider") - void errorOnThirdRequest_shouldCompleteExceptionallyOnlyPartsGreaterThanTwo( + void nonRetryableErrorOnThirdPart_shouldCompleteExceptionallyOnlyPartsGreaterThanTwo( AsyncResponseTransformerTestSupplier supplier, int amountOfPartToTest, int partSize) { @@ -149,9 +147,8 @@ void errorOnThirdRequest_shouldCompleteExceptionallyOnlyPartsGreaterThanTwo( .build()); if (partSize > 1) { - split.publisher().subscribe(subscriber); assertThatThrownBy(() -> { - T res = split.resultFuture().join(); + T res = s3AsyncClient.getObject(b -> b.bucket(testBucket).key(testKey), transformer).join(); supplier.body(res); }).hasMessageContaining("test error message"); } else { @@ -160,6 +157,63 @@ void errorOnThirdRequest_shouldCompleteExceptionallyOnlyPartsGreaterThanTwo( } } + @ParameterizedTest + @MethodSource("argumentsProvider") + void ioError_retryExhausted_shouldFail(AsyncResponseTransformerTestSupplier supplier, + int amountOfPartToTest, + int partSize) { + util.stubIoError( 1); + AsyncResponseTransformer transformer = supplier.transformer(); + + assertThatThrownBy(() -> s3AsyncClient.getObject(b -> b.bucket(testBucket).key(testKey), transformer).join()) + .hasMessageContaining("The connection was closed during the request"); + verify(MAX_ATTEMPT, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", + testBucket, testKey)))); + } + + @ParameterizedTest + @MethodSource("argumentsProvider") + void serverError_retryExhausted_shouldFail(AsyncResponseTransformerTestSupplier supplier, + int amountOfPartToTest, + int partSize) { + util.stubSeverError(1, util.internalErrorBody(), amountOfPartToTest); + AsyncResponseTransformer transformer = supplier.transformer(); + + + // Only enable this test for ByteArrayAsyncResponseTransformer because only ByteArrayAsyncResponseTransformer supports + // retry + Assumptions.assumeTrue(transformer instanceof ByteArrayAsyncResponseTransformer); + + assertThatThrownBy(() -> s3AsyncClient.getObject(b -> b.bucket(testBucket).key(testKey), transformer).join()) + .hasMessageContaining(" We encountered an internal error"); + verify(MAX_ATTEMPT, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", + testBucket, testKey)))); + } + + @ParameterizedTest + @MethodSource("argumentsProvider") + void serverError_retrySucceeds_shouldSucceed(AsyncResponseTransformerTestSupplier supplier, + int amountOfPartToTest, + int partSize) { + + byte[] expectedBody = util.stubFirst503Second200AllParts(amountOfPartToTest, partSize); + AsyncResponseTransformer transformer = supplier.transformer(); + + // Only enable this test for ByteArrayAsyncResponseTransformer because only ByteArrayAsyncResponseTransformer supports + // retry + Assumptions.assumeTrue(transformer instanceof ByteArrayAsyncResponseTransformer); + + T response = s3AsyncClient.getObject(b -> b.bucket(testBucket).key(testKey), transformer).join(); + + byte[] body = supplier.body(response); + assertArrayEquals(expectedBody, body); + util.verifyCorrectAmountOfRequestsMade(amountOfPartToTest); + + IntStream.range(1, amountOfPartToTest) + .forEach(index -> verify(2, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber="+ index, + testBucket, testKey))))); + } + private static Stream argumentsProvider() { // amount of part, individual part size List> partSizes = Arrays.asList( From 00e97c102bfa3baa45d07b8a1472a7662779f663 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Thu, 7 Aug 2025 21:26:06 -0700 Subject: [PATCH 02/16] Rename duplicate test --- ...est.java => S3MultipartClientGetObjectWiremockTest2.java} | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) rename services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/{S3MultipartClientGetObjectWiremockTest.java => S3MultipartClientGetObjectWiremockTest2.java} (98%) diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest2.java similarity index 98% rename from services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java rename to services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest2.java index 0186101dd63a..5a024901949e 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest2.java @@ -35,7 +35,6 @@ import java.util.UUID; import java.util.stream.IntStream; import java.util.stream.Stream; -import org.apache.commons.lang3.RandomStringUtils; import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.params.ParameterizedTest; @@ -46,9 +45,7 @@ import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; import software.amazon.awssdk.core.SplittingTransformerConfiguration; import software.amazon.awssdk.core.async.AsyncResponseTransformer; -import software.amazon.awssdk.core.exception.SdkClientException; import software.amazon.awssdk.core.internal.async.ByteArrayAsyncResponseTransformer; -import software.amazon.awssdk.core.retry.RetryMode; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.s3.S3AsyncClient; import software.amazon.awssdk.services.s3.S3Configuration; @@ -58,7 +55,7 @@ import software.amazon.awssdk.utils.Pair; @WireMockTest -class S3MultipartClientGetObjectWiremockTest { +class S3MultipartClientGetObjectWiremockTest2 { private final String testBucket = "test-bucket"; private final String testKey = "test-key"; From 49c7ef2375d79ce85f910afa3432adf5762d3b66 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Thu, 7 Aug 2025 23:36:57 -0700 Subject: [PATCH 03/16] Update tests and refactor --- .../ByteArrayAsyncResponseTransformer.java | 6 +- .../async/ByteArraySplittingTransformer.java | 24 +- ...artClientGetObjectToBytesWiremockTest.java | 128 ++-------- ...3MultipartClientGetObjectWiremockTest.java | 197 +++++++++++++-- ...MultipartClientGetObjectWiremockTest2.java | 233 ------------------ .../multipart/UploadObjectHelperTest.java | 8 +- ...oadWithUnknownContentLengthHelperTest.java | 10 +- .../MultipartDownloadTestUtils.java} | 45 ++-- .../MultipartUploadTestUtils.java} | 6 +- 9 files changed, 254 insertions(+), 403 deletions(-) delete mode 100644 services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest2.java rename services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/{MultipartDownloadTestUtil.java => utils/MultipartDownloadTestUtils.java} (91%) rename services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/{MpuTestUtils.java => utils/MultipartUploadTestUtils.java} (96%) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java index c86bcb50395e..77ade8761ced 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java @@ -20,7 +20,6 @@ import java.io.ByteArrayOutputStream; import java.nio.ByteBuffer; import java.util.concurrent.CompletableFuture; -import java.util.function.Consumer; import org.reactivestreams.Subscriber; import org.reactivestreams.Subscription; import software.amazon.awssdk.annotations.SdkInternalApi; @@ -69,9 +68,11 @@ public void exceptionOccurred(Throwable throwable) { @Override public SplitResult> split(SplittingTransformerConfiguration splitConfig) { + // TODO - splitConfig not used - support this or log warning/update javdocs + CompletableFuture> future = new CompletableFuture<>(); SdkPublisher> transformer = - new ByteArraySplittingTransformer(this, future); + new ByteArraySplittingTransformer<>(this, future); return AsyncResponseTransformer.SplitResult.>builder() .publisher(transformer) .resultFuture(future) @@ -121,5 +122,4 @@ public void onComplete() { resultFuture.complete(baos.toByteArray()); } } - } diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java index 9ef959ece266..30d65a0b4b2e 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java @@ -27,6 +27,7 @@ import java.util.concurrent.atomic.AtomicReference; import org.reactivestreams.Subscriber; import org.reactivestreams.Subscription; +import software.amazon.awssdk.annotations.SdkInternalApi; import software.amazon.awssdk.core.ResponseBytes; import software.amazon.awssdk.core.async.AsyncResponseTransformer; import software.amazon.awssdk.core.async.SdkPublisher; @@ -36,13 +37,13 @@ import software.amazon.awssdk.utils.async.DelegatingBufferingSubscriber; import software.amazon.awssdk.utils.async.SimplePublisher; +@SdkInternalApi public class ByteArraySplittingTransformer implements SdkPublisher> { private static final Logger log = Logger.loggerFor(ByteArraySplittingTransformer.class); private final AsyncResponseTransformer> upstreamResponseTransformer; - private final SimplePublisher> simplePublisher = new SimplePublisher<>(); private final CompletableFuture> resultFuture; private Subscriber> downstreamSubscriber; - private final AtomicInteger partNumber = new AtomicInteger(0); + private final AtomicInteger onNextSignalsSent = new AtomicInteger(0); private final AtomicReference responseT = new AtomicReference<>(); /** @@ -77,7 +78,8 @@ public class ByteArraySplittingTransformer implements SdkPublisher buffers; - public ByteArraySplittingTransformer(AsyncResponseTransformer> upstreamResponseTransformer, + public ByteArraySplittingTransformer(AsyncResponseTransformer> + upstreamResponseTransformer, CompletableFuture> resultFuture) { this.upstreamResponseTransformer = upstreamResponseTransformer; this.resultFuture = resultFuture; @@ -141,7 +143,7 @@ private boolean doEmit() { } if (outstandingDemand.get() > 0) { demand = outstandingDemand.decrementAndGet(); - downstreamSubscriber.onNext(new IndividualTransformer(partNumber.incrementAndGet())); + downstreamSubscriber.onNext(new IndividualTransformer(onNextSignalsSent.incrementAndGet())); } } return false; @@ -195,15 +197,15 @@ private void handleSubscriptionCancel() { } } - private class IndividualTransformer implements AsyncResponseTransformer { - private final int partNumber; - private ByteArrayAsyncResponseTransformer delegate = new ByteArrayAsyncResponseTransformer<>(); + private final class IndividualTransformer implements AsyncResponseTransformer { + private final int onNextCount; + private final ByteArrayAsyncResponseTransformer delegate = new ByteArrayAsyncResponseTransformer<>(); private CompletableFuture future; - private List>> delegatePrepareFutures = new ArrayList<>(); + private final List>> delegatePrepareFutures = new ArrayList<>(); - private IndividualTransformer(int partNumber) { - this.partNumber = partNumber; + private IndividualTransformer(int onNextCount) { + this.onNextCount = onNextCount; } @Override @@ -213,7 +215,7 @@ public CompletableFuture prepare() { CompletableFutureUtils.forwardExceptionTo(prepare, future); delegatePrepareFutures.add(prepare); return prepare.thenApply(responseTResponseBytes -> { - buffers.put(partNumber, responseTResponseBytes.asByteBuffer()); + buffers.put(onNextCount, responseTResponseBytes.asByteBuffer()); return responseTResponseBytes.response(); }); } diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java index 14ff8c9131f4..7d6d9c45dfae 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java @@ -29,14 +29,14 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertTrue; +import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartDownloadTestUtils.internalErrorBody; +import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartDownloadTestUtils.slowdownErrorBody; import com.github.tomakehurst.wiremock.http.Fault; import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; import com.github.tomakehurst.wiremock.junit5.WireMockTest; import com.github.tomakehurst.wiremock.stubbing.Scenario; import java.net.URI; -import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.time.Duration; import java.util.ArrayList; @@ -46,19 +46,15 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicBoolean; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.Timeout; -import org.reactivestreams.Subscriber; -import org.reactivestreams.Subscription; import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; import software.amazon.awssdk.awscore.retry.AwsRetryStrategy; import software.amazon.awssdk.core.ResponseBytes; import software.amazon.awssdk.core.async.AsyncResponseTransformer; -import software.amazon.awssdk.core.async.SdkPublisher; import software.amazon.awssdk.core.exception.SdkClientException; import software.amazon.awssdk.core.interceptor.Context; import software.amazon.awssdk.core.interceptor.ExecutionAttributes; @@ -213,23 +209,6 @@ public void getObject_5xxErrorResponses_shouldNotReuseInitialRequestId() { verify(0, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); } - @Test - public void getObject_ioException_shouldRetryAndFail() { - String firstRequestId = UUID.randomUUID().toString(); - String secondRequestId = UUID.randomUUID().toString(); - - stubIoError(1); - assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), - AsyncResponseTransformer.toBytes()).join()) - .isInstanceOf(CompletionException.class) - .hasCauseInstanceOf(SdkClientException.class).hasMessageContaining("The connection was closed") - .hasStackTraceContaining("Error encountered during GetObjectRequest"); - - verify(MAX_ATTEMPTS, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); - verify(0, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY)))); - verify(0, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); - } - @Test public void multipartDownload_200Response_shouldSucceed() { @@ -254,16 +233,14 @@ public void multipartDownload_200Response_shouldSucceed() { @Test public void multipartDownload_secondPartNonRetryableError_shouldFail() { stub200SuccessPart1(); - stubError(2, errorBody(String.valueOf(500), "Internal Error")); + stubError(2, internalErrorBody()); CompletableFuture> future = multipartClient.getObject(GetObjectRequest.builder().bucket(BUCKET).key(KEY).build(), AsyncResponseTransformer.toBytes()); - assertThatThrownBy(() -> future.join()).hasCauseInstanceOf(S3Exception.class) - .hasStackTraceContaining("Error encountered " - + "during " - + "GetObjectRequest"); + assertThatThrownBy(future::join).hasCauseInstanceOf(S3Exception.class) + .hasMessageContaining("We encountered an internal error. Please try again. (Service: S3, Status Code: 500"); verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); verify(MAX_ATTEMPTS, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY)))); @@ -369,7 +346,22 @@ public void multipartDownload_503OnFirstPartAndSecondPart_shouldRetrySuccessfull } @Test - public void getObject_iOError_shouldRetrySuccessfully() { + public void getObject_ioExceptionOnly_shouldExhaustRetriesAndFail() { + stubIoError(1); + stub200SuccessPart2(); + stub200SuccessPart3(); + assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), + AsyncResponseTransformer.toBytes()).join()) + .isInstanceOf(CompletionException.class) + .hasCauseInstanceOf(SdkClientException.class); + + verify(MAX_ATTEMPTS, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); + verify(0, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY)))); + verify(0, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); + } + + @Test + public void getObject_iOErrorThen200Response_shouldRetrySuccessfully() { String requestId = UUID.randomUUID().toString(); stubFor(any(anyUrl()) @@ -405,24 +397,8 @@ public void getObject_iOError_shouldRetrySuccessfully() { assertEquals(requestId, finalRequestId); } - private String errorBody(String errorCode, String errorMessage) { - return "\n" - + "\n" - + " " + errorCode + "\n" - + " " + errorMessage + "\n" - + ""; - } - - private String internalErrorBody() { - return errorBody("InternalError", "We encountered an internal error. Please try again."); - } - - private String slowdownErrorBody() { - return errorBody("SlowDown", "Please reduce your request rate."); - } - private void stubError(int partNumber, String errorBody) { - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=" + partNumber, BUCKET, KEY))) + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%d", BUCKET, KEY, partNumber))) .willReturn(aResponse() .withHeader("x-amz-request-id", String.valueOf(UUID.randomUUID())) .withHeader("x-amz-mp-parts-count", String.valueOf(TOTAL_PARTS)) @@ -430,7 +406,7 @@ private void stubError(int partNumber, String errorBody) { } private void stubIoError(int partNumber) { - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=" + partNumber, BUCKET, KEY))) + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%d", BUCKET, KEY, partNumber))) .willReturn(aResponse() .withFault(Fault.CONNECTION_RESET_BY_PEER))); } @@ -520,62 +496,4 @@ public void clear() { responses.clear(); } } - - /** - * Custom AsyncResponseTransformer that simulates an error occurring after onStream() has been called - */ - private static final class StreamingErrorTransformer - implements AsyncResponseTransformer> { - - private final CompletableFuture> future = new CompletableFuture<>(); - private final AtomicBoolean errorThrown = new AtomicBoolean(); - private final AtomicBoolean onStreamCalled = new AtomicBoolean(); - - @Override - public CompletableFuture> prepare() { - return future; - } - - @Override - public void onResponse(GetObjectResponse response) { - // - } - - @Override - public void onStream(SdkPublisher publisher) { - onStreamCalled.set(true); - publisher.subscribe(new Subscriber() { - private Subscription subscription; - - @Override - public void onSubscribe(Subscription s) { - this.subscription = s; - s.request(1); - } - - @Override - public void onNext(ByteBuffer byteBuffer) { - if (errorThrown.compareAndSet(false, true)) { - future.completeExceptionally(new RuntimeException()); - subscription.cancel(); - } - } - - @Override - public void onError(Throwable t) { - future.completeExceptionally(t); - } - - @Override - public void onComplete() { - // - } - }); - } - - @Override - public void exceptionOccurred(Throwable throwable) { - future.completeExceptionally(throwable); - } - } } diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java index 5869b1a82733..54bff6062ee4 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java @@ -25,7 +25,13 @@ import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; import static com.github.tomakehurst.wiremock.client.WireMock.verify; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.assertj.core.api.Assertions.fail; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.params.provider.Arguments.arguments; +import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartDownloadTestUtils.internalErrorBody; +import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartDownloadTestUtils.transformersSuppliers; import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; import com.github.tomakehurst.wiremock.junit5.WireMockTest; @@ -37,35 +43,55 @@ import java.nio.file.Path; import java.time.Duration; import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.UUID; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.TimeUnit; +import java.util.stream.IntStream; import java.util.stream.Stream; +import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Timeout; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.reactivestreams.Subscriber; import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.core.SplittingTransformerConfiguration; import software.amazon.awssdk.core.async.AsyncResponseTransformer; +import software.amazon.awssdk.core.internal.async.ByteArrayAsyncResponseTransformer; +import software.amazon.awssdk.core.internal.async.FileAsyncResponseTransformer; +import software.amazon.awssdk.core.internal.async.InputStreamResponseTransformer; +import software.amazon.awssdk.core.internal.async.PublisherAsyncResponseTransformer; import software.amazon.awssdk.http.nio.netty.NettyNioAsyncHttpClient; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.s3.S3AsyncClient; +import software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartDownloadTestUtils; +import software.amazon.awssdk.services.s3.model.GetObjectRequest; import software.amazon.awssdk.services.s3.model.GetObjectResponse; import software.amazon.awssdk.services.s3.model.S3Exception; +import software.amazon.awssdk.services.s3.utils.AsyncResponseTransformerTestSupplier; +import software.amazon.awssdk.utils.Pair; @WireMockTest @Timeout(value = 45, unit = TimeUnit.SECONDS) public class S3MultipartClientGetObjectWiremockTest { private static final String BUCKET = "Example-Bucket"; private static final String KEY = "Key"; + private static final int MAX_ATTEMPTS = 3; private static int fileCounter = 0; private S3AsyncClient multipartClient; + private MultipartDownloadTestUtils util; @BeforeEach - public void setup(WireMockRuntimeInfo wm) { + public void setup(WireMockRuntimeInfo wm) + { + wm.getWireMock().resetRequests(); + wm.getWireMock().resetScenarios(); + wm.getWireMock().resetMappings(); multipartClient = S3AsyncClient.builder() .region(Region.US_EAST_1) .endpointOverride(URI.create(wm.getHttpBaseUrl())) @@ -74,12 +100,156 @@ public void setup(WireMockRuntimeInfo wm) { .maxConcurrency(100) .connectionAcquisitionTimeout(Duration.ofSeconds(60))) .credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("key", "secret"))) + .overrideConfiguration(o -> o.retryStrategy(b -> b.maxAttempts(MAX_ATTEMPTS))) .build(); + util = new MultipartDownloadTestUtils(BUCKET, KEY, UUID.randomUUID().toString()); } + @ParameterizedTest + @MethodSource("partSizeAndTransformerParams") + void happyPath_shouldReceiveAllBodyPartInCorrectOrder(AsyncResponseTransformerTestSupplier supplier, + int amountOfPartToTest, + int partSize) { + byte[] expectedBody = util.stubAllParts(BUCKET, KEY, amountOfPartToTest, partSize); + AsyncResponseTransformer transformer = supplier.transformer(); + AsyncResponseTransformer.SplitResult split = transformer.split( + SplittingTransformerConfiguration.builder() + .bufferSizeInBytes(1024 * 32L) + .build()); + + T response = multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), transformer).join(); + + byte[] body = supplier.body(response); + assertArrayEquals(expectedBody, body); + util.verifyCorrectAmountOfRequestsMade(amountOfPartToTest); + } + + @ParameterizedTest + @MethodSource("partSizeAndTransformerParams") + void nonRetryableErrorOnFirstPart_shouldFail(AsyncResponseTransformerTestSupplier supplier, + int amountOfPartToTest, + int partSize) { + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))).willReturn( + aResponse() + .withStatus(400) + .withBody("400test error message"))); + AsyncResponseTransformer transformer = supplier.transformer(); + AsyncResponseTransformer.SplitResult split = transformer.split( + SplittingTransformerConfiguration.builder() + .bufferSizeInBytes(1024 * 32L) + .build()); + + assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), transformer).join()) + .hasMessageContaining("test error message"); + } + + @ParameterizedTest + @MethodSource("partSizeAndTransformerParams") + void nonRetryableErrorOnThirdPart_shouldCompleteExceptionallyOnlyPartsGreaterThanTwo( + AsyncResponseTransformerTestSupplier supplier, + int amountOfPartToTest, + int partSize) { + util.stubForPart(BUCKET, KEY, 1, 3, partSize); + util.stubForPart(BUCKET, KEY, 2, 3, partSize); + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY))).willReturn( + aResponse() + .withStatus(400) + .withBody("400test error message"))); + AsyncResponseTransformer transformer = supplier.transformer(); + AsyncResponseTransformer.SplitResult split = transformer.split( + SplittingTransformerConfiguration.builder() + .bufferSizeInBytes(1024 * 32L) + .build()); + Subscriber> subscriber = new MultipartDownloaderSubscriber( + multipartClient, + GetObjectRequest.builder() + .bucket(BUCKET) + .key(KEY) + .build()); + + if (partSize > 1) { + assertThatThrownBy(() -> { + T res = multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), transformer).join(); + supplier.body(res); + }).hasMessageContaining("test error message"); + } else { + T res = split.resultFuture().join(); + assertNotNull(supplier.body(res)); + } + } + + @ParameterizedTest + @MethodSource("partSizeAndTransformerParams") + void serverError_retryExhausted_shouldFail(AsyncResponseTransformerTestSupplier supplier, + int amountOfPartToTest, + int partSize) { + util.stubSeverError(1, internalErrorBody(), amountOfPartToTest); + AsyncResponseTransformer transformer = supplier.transformer(); + + + // Only enable this test for ByteArrayAsyncResponseTransformer because only ByteArrayAsyncResponseTransformer supports + // retry + Assumptions.assumeTrue(transformer instanceof ByteArrayAsyncResponseTransformer); + + assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), transformer).join()) + .hasMessageContaining(" We encountered an internal error"); + verify(MAX_ATTEMPTS, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", + BUCKET, KEY)))); + } + + @ParameterizedTest + @MethodSource("partSizeAndTransformerParams") + void serverError_retrySucceeds_shouldSucceed(AsyncResponseTransformerTestSupplier supplier, + int amountOfPartToTest, + int partSize) { + + byte[] expectedBody = util.stubFirst503Second200AllParts(amountOfPartToTest, partSize); + AsyncResponseTransformer transformer = supplier.transformer(); + + // Only enable this test for ByteArrayAsyncResponseTransformer because only ByteArrayAsyncResponseTransformer supports + // retry + Assumptions.assumeTrue(transformer instanceof ByteArrayAsyncResponseTransformer); + + T response = multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), transformer).join(); + + byte[] body = supplier.body(response); + assertArrayEquals(expectedBody, body); + util.verifyCorrectAmountOfRequestsMade(amountOfPartToTest); + + IntStream.range(1, amountOfPartToTest) + .forEach(index -> verify(2, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber="+ index, + BUCKET, KEY))))); + } + + private static Stream partSizeAndTransformerParams() { + // amount of part, individual part size + List> partSizes = Arrays.asList( + Pair.of(4, 16), + Pair.of(1, 1024), + Pair.of(31, 1243), + Pair.of(16, 16 * 1024), + Pair.of(1, 1024 * 1024), + Pair.of(4, 1024 * 1024), + Pair.of(1, 4 * 1024 * 1024), + Pair.of(4, 6 * 1024 * 1024), + Pair.of(7, 5 * 3752) + ); + + Stream.Builder sb = Stream.builder(); + transformersSuppliers().forEach(tr -> partSizes.forEach(p -> sb.accept(arguments(tr, p.left(), p.right())))); + return sb.build(); + } + + /** + * Testing {@link PublisherAsyncResponseTransformer}, {@link InputStreamResponseTransformer}, and + * {@link FileAsyncResponseTransformer} + *

+ * + * Retry for multipart download is supported for {@link ByteArrayAsyncResponseTransformer}, tested in + * {@link S3MultipartClientGetObjectToBytesWiremockTest}. + */ private static Stream responseTransformerFactories() { return Stream.of( - AsyncResponseTransformer::toBytes, AsyncResponseTransformer::toBlockingInputStream, AsyncResponseTransformer::toPublisher, () -> { @@ -100,6 +270,17 @@ interface TransformerFactory { AsyncResponseTransformer create(); } + @ParameterizedTest + @MethodSource("responseTransformerFactories") + public void ioError_shouldFailAndNotRetry(TransformerFactory transformerFactory) { + util.stubIoError( 1); + AsyncResponseTransformer transformer = transformerFactory.create(); + + assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), transformer).join()) + .hasCauseInstanceOf(IOException.class); + verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); + } + @ParameterizedTest @MethodSource("responseTransformerFactories") public void getObject_single500WithinMany200s_shouldNotRetryError(TransformerFactory transformerFactory) { @@ -163,16 +344,4 @@ private CompletableFuture mock200Response(S3AsyncClient s3Client, int runNumb .overrideConfiguration(c -> c.putHeader("RunNum", runId)), transformerFactory.create()); } - - private String errorBody(String errorCode, String errorMessage) { - return "\n" - + "\n" - + " " + errorCode + "\n" - + " " + errorMessage + "\n" - + ""; - } - - private String internalErrorBody() { - return errorBody("InternalError", "We encountered an internal error. Please try again."); - } } diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest2.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest2.java deleted file mode 100644 index 5a024901949e..000000000000 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest2.java +++ /dev/null @@ -1,233 +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.services.s3.internal.multipart; - -import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; -import static com.github.tomakehurst.wiremock.client.WireMock.get; -import static com.github.tomakehurst.wiremock.client.WireMock.getRequestedFor; -import static com.github.tomakehurst.wiremock.client.WireMock.stubFor; -import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo; -import static com.github.tomakehurst.wiremock.client.WireMock.verify; -import static org.assertj.core.api.Assertions.assertThatThrownBy; -import static org.junit.jupiter.api.Assertions.assertArrayEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.params.provider.Arguments.arguments; -import static software.amazon.awssdk.services.s3.internal.multipart.MultipartDownloadTestUtil.transformersSuppliers; - -import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; -import com.github.tomakehurst.wiremock.junit5.WireMockTest; -import java.net.URI; -import java.util.Arrays; -import java.util.List; -import java.util.UUID; -import java.util.stream.IntStream; -import java.util.stream.Stream; -import org.junit.jupiter.api.Assumptions; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.params.ParameterizedTest; -import org.junit.jupiter.params.provider.Arguments; -import org.junit.jupiter.params.provider.MethodSource; -import org.reactivestreams.Subscriber; -import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; -import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; -import software.amazon.awssdk.core.SplittingTransformerConfiguration; -import software.amazon.awssdk.core.async.AsyncResponseTransformer; -import software.amazon.awssdk.core.internal.async.ByteArrayAsyncResponseTransformer; -import software.amazon.awssdk.regions.Region; -import software.amazon.awssdk.services.s3.S3AsyncClient; -import software.amazon.awssdk.services.s3.S3Configuration; -import software.amazon.awssdk.services.s3.model.GetObjectRequest; -import software.amazon.awssdk.services.s3.model.GetObjectResponse; -import software.amazon.awssdk.services.s3.utils.AsyncResponseTransformerTestSupplier; -import software.amazon.awssdk.utils.Pair; - -@WireMockTest -class S3MultipartClientGetObjectWiremockTest2 { - - private final String testBucket = "test-bucket"; - private final String testKey = "test-key"; - private final static int MAX_ATTEMPT = 3; - - private S3AsyncClient s3AsyncClient; - private MultipartDownloadTestUtil util; - - @BeforeEach - public void init(WireMockRuntimeInfo wiremock) { - wiremock.getWireMock().resetMappings(); - s3AsyncClient = S3AsyncClient.builder() - .credentialsProvider(StaticCredentialsProvider.create( - AwsBasicCredentials.create("key", "secret"))) - .region(Region.US_WEST_2) - .endpointOverride(URI.create("http://localhost:" + wiremock.getHttpPort())) - .multipartEnabled(true) - .overrideConfiguration(o -> o.retryStrategy(b -> b.maxAttempts(MAX_ATTEMPT))) - .serviceConfiguration(S3Configuration.builder() - .pathStyleAccessEnabled(true) - .build()) - .build(); - util = new MultipartDownloadTestUtil(testBucket, testKey, UUID.randomUUID().toString()); - } - - @ParameterizedTest - @MethodSource("argumentsProvider") - void happyPath_shouldReceiveAllBodyPartInCorrectOrder(AsyncResponseTransformerTestSupplier supplier, - int amountOfPartToTest, - int partSize) { - byte[] expectedBody = util.stubAllParts(testBucket, testKey, amountOfPartToTest, partSize); - AsyncResponseTransformer transformer = supplier.transformer(); - AsyncResponseTransformer.SplitResult split = transformer.split( - SplittingTransformerConfiguration.builder() - .bufferSizeInBytes(1024 * 32L) - .build()); - - T response = s3AsyncClient.getObject(b -> b.bucket(testBucket).key(testKey), transformer).join(); - - byte[] body = supplier.body(response); - assertArrayEquals(expectedBody, body); - util.verifyCorrectAmountOfRequestsMade(amountOfPartToTest); - } - - @ParameterizedTest - @MethodSource("argumentsProvider") - void nonRetryableErrorOnFirstPart_shouldFail(AsyncResponseTransformerTestSupplier supplier, - int amountOfPartToTest, - int partSize) { - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", testBucket, testKey))).willReturn( - aResponse() - .withStatus(400) - .withBody("400test error message"))); - AsyncResponseTransformer transformer = supplier.transformer(); - AsyncResponseTransformer.SplitResult split = transformer.split( - SplittingTransformerConfiguration.builder() - .bufferSizeInBytes(1024 * 32L) - .build()); - - assertThatThrownBy(() -> s3AsyncClient.getObject(b -> b.bucket(testBucket).key(testKey), transformer).join()) - .hasMessageContaining("test error message"); - } - - @ParameterizedTest - @MethodSource("argumentsProvider") - void nonRetryableErrorOnThirdPart_shouldCompleteExceptionallyOnlyPartsGreaterThanTwo( - AsyncResponseTransformerTestSupplier supplier, - int amountOfPartToTest, - int partSize) { - util.stubForPart(testBucket, testKey, 1, 3, partSize); - util.stubForPart(testBucket, testKey, 2, 3, partSize); - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=3", testBucket, testKey))).willReturn( - aResponse() - .withStatus(400) - .withBody("400test error message"))); - AsyncResponseTransformer transformer = supplier.transformer(); - AsyncResponseTransformer.SplitResult split = transformer.split( - SplittingTransformerConfiguration.builder() - .bufferSizeInBytes(1024 * 32L) - .build()); - Subscriber> subscriber = new MultipartDownloaderSubscriber( - s3AsyncClient, - GetObjectRequest.builder() - .bucket(testBucket) - .key(testKey) - .build()); - - if (partSize > 1) { - assertThatThrownBy(() -> { - T res = s3AsyncClient.getObject(b -> b.bucket(testBucket).key(testKey), transformer).join(); - supplier.body(res); - }).hasMessageContaining("test error message"); - } else { - T res = split.resultFuture().join(); - assertNotNull(supplier.body(res)); - } - } - - @ParameterizedTest - @MethodSource("argumentsProvider") - void ioError_retryExhausted_shouldFail(AsyncResponseTransformerTestSupplier supplier, - int amountOfPartToTest, - int partSize) { - util.stubIoError( 1); - AsyncResponseTransformer transformer = supplier.transformer(); - - assertThatThrownBy(() -> s3AsyncClient.getObject(b -> b.bucket(testBucket).key(testKey), transformer).join()) - .hasMessageContaining("The connection was closed during the request"); - verify(MAX_ATTEMPT, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", - testBucket, testKey)))); - } - - @ParameterizedTest - @MethodSource("argumentsProvider") - void serverError_retryExhausted_shouldFail(AsyncResponseTransformerTestSupplier supplier, - int amountOfPartToTest, - int partSize) { - util.stubSeverError(1, util.internalErrorBody(), amountOfPartToTest); - AsyncResponseTransformer transformer = supplier.transformer(); - - - // Only enable this test for ByteArrayAsyncResponseTransformer because only ByteArrayAsyncResponseTransformer supports - // retry - Assumptions.assumeTrue(transformer instanceof ByteArrayAsyncResponseTransformer); - - assertThatThrownBy(() -> s3AsyncClient.getObject(b -> b.bucket(testBucket).key(testKey), transformer).join()) - .hasMessageContaining(" We encountered an internal error"); - verify(MAX_ATTEMPT, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", - testBucket, testKey)))); - } - - @ParameterizedTest - @MethodSource("argumentsProvider") - void serverError_retrySucceeds_shouldSucceed(AsyncResponseTransformerTestSupplier supplier, - int amountOfPartToTest, - int partSize) { - - byte[] expectedBody = util.stubFirst503Second200AllParts(amountOfPartToTest, partSize); - AsyncResponseTransformer transformer = supplier.transformer(); - - // Only enable this test for ByteArrayAsyncResponseTransformer because only ByteArrayAsyncResponseTransformer supports - // retry - Assumptions.assumeTrue(transformer instanceof ByteArrayAsyncResponseTransformer); - - T response = s3AsyncClient.getObject(b -> b.bucket(testBucket).key(testKey), transformer).join(); - - byte[] body = supplier.body(response); - assertArrayEquals(expectedBody, body); - util.verifyCorrectAmountOfRequestsMade(amountOfPartToTest); - - IntStream.range(1, amountOfPartToTest) - .forEach(index -> verify(2, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber="+ index, - testBucket, testKey))))); - } - - private static Stream argumentsProvider() { - // amount of part, individual part size - List> partSizes = Arrays.asList( - Pair.of(4, 16), - Pair.of(1, 1024), - Pair.of(31, 1243), - Pair.of(16, 16 * 1024), - Pair.of(1, 1024 * 1024), - Pair.of(4, 1024 * 1024), - Pair.of(1, 4 * 1024 * 1024), - Pair.of(4, 6 * 1024 * 1024), - Pair.of(7, 5 * 3752) - ); - - Stream.Builder sb = Stream.builder(); - transformersSuppliers().forEach(tr -> partSizes.forEach(p -> sb.accept(arguments(tr, p.left(), p.right())))); - return sb.build(); - } - -} \ No newline at end of file diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/UploadObjectHelperTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/UploadObjectHelperTest.java index 284a392086df..20ae807dc334 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/UploadObjectHelperTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/UploadObjectHelperTest.java @@ -24,10 +24,10 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import static software.amazon.awssdk.services.s3.internal.multipart.MpuTestUtils.s3ResumeToken; -import static software.amazon.awssdk.services.s3.internal.multipart.MpuTestUtils.stubSuccessfulCompleteMultipartCall; -import static software.amazon.awssdk.services.s3.internal.multipart.MpuTestUtils.stubSuccessfulCreateMultipartCall; -import static software.amazon.awssdk.services.s3.internal.multipart.MpuTestUtils.stubSuccessfulUploadPartCalls; +import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartUploadTestUtils.s3ResumeToken; +import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartUploadTestUtils.stubSuccessfulCompleteMultipartCall; +import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartUploadTestUtils.stubSuccessfulCreateMultipartCall; +import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartUploadTestUtils.stubSuccessfulUploadPartCalls; import static software.amazon.awssdk.services.s3.multipart.S3MultipartExecutionAttribute.PAUSE_OBSERVABLE; import static software.amazon.awssdk.services.s3.multipart.S3MultipartExecutionAttribute.RESUME_TOKEN; diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/UploadWithUnknownContentLengthHelperTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/UploadWithUnknownContentLengthHelperTest.java index 972f0b86241a..90c5bfed038c 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/UploadWithUnknownContentLengthHelperTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/UploadWithUnknownContentLengthHelperTest.java @@ -22,15 +22,13 @@ import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; -import static software.amazon.awssdk.services.s3.internal.multipart.MpuTestUtils.stubSuccessfulCompleteMultipartCall; -import static software.amazon.awssdk.services.s3.internal.multipart.MpuTestUtils.stubSuccessfulCreateMultipartCall; -import static software.amazon.awssdk.services.s3.internal.multipart.MpuTestUtils.stubSuccessfulUploadPartCalls; +import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartUploadTestUtils.stubSuccessfulCompleteMultipartCall; +import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartUploadTestUtils.stubSuccessfulCreateMultipartCall; +import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartUploadTestUtils.stubSuccessfulUploadPartCalls; -import java.io.ByteArrayInputStream; import java.io.FileInputStream; import java.io.FileNotFoundException; import java.io.IOException; -import java.io.InputStream; import java.util.List; import java.util.Optional; import java.util.concurrent.CompletableFuture; @@ -52,12 +50,10 @@ import software.amazon.awssdk.services.s3.S3AsyncClient; import software.amazon.awssdk.services.s3.model.CompleteMultipartUploadRequest; import software.amazon.awssdk.services.s3.model.CompletedPart; -import software.amazon.awssdk.services.s3.model.CreateMultipartUploadRequest; import software.amazon.awssdk.services.s3.model.PutObjectRequest; import software.amazon.awssdk.services.s3.model.PutObjectResponse; import software.amazon.awssdk.services.s3.model.UploadPartRequest; import software.amazon.awssdk.testutils.RandomTempFile; -import software.amazon.awssdk.utils.StringInputStream; public class UploadWithUnknownContentLengthHelperTest { private static final String BUCKET = "bucket"; diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadTestUtil.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/utils/MultipartDownloadTestUtils.java similarity index 91% rename from services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadTestUtil.java rename to services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/utils/MultipartDownloadTestUtils.java index ea8c51818905..d4c2ecbf047a 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadTestUtil.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/utils/MultipartDownloadTestUtils.java @@ -13,7 +13,7 @@ * permissions and limitations under the License. */ -package software.amazon.awssdk.services.s3.internal.multipart; +package software.amazon.awssdk.services.s3.internal.multipart.utils; import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; import static com.github.tomakehurst.wiremock.client.WireMock.get; @@ -31,18 +31,17 @@ import java.util.UUID; import software.amazon.awssdk.services.s3.utils.AsyncResponseTransformerTestSupplier; -public class MultipartDownloadTestUtil { +public class MultipartDownloadTestUtils { private static final String RETRY_SCENARIO = "retry"; private static final String SUCCESS_STATE = "success"; - private static final String FAILED_STATE = "failed"; - private String testBucket; - private String testKey; - private String eTag; - private Random random = new Random(); + private final String testBucket; + private final String testKey; + private final String eTag; + private final Random random = new Random(); - public MultipartDownloadTestUtil(String testBucket, String testKey, String eTag) { + public MultipartDownloadTestUtils(String testBucket, String testKey, String eTag) { this.testBucket = testBucket; this.testKey = testKey; this.eTag = eTag; @@ -66,7 +65,7 @@ public byte[] stubAllParts(String testBucket, String testKey, int amountOfPartTo return expectedBody; } - void stubIoError(int partNumber) { + public void stubIoError(int partNumber) { stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%s", testBucket, testKey, partNumber))) .willReturn(aResponse() .withFault(Fault.CONNECTION_RESET_BY_PEER))); @@ -83,20 +82,8 @@ public byte[] stubForPart(String testBucket, String testKey,int part, int totalP return body; } - String internalErrorBody() { - return errorBody("InternalError", "We encountered an internal error. Please try again."); - } - - private String errorBody(String errorCode, String errorMessage) { - return "\n" - + "\n" - + " " + errorCode + "\n" - + " " + errorMessage + "\n" - + ""; - } - - void stubSeverError(int partNumber, String errorBody, int totalPart) { + public void stubSeverError(int partNumber, String errorBody, int totalPart) { stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%d", testBucket, testKey, partNumber))) .willReturn(aResponse() .withHeader("x-amz-request-id", String.valueOf(UUID.randomUUID())) @@ -159,7 +146,19 @@ public byte[] stubFirst503Second200AllParts(int totalPart, int partSize) { return expectedBody; } - private String slowdownErrorBody() { + public static String errorBody(String errorCode, String errorMessage) { + return "\n" + + "\n" + + " " + errorCode + "\n" + + " " + errorMessage + "\n" + + ""; + } + + public static String internalErrorBody() { + return errorBody("InternalError", "We encountered an internal error. Please try again."); + } + + public static String slowdownErrorBody() { return errorBody("SlowDown", "Please reduce your request rate."); } } diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MpuTestUtils.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/utils/MultipartUploadTestUtils.java similarity index 96% rename from services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MpuTestUtils.java rename to services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/utils/MultipartUploadTestUtils.java index 23fe07ab2743..9a97bf70c1e8 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MpuTestUtils.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/utils/MultipartUploadTestUtils.java @@ -13,7 +13,7 @@ * permissions and limitations under the License. */ -package software.amazon.awssdk.services.s3.internal.multipart; +package software.amazon.awssdk.services.s3.internal.multipart.utils; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.when; @@ -33,9 +33,9 @@ import software.amazon.awssdk.services.s3.model.UploadPartResponse; import software.amazon.awssdk.services.s3.multipart.S3ResumeToken; -public final class MpuTestUtils { +public final class MultipartUploadTestUtils { - private MpuTestUtils() { + private MultipartUploadTestUtils() { } public static void stubSuccessfulHeadObjectCall(long contentLength, S3AsyncClient s3AsyncClient) { From 78058b1f0eafde5b81677f0ad4fcf369b63d4e4e Mon Sep 17 00:00:00 2001 From: hdavidh Date: Fri, 8 Aug 2025 16:30:08 -0700 Subject: [PATCH 04/16] Clean up comments --- .../internal/async/ByteArrayAsyncResponseTransformer.java | 2 -- .../core/internal/async/ByteArraySplittingTransformer.java | 6 +----- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java index 77ade8761ced..79838f739d4a 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java @@ -68,8 +68,6 @@ public void exceptionOccurred(Throwable throwable) { @Override public SplitResult> split(SplittingTransformerConfiguration splitConfig) { - // TODO - splitConfig not used - support this or log warning/update javdocs - CompletableFuture> future = new CompletableFuture<>(); SdkPublisher> transformer = new ByteArraySplittingTransformer<>(this, future); diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java index 30d65a0b4b2e..76e33e00b05a 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java @@ -46,13 +46,9 @@ public class ByteArraySplittingTransformer implements SdkPublisher responseT = new AtomicReference<>(); - /** - * This publisher is used to send the bytes received from the downstream subscriber's transformers to a - * {@link DelegatingBufferingSubscriber} that will buffer a number of bytes up to {@code maximumBufferSize}. - */ private final SimplePublisher publisherToUpstream = new SimplePublisher<>(); /** - * The amount requested by the downstream subscriber that is still left to fulfill. Updated. when the + * The amount requested by the downstream subscriber that is still left to fulfill. Updated when the * {@link Subscription#request(long) request} method is called on the downstream subscriber's subscription. Corresponds to the * number of {@code AsyncResponseTransformer} that will be published to the downstream subscriber. */ From 76d32773d0a27f3bdfcb38b3aa3ae50b69588723 Mon Sep 17 00:00:00 2001 From: David Ho <70000000+davidh44@users.noreply.github.com> Date: Fri, 8 Aug 2025 19:24:33 -0700 Subject: [PATCH 05/16] Remove unused import --- .../core/internal/async/ByteArraySplittingTransformer.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java index 76e33e00b05a..7d3b3a249067 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java @@ -34,7 +34,6 @@ import software.amazon.awssdk.core.exception.SdkClientException; import software.amazon.awssdk.utils.CompletableFutureUtils; import software.amazon.awssdk.utils.Logger; -import software.amazon.awssdk.utils.async.DelegatingBufferingSubscriber; import software.amazon.awssdk.utils.async.SimplePublisher; @SdkInternalApi From f386bfee3f21b0531efb3fe5148ee243569d7648 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 11 Aug 2025 10:58:11 -0700 Subject: [PATCH 06/16] Refactor tests --- ...artClientGetObjectToBytesWiremockTest.java | 14 +- ...3MultipartClientGetObjectWiremockTest.java | 200 ++++++------------ .../utils/MultipartDownloadTestUtils.java | 68 ------ 3 files changed, 70 insertions(+), 212 deletions(-) diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java index 7d6d9c45dfae..2a29d385999c 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java @@ -52,7 +52,6 @@ import org.junit.jupiter.api.Timeout; import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; -import software.amazon.awssdk.awscore.retry.AwsRetryStrategy; import software.amazon.awssdk.core.ResponseBytes; import software.amazon.awssdk.core.async.AsyncResponseTransformer; import software.amazon.awssdk.core.exception.SdkClientException; @@ -104,10 +103,7 @@ public void setup(WireMockRuntimeInfo wm) { .httpClientBuilder(NettyNioAsyncHttpClient.builder().maxConcurrency(100).connectionAcquisitionTimeout(Duration.ofSeconds(100))) .credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("key", "secret"))) .overrideConfiguration( - o -> o.retryStrategy(AwsRetryStrategy.standardRetryStrategy().toBuilder() - .maxAttempts(MAX_ATTEMPTS) - .circuitBreakerEnabled(false) - .build()) + o -> o.retryStrategy(b -> b.maxAttempts(MAX_ATTEMPTS)) .addExecutionInterceptor(capturingInterceptor)) .build(); } @@ -151,7 +147,7 @@ public void getObject_single500WithinMany200s_shouldRetrySuccessfully() { public void getObject_concurrent503s_shouldRetrySuccessfully() { List>> futures = new ArrayList<>(); - int numRuns = 1000; + int numRuns = 100; for (int i = 0; i < numRuns; i++) { CompletableFuture> resp = mockRetryableErrorThen200Response(multipartClient, i); futures.add(resp); @@ -188,7 +184,7 @@ public void getObject_5xxErrorResponses_shouldNotReuseInitialRequestId() { List responses = capturingInterceptor.getResponses(); - assertEquals(MAX_ATTEMPTS, responses.size(), () -> String.format("Expected exactly %s responses", MAX_ATTEMPTS)); + assertEquals(MAX_ATTEMPTS, responses.size()); String actualFirstRequestId = responses.get(0).firstMatchingHeader("x-amz-request-id").orElse(null); String actualSecondRequestId = responses.get(1).firstMatchingHeader("x-amz-request-id").orElse(null); @@ -222,7 +218,7 @@ public void multipartDownload_200Response_shouldSucceed() { ResponseBytes response = future.join(); byte[] actualBody = response.asByteArray(); - assertArrayEquals(expectedBody, actualBody, "Downloaded body should match expected combined parts"); + assertArrayEquals(expectedBody, actualBody); // Verify that all 3 parts were requested only once verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); @@ -336,7 +332,7 @@ public void multipartDownload_503OnFirstPartAndSecondPart_shouldRetrySuccessfull AsyncResponseTransformer.toBytes()).join(); byte[] actualBody = response.asByteArray(); - assertArrayEquals(expectedBody, actualBody, "Downloaded body should match expected combined parts"); + assertArrayEquals(expectedBody, actualBody); // Verify that part 1 was requested twice (initial 503 + retry) verify(2, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java index 54bff6062ee4..fbe8bc17eca4 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java @@ -33,14 +33,13 @@ import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartDownloadTestUtils.internalErrorBody; import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartDownloadTestUtils.transformersSuppliers; +import com.github.tomakehurst.wiremock.http.Fault; import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; import com.github.tomakehurst.wiremock.junit5.WireMockTest; import com.github.tomakehurst.wiremock.stubbing.Scenario; import java.io.IOException; -import java.io.UncheckedIOException; +import java.net.SocketException; import java.net.URI; -import java.nio.file.Files; -import java.nio.file.Path; import java.time.Duration; import java.util.ArrayList; import java.util.Arrays; @@ -49,15 +48,12 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.TimeUnit; -import java.util.stream.IntStream; import java.util.stream.Stream; -import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Timeout; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; -import org.reactivestreams.Subscriber; import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; import software.amazon.awssdk.core.SplittingTransformerConfiguration; @@ -70,7 +66,6 @@ import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.s3.S3AsyncClient; import software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartDownloadTestUtils; -import software.amazon.awssdk.services.s3.model.GetObjectRequest; import software.amazon.awssdk.services.s3.model.GetObjectResponse; import software.amazon.awssdk.services.s3.model.S3Exception; import software.amazon.awssdk.services.s3.utils.AsyncResponseTransformerTestSupplier; @@ -82,7 +77,6 @@ public class S3MultipartClientGetObjectWiremockTest { private static final String BUCKET = "Example-Bucket"; private static final String KEY = "Key"; private static final int MAX_ATTEMPTS = 3; - private static int fileCounter = 0; private S3AsyncClient multipartClient; private MultipartDownloadTestUtils util; @@ -112,10 +106,6 @@ void happyPath_shouldReceiveAllBodyPartInCorrectOrder(AsyncResponseTransform int partSize) { byte[] expectedBody = util.stubAllParts(BUCKET, KEY, amountOfPartToTest, partSize); AsyncResponseTransformer transformer = supplier.transformer(); - AsyncResponseTransformer.SplitResult split = transformer.split( - SplittingTransformerConfiguration.builder() - .bufferSizeInBytes(1024 * 32L) - .build()); T response = multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), transformer).join(); @@ -124,25 +114,6 @@ void happyPath_shouldReceiveAllBodyPartInCorrectOrder(AsyncResponseTransform util.verifyCorrectAmountOfRequestsMade(amountOfPartToTest); } - @ParameterizedTest - @MethodSource("partSizeAndTransformerParams") - void nonRetryableErrorOnFirstPart_shouldFail(AsyncResponseTransformerTestSupplier supplier, - int amountOfPartToTest, - int partSize) { - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))).willReturn( - aResponse() - .withStatus(400) - .withBody("400test error message"))); - AsyncResponseTransformer transformer = supplier.transformer(); - AsyncResponseTransformer.SplitResult split = transformer.split( - SplittingTransformerConfiguration.builder() - .bufferSizeInBytes(1024 * 32L) - .build()); - - assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), transformer).join()) - .hasMessageContaining("test error message"); - } - @ParameterizedTest @MethodSource("partSizeAndTransformerParams") void nonRetryableErrorOnThirdPart_shouldCompleteExceptionallyOnlyPartsGreaterThanTwo( @@ -160,12 +131,6 @@ void nonRetryableErrorOnThirdPart_shouldCompleteExceptionallyOnlyPartsGreate SplittingTransformerConfiguration.builder() .bufferSizeInBytes(1024 * 32L) .build()); - Subscriber> subscriber = new MultipartDownloaderSubscriber( - multipartClient, - GetObjectRequest.builder() - .bucket(BUCKET) - .key(KEY) - .build()); if (partSize > 1) { assertThatThrownBy(() -> { @@ -179,116 +144,46 @@ void nonRetryableErrorOnThirdPart_shouldCompleteExceptionallyOnlyPartsGreate } @ParameterizedTest - @MethodSource("partSizeAndTransformerParams") - void serverError_retryExhausted_shouldFail(AsyncResponseTransformerTestSupplier supplier, - int amountOfPartToTest, - int partSize) { - util.stubSeverError(1, internalErrorBody(), amountOfPartToTest); + @MethodSource("responseTransformers") + void nonRetryableErrorOnFirstPart_shouldFail(AsyncResponseTransformerTestSupplier supplier) { + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))).willReturn( + aResponse() + .withStatus(400) + .withBody("400test error message"))); AsyncResponseTransformer transformer = supplier.transformer(); - - // Only enable this test for ByteArrayAsyncResponseTransformer because only ByteArrayAsyncResponseTransformer supports - // retry - Assumptions.assumeTrue(transformer instanceof ByteArrayAsyncResponseTransformer); - assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), transformer).join()) - .hasMessageContaining(" We encountered an internal error"); - verify(MAX_ATTEMPTS, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", - BUCKET, KEY)))); + .hasMessageContaining("test error message"); } @ParameterizedTest - @MethodSource("partSizeAndTransformerParams") - void serverError_retrySucceeds_shouldSucceed(AsyncResponseTransformerTestSupplier supplier, - int amountOfPartToTest, - int partSize) { - - byte[] expectedBody = util.stubFirst503Second200AllParts(amountOfPartToTest, partSize); - AsyncResponseTransformer transformer = supplier.transformer(); - - // Only enable this test for ByteArrayAsyncResponseTransformer because only ByteArrayAsyncResponseTransformer supports - // retry - Assumptions.assumeTrue(transformer instanceof ByteArrayAsyncResponseTransformer); - - T response = multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), transformer).join(); - - byte[] body = supplier.body(response); - assertArrayEquals(expectedBody, body); - util.verifyCorrectAmountOfRequestsMade(amountOfPartToTest); - - IntStream.range(1, amountOfPartToTest) - .forEach(index -> verify(2, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber="+ index, - BUCKET, KEY))))); - } - - private static Stream partSizeAndTransformerParams() { - // amount of part, individual part size - List> partSizes = Arrays.asList( - Pair.of(4, 16), - Pair.of(1, 1024), - Pair.of(31, 1243), - Pair.of(16, 16 * 1024), - Pair.of(1, 1024 * 1024), - Pair.of(4, 1024 * 1024), - Pair.of(1, 4 * 1024 * 1024), - Pair.of(4, 6 * 1024 * 1024), - Pair.of(7, 5 * 3752) - ); - - Stream.Builder sb = Stream.builder(); - transformersSuppliers().forEach(tr -> partSizes.forEach(p -> sb.accept(arguments(tr, p.left(), p.right())))); - return sb.build(); - } - - /** - * Testing {@link PublisherAsyncResponseTransformer}, {@link InputStreamResponseTransformer}, and - * {@link FileAsyncResponseTransformer} - *

- * - * Retry for multipart download is supported for {@link ByteArrayAsyncResponseTransformer}, tested in - * {@link S3MultipartClientGetObjectToBytesWiremockTest}. - */ - private static Stream responseTransformerFactories() { - return Stream.of( - AsyncResponseTransformer::toBlockingInputStream, - AsyncResponseTransformer::toPublisher, - () -> { - try { - Path tempDir = Files.createTempDirectory("s3-test"); - Path tempFile = tempDir.resolve("testFile" + fileCounter + ".txt"); - fileCounter++; - tempFile.toFile().deleteOnExit(); - return AsyncResponseTransformer.toFile(tempFile); - } catch (IOException e) { - throw new UncheckedIOException(e); - } - } - ); - } + @MethodSource("responseTransformers") + public void ioError_shouldFailAndNotRetry() { + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))) + .willReturn(aResponse() + .withFault(Fault.CONNECTION_RESET_BY_PEER))); - interface TransformerFactory { - AsyncResponseTransformer create(); - } + assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), + AsyncResponseTransformer.toBlockingInputStream()).join()) + .satisfiesAnyOf( + throwable -> assertThat(throwable) + .hasMessageContaining("The connection was closed during the request"), - @ParameterizedTest - @MethodSource("responseTransformerFactories") - public void ioError_shouldFailAndNotRetry(TransformerFactory transformerFactory) { - util.stubIoError( 1); - AsyncResponseTransformer transformer = transformerFactory.create(); + throwable -> assertThat(throwable) + .hasMessageContaining("Connection reset") + ); - assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), transformer).join()) - .hasCauseInstanceOf(IOException.class); verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); } @ParameterizedTest - @MethodSource("responseTransformerFactories") - public void getObject_single500WithinMany200s_shouldNotRetryError(TransformerFactory transformerFactory) { + @MethodSource("responseTransformers") + public void getObject_single500WithinMany200s_shouldNotRetryError(AsyncResponseTransformerTestSupplier transformerSupplier) { List> futures = new ArrayList<>(); - int numRuns = 100; + int numRuns = 50; for (int i = 0; i < numRuns; i++) { - CompletableFuture resp = mock200Response(multipartClient, i, transformerFactory); + CompletableFuture resp = mock200Response(multipartClient, i, transformerSupplier); futures.add(resp); } @@ -311,11 +206,11 @@ public void getObject_single500WithinMany200s_shouldNotRetryError(TransformerFac .withBody("Hello World"))); CompletableFuture requestWithRetryableError = - multipartClient.getObject(r -> r.bucket(BUCKET).key(errorKey), transformerFactory.create()); + multipartClient.getObject(r -> r.bucket(BUCKET).key(errorKey), transformerSupplier.transformer()); futures.add(requestWithRetryableError); for (int i = 0; i < numRuns; i++) { - CompletableFuture resp = mock200Response(multipartClient, i + 1000, transformerFactory); + CompletableFuture resp = mock200Response(multipartClient, i + 1000, transformerSupplier); futures.add(resp); } @@ -329,7 +224,42 @@ public void getObject_single500WithinMany200s_shouldNotRetryError(TransformerFac verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, errorKey)))); } - private CompletableFuture mock200Response(S3AsyncClient s3Client, int runNumber, TransformerFactory transformerFactory) { + private static Stream partSizeAndTransformerParams() { + // amount of part, individual part size + List> partSizes = Arrays.asList( + Pair.of(4, 16), + Pair.of(1, 1024), + Pair.of(31, 1243), + Pair.of(16, 16 * 1024), + Pair.of(1, 1024 * 1024), + Pair.of(4, 1024 * 1024), + Pair.of(1, 4 * 1024 * 1024), + Pair.of(4, 6 * 1024 * 1024), + Pair.of(7, 5 * 3752) + ); + + Stream.Builder sb = Stream.builder(); + transformersSuppliers().forEach(tr -> partSizes.forEach(p -> sb.accept(arguments(tr, p.left(), p.right())))); + return sb.build(); + } + + + /** + * Testing {@link PublisherAsyncResponseTransformer}, {@link InputStreamResponseTransformer}, and + * {@link FileAsyncResponseTransformer} + *

+ * + * Retry for multipart download is supported for {@link ByteArrayAsyncResponseTransformer}, tested in + * {@link S3MultipartClientGetObjectToBytesWiremockTest}. + */ + private static Stream> responseTransformers() { + return Stream.of(new AsyncResponseTransformerTestSupplier.InputStreamArtSupplier(), + new AsyncResponseTransformerTestSupplier.PublisherArtSupplier(), + new AsyncResponseTransformerTestSupplier.FileArtSupplier()); + } + + private CompletableFuture mock200Response(S3AsyncClient s3Client, int runNumber, + AsyncResponseTransformerTestSupplier transformerSupplier) { String runId = runNumber + " success"; stubFor(any(anyUrl()) @@ -342,6 +272,6 @@ private CompletableFuture mock200Response(S3AsyncClient s3Client, int runNumb return s3Client.getObject(r -> r.bucket(BUCKET).key(KEY) .overrideConfiguration(c -> c.putHeader("RunNum", runId)), - transformerFactory.create()); + transformerSupplier.transformer()); } } diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/utils/MultipartDownloadTestUtils.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/utils/MultipartDownloadTestUtils.java index d4c2ecbf047a..ac667912a33f 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/utils/MultipartDownloadTestUtils.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/utils/MultipartDownloadTestUtils.java @@ -23,19 +23,13 @@ import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching; import static com.github.tomakehurst.wiremock.client.WireMock.verify; -import com.github.tomakehurst.wiremock.http.Fault; -import com.github.tomakehurst.wiremock.stubbing.Scenario; import java.util.Arrays; import java.util.List; import java.util.Random; -import java.util.UUID; import software.amazon.awssdk.services.s3.utils.AsyncResponseTransformerTestSupplier; public class MultipartDownloadTestUtils { - private static final String RETRY_SCENARIO = "retry"; - private static final String SUCCESS_STATE = "success"; - private final String testBucket; private final String testKey; private final String eTag; @@ -65,12 +59,6 @@ public byte[] stubAllParts(String testBucket, String testKey, int amountOfPartTo return expectedBody; } - public void stubIoError(int partNumber) { - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%s", testBucket, testKey, partNumber))) - .willReturn(aResponse() - .withFault(Fault.CONNECTION_RESET_BY_PEER))); - } - public byte[] stubForPart(String testBucket, String testKey,int part, int totalPart, int partSize) { byte[] body = new byte[partSize]; random.nextBytes(body); @@ -82,15 +70,6 @@ public byte[] stubForPart(String testBucket, String testKey,int part, int totalP return body; } - - public void stubSeverError(int partNumber, String errorBody, int totalPart) { - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%d", testBucket, testKey, partNumber))) - .willReturn(aResponse() - .withHeader("x-amz-request-id", String.valueOf(UUID.randomUUID())) - .withHeader("x-amz-mp-parts-count", String.valueOf(totalPart)) - .withStatus(500).withBody(errorBody))); - } - public void verifyCorrectAmountOfRequestsMade(int amountOfPartToTest) { String urlTemplate = ".*partNumber=%d.*"; for (int i = 1; i <= amountOfPartToTest; i++) { @@ -99,53 +78,6 @@ public void verifyCorrectAmountOfRequestsMade(int amountOfPartToTest) { verify(0, getRequestedFor(urlMatching(String.format(urlTemplate, amountOfPartToTest + 1)))); } - public byte[] stubForPartSuccess(int part, int totalPart, int partSize) { - byte[] body = new byte[partSize]; - random.nextBytes(body); - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%d", testBucket, testKey, part))) - .inScenario(RETRY_SCENARIO) - .whenScenarioStateIs(SUCCESS_STATE) - .willReturn( - aResponse() - .withHeader("x-amz-mp-parts-count", totalPart + "") - .withHeader("ETag", eTag) - .withBody(body))); - return body; - } - - public byte[] stubFirst503Second200(int partNumber, int totalPart, int partSize) { - byte[] body = new byte[partSize]; - random.nextBytes(body); - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%s", testBucket, testKey, partNumber))) - .inScenario("part-retry" + partNumber) - .whenScenarioStateIs(Scenario.STARTED) - .willReturn(aResponse() - .withStatus(500) - .withHeader("x-amz-request-id", UUID.randomUUID().toString()) - .withBody(internalErrorBody())) - .willSetStateTo("retry-attempt" + partNumber)); - - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%s", testBucket, testKey, partNumber))) - .inScenario("part-retry" + partNumber) - .whenScenarioStateIs("retry-attempt" + partNumber) - .willReturn(aResponse() - .withStatus(200) - .withHeader("x-amz-mp-parts-count", String.valueOf(totalPart)) - .withHeader("x-amz-request-id", UUID.randomUUID().toString()) - .withBody(body))); - return body; - } - - public byte[] stubFirst503Second200AllParts(int totalPart, int partSize) { - - byte[] expectedBody = new byte[totalPart * partSize]; - for (int i = 0; i < totalPart; i++) { - byte[] individualBody = stubFirst503Second200(i + 1, totalPart, partSize); - System.arraycopy(individualBody, 0, expectedBody, i * partSize, individualBody.length); - } - return expectedBody; - } - public static String errorBody(String errorCode, String errorMessage) { return "\n" + "\n" From 7bae1ed8b8cc1535057f04fb0bf0d4fc12b0bc6d Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 11 Aug 2025 11:02:21 -0700 Subject: [PATCH 07/16] Revert logging enabled --- core/sdk-core/src/test/resources/log4j2.properties | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/sdk-core/src/test/resources/log4j2.properties b/core/sdk-core/src/test/resources/log4j2.properties index 0d89a7c40277..06b1b6bb3d05 100644 --- a/core/sdk-core/src/test/resources/log4j2.properties +++ b/core/sdk-core/src/test/resources/log4j2.properties @@ -25,8 +25,8 @@ rootLogger.appenderRef.stdout.ref = ConsoleAppender # Uncomment below to enable more specific logging # -logger.sdk.name = software.amazon.awssdk.services.s3.internal.multipart -logger.sdk.level = debug +#logger.sdk.name = software.amazon.awssdk.services.s3.internal.multipart +#logger.sdk.level = debug # #logger.request.name = software.amazon.awssdk.request #logger.request.level = debug From 292df0cd3a5fbdf5504d7cfdbbf215d3f6a84355 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 11 Aug 2025 11:02:47 -0700 Subject: [PATCH 08/16] Revert logging enabled --- core/sdk-core/src/test/resources/log4j2.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/sdk-core/src/test/resources/log4j2.properties b/core/sdk-core/src/test/resources/log4j2.properties index 06b1b6bb3d05..99ad57594f6a 100644 --- a/core/sdk-core/src/test/resources/log4j2.properties +++ b/core/sdk-core/src/test/resources/log4j2.properties @@ -25,7 +25,7 @@ rootLogger.appenderRef.stdout.ref = ConsoleAppender # Uncomment below to enable more specific logging # -#logger.sdk.name = software.amazon.awssdk.services.s3.internal.multipart +#logger.sdk.name = software.amazon.awssdk.services #logger.sdk.level = debug # #logger.request.name = software.amazon.awssdk.request From 1d2dc6cf4d054dd648e799e3fa96aad06e6f38c4 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 11 Aug 2025 11:03:10 -0700 Subject: [PATCH 09/16] Revert logging enabled --- core/sdk-core/src/test/resources/log4j2.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/sdk-core/src/test/resources/log4j2.properties b/core/sdk-core/src/test/resources/log4j2.properties index 99ad57594f6a..e5e68dd2faa4 100644 --- a/core/sdk-core/src/test/resources/log4j2.properties +++ b/core/sdk-core/src/test/resources/log4j2.properties @@ -25,7 +25,7 @@ rootLogger.appenderRef.stdout.ref = ConsoleAppender # Uncomment below to enable more specific logging # -#logger.sdk.name = software.amazon.awssdk.services +#logger.sdk.name = software.amazon.awssdk #logger.sdk.level = debug # #logger.request.name = software.amazon.awssdk.request From 523b2bd639c5108e1457cc8ea15e7154bddf86f0 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 11 Aug 2025 11:07:39 -0700 Subject: [PATCH 10/16] Add changelog --- .changes/next-release/feature-AmazonS3-7c1530f.json | 6 ++++++ .../multipart/S3MultipartClientGetObjectWiremockTest.java | 2 -- 2 files changed, 6 insertions(+), 2 deletions(-) create mode 100644 .changes/next-release/feature-AmazonS3-7c1530f.json diff --git a/.changes/next-release/feature-AmazonS3-7c1530f.json b/.changes/next-release/feature-AmazonS3-7c1530f.json new file mode 100644 index 000000000000..0d6ff52b5325 --- /dev/null +++ b/.changes/next-release/feature-AmazonS3-7c1530f.json @@ -0,0 +1,6 @@ +{ + "type": "feature", + "category": "Amazon S3", + "contributor": "", + "description": "Add retry support for Java based S3 multipart client download to Byte array" +} diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java index fbe8bc17eca4..fb93ab8ec0ce 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java @@ -37,8 +37,6 @@ import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; import com.github.tomakehurst.wiremock.junit5.WireMockTest; import com.github.tomakehurst.wiremock.stubbing.Scenario; -import java.io.IOException; -import java.net.SocketException; import java.net.URI; import java.time.Duration; import java.util.ArrayList; From 76281d61a3a6f67da0645803a2c34a70dca89e7c Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 11 Aug 2025 12:00:23 -0700 Subject: [PATCH 11/16] Update javadocs --- .../s3/multipart/MultipartConfiguration.java | 34 ++++++++++++++----- .../codegen-resources/customization.config | 4 +-- 2 files changed, 28 insertions(+), 10 deletions(-) diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java index 9b074d6244f3..bb96fa9bcb23 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java @@ -18,6 +18,7 @@ import software.amazon.awssdk.annotations.SdkPublicApi; import software.amazon.awssdk.core.async.AsyncRequestBody; import software.amazon.awssdk.core.async.AsyncResponseTransformer; +import software.amazon.awssdk.core.internal.async.ByteArrayAsyncResponseTransformer; import software.amazon.awssdk.services.s3.S3AsyncClient; import software.amazon.awssdk.services.s3.S3AsyncClientBuilder; import software.amazon.awssdk.services.s3.model.CopyObjectRequest; @@ -28,15 +29,21 @@ import software.amazon.awssdk.utils.builder.ToCopyableBuilder; /** - * Class that hold configuration properties related to multipart operation for a {@link S3AsyncClient}. Passing this class to the - * {@link S3AsyncClientBuilder#multipartConfiguration(MultipartConfiguration)} will enable automatic conversion of - * {@link S3AsyncClient#getObject(GetObjectRequest, AsyncResponseTransformer)}, - * {@link S3AsyncClient#putObject(PutObjectRequest, AsyncRequestBody)} and - * {@link S3AsyncClient#copyObject(CopyObjectRequest)} to their respective multipart operation. + * Class that holds configuration properties related to multipart operations for a {@link S3AsyncClient}. Passing this class to + * the {@link S3AsyncClientBuilder#multipartConfiguration(MultipartConfiguration)} will enable automatic conversion of the + * following operations to their respective multipart variants: + *

    + *
  • {@link S3AsyncClient#getObject(GetObjectRequest, AsyncResponseTransformer)}, + *
  • {@link S3AsyncClient#putObject(PutObjectRequest, AsyncRequestBody)} + *
  • {@link S3AsyncClient#copyObject(CopyObjectRequest)} + *
*

- * Note that multipart download fetch individual part of the object using {@link GetObjectRequest#partNumber() part number}, this - * means it will only download multiple parts if the - * object itself was uploaded as a {@link S3AsyncClient#createMultipartUpload(CreateMultipartUploadRequest) multipart object} + * Note that multipart download fetches individual parts of the object using {@link GetObjectRequest#partNumber() PartNumber}. + * This means the S3 client will only download multiple parts if the object itself was uploaded as a + * {@link S3AsyncClient#createMultipartUpload(CreateMultipartUploadRequest) multipart object} + *

+ * When performing multipart download, retry is only supported for downloading to byte array, i.e., when providing a + * {@link ByteArrayAsyncResponseTransformer} */ @SdkPublicApi public final class MultipartConfiguration implements ToCopyableBuilder { @@ -83,6 +90,10 @@ public Long minimumPartSizeInBytes() { /** * The maximum memory, in bytes, that the SDK will use to buffer requests content into memory. + *

+ * This setting is not supported and will be ignored when downloading to a byte array, i.e., when providing a + * {@link ByteArrayAsyncResponseTransformer}. + * * @return the value of the configured maximum memory usage. */ public Long apiCallBufferSizeInBytes() { @@ -152,6 +163,9 @@ public interface Builder extends CopyableBuilder * Default value: If not specified, the SDK will use the equivalent of four parts worth of memory, so 32 Mib by default. + *

+ * This setting is not supported and will be ignored when downloading to a byte array, i.e., when providing a + * {@link ByteArrayAsyncResponseTransformer}. * * @param apiCallBufferSizeInBytes the value of the maximum memory usage. * @return an instance of this builder. @@ -170,20 +184,24 @@ private static class DefaultMultipartConfigBuilder implements Builder { private Long minimumPartSizeInBytes; private Long apiCallBufferSizeInBytes; + @Override public Builder thresholdInBytes(Long thresholdInBytes) { this.thresholdInBytes = thresholdInBytes; return this; } + @Override public Long thresholdInBytes() { return this.thresholdInBytes; } + @Override public Builder minimumPartSizeInBytes(Long minimumPartSizeInBytes) { this.minimumPartSizeInBytes = minimumPartSizeInBytes; return this; } + @Override public Long minimumPartSizeInBytes() { return this.minimumPartSizeInBytes; } diff --git a/services/s3/src/main/resources/codegen-resources/customization.config b/services/s3/src/main/resources/codegen-resources/customization.config index 8acd43509eab..44484c5ff0c2 100644 --- a/services/s3/src/main/resources/codegen-resources/customization.config +++ b/services/s3/src/main/resources/codegen-resources/customization.config @@ -289,8 +289,8 @@ "useS3ExpressSessionAuth": true, "multipartCustomization": { "multipartConfigurationClass": "software.amazon.awssdk.services.s3.multipart.MultipartConfiguration", - "multipartConfigMethodDoc": "Configuration for multipart operation of this client.", - "multipartEnableMethodDoc": "Enables automatic conversion of GET, PUT and COPY methods to their equivalent multipart operation. CRC32 checksum will be enabled for PUT, unless the checksum is specified or checksum validation is disabled.", + "multipartConfigMethodDoc": "Configuration for multipart operation of this client.

When performing multipart download, retry is only supported for downloading to byte array, i.e., when providing a {@code ByteArrayAsyncResponseTransformer}", + "multipartEnableMethodDoc": "Enables automatic conversion of GET, PUT and COPY methods to their equivalent multipart operation. CRC32 checksum will be enabled for PUT, unless the checksum is specified or checksum validation is disabled.

When performing multipart download, retry is only supported for downloading to byte array, i.e., when providing a {@code ByteArrayAsyncResponseTransformer}", "contextParamEnabledKey": "S3AsyncClientDecorator.MULTIPART_ENABLED_KEY", "contextParamConfigKey": "S3AsyncClientDecorator.MULTIPART_CONFIGURATION_KEY" }, From 1d136e52f5c76b452906c062f9d7e1056b6b4628 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 11 Aug 2025 12:59:56 -0700 Subject: [PATCH 12/16] Use maximumBufferSize in ByteArraySplittingTransformer --- .../async/ByteArrayAsyncResponseTransformer.java | 12 +++++++++++- .../async/ByteArraySplittingTransformer.java | 14 ++++++++++++-- .../s3/multipart/MultipartConfiguration.java | 7 ------- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java index 79838f739d4a..e76833dba0db 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java @@ -28,6 +28,8 @@ import software.amazon.awssdk.core.async.AsyncResponseTransformer; import software.amazon.awssdk.core.async.SdkPublisher; import software.amazon.awssdk.utils.BinaryUtils; +import software.amazon.awssdk.utils.Validate; +import software.amazon.awssdk.utils.async.DelegatingBufferingSubscriber; /** * Implementation of {@link AsyncResponseTransformer} that dumps content into a byte array and supports further @@ -61,6 +63,13 @@ public void onStream(SdkPublisher publisher) { publisher.subscribe(new BaosSubscriber(cf)); } + public void onStream(SdkPublisher publisher, long maximumBufferSize) { + publisher.subscribe(DelegatingBufferingSubscriber.builder() + .maximumBufferInBytes(maximumBufferSize) + .delegate(new BaosSubscriber(cf)) + .build()); + } + @Override public void exceptionOccurred(Throwable throwable) { cf.completeExceptionally(throwable); @@ -68,9 +77,10 @@ public void exceptionOccurred(Throwable throwable) { @Override public SplitResult> split(SplittingTransformerConfiguration splitConfig) { + Validate.notNull(splitConfig, "splitConfig must not be null"); CompletableFuture> future = new CompletableFuture<>(); SdkPublisher> transformer = - new ByteArraySplittingTransformer<>(this, future); + new ByteArraySplittingTransformer<>(this, future, splitConfig.bufferSizeInBytes()); return AsyncResponseTransformer.SplitResult.>builder() .publisher(transformer) .resultFuture(future) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java index 7d3b3a249067..576626edd5b2 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java @@ -34,6 +34,7 @@ import software.amazon.awssdk.core.exception.SdkClientException; import software.amazon.awssdk.utils.CompletableFutureUtils; import software.amazon.awssdk.utils.Logger; +import software.amazon.awssdk.utils.Validate; import software.amazon.awssdk.utils.async.SimplePublisher; @SdkInternalApi @@ -73,9 +74,18 @@ public class ByteArraySplittingTransformer implements SdkPublisher buffers; + /** + * The buffer size used to buffer the content received from the downstream subscriber + */ + private final long maximumBufferInBytes; + public ByteArraySplittingTransformer(AsyncResponseTransformer> upstreamResponseTransformer, - CompletableFuture> resultFuture) { + CompletableFuture> resultFuture, + Long maximumBufferSizeInBytes) { + Validate.notNull(maximumBufferSizeInBytes, "maximumBufferSizeInBytes"); + this.maximumBufferInBytes = Validate.isPositive( + maximumBufferSizeInBytes, "maximumBufferSizeInBytes"); this.upstreamResponseTransformer = upstreamResponseTransformer; this.resultFuture = resultFuture; this.buffers = new ConcurrentHashMap<>(); @@ -223,7 +233,7 @@ public void onResponse(ResponseT response) { @Override public void onStream(SdkPublisher publisher) { - delegate.onStream(publisher); + delegate.onStream(publisher, maximumBufferInBytes); synchronized (lock) { if (!onStreamCalled) { onStreamCalled = true; diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java index bb96fa9bcb23..8631403b97f8 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java @@ -90,10 +90,6 @@ public Long minimumPartSizeInBytes() { /** * The maximum memory, in bytes, that the SDK will use to buffer requests content into memory. - *

- * This setting is not supported and will be ignored when downloading to a byte array, i.e., when providing a - * {@link ByteArrayAsyncResponseTransformer}. - * * @return the value of the configured maximum memory usage. */ public Long apiCallBufferSizeInBytes() { @@ -163,9 +159,6 @@ public interface Builder extends CopyableBuilder * Default value: If not specified, the SDK will use the equivalent of four parts worth of memory, so 32 Mib by default. - *

- * This setting is not supported and will be ignored when downloading to a byte array, i.e., when providing a - * {@link ByteArrayAsyncResponseTransformer}. * * @param apiCallBufferSizeInBytes the value of the maximum memory usage. * @return an instance of this builder. From c381f8a1ca974c9babb3d18c04dbdebac600d575 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Mon, 11 Aug 2025 18:59:23 -0700 Subject: [PATCH 13/16] Revert "Use maximumBufferSize in ByteArraySplittingTransformer" This reverts commit 1d136e52f5c76b452906c062f9d7e1056b6b4628. --- .../async/ByteArrayAsyncResponseTransformer.java | 12 +----------- .../async/ByteArraySplittingTransformer.java | 14 ++------------ .../s3/multipart/MultipartConfiguration.java | 7 +++++++ 3 files changed, 10 insertions(+), 23 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java index e76833dba0db..79838f739d4a 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArrayAsyncResponseTransformer.java @@ -28,8 +28,6 @@ import software.amazon.awssdk.core.async.AsyncResponseTransformer; import software.amazon.awssdk.core.async.SdkPublisher; import software.amazon.awssdk.utils.BinaryUtils; -import software.amazon.awssdk.utils.Validate; -import software.amazon.awssdk.utils.async.DelegatingBufferingSubscriber; /** * Implementation of {@link AsyncResponseTransformer} that dumps content into a byte array and supports further @@ -63,13 +61,6 @@ public void onStream(SdkPublisher publisher) { publisher.subscribe(new BaosSubscriber(cf)); } - public void onStream(SdkPublisher publisher, long maximumBufferSize) { - publisher.subscribe(DelegatingBufferingSubscriber.builder() - .maximumBufferInBytes(maximumBufferSize) - .delegate(new BaosSubscriber(cf)) - .build()); - } - @Override public void exceptionOccurred(Throwable throwable) { cf.completeExceptionally(throwable); @@ -77,10 +68,9 @@ public void exceptionOccurred(Throwable throwable) { @Override public SplitResult> split(SplittingTransformerConfiguration splitConfig) { - Validate.notNull(splitConfig, "splitConfig must not be null"); CompletableFuture> future = new CompletableFuture<>(); SdkPublisher> transformer = - new ByteArraySplittingTransformer<>(this, future, splitConfig.bufferSizeInBytes()); + new ByteArraySplittingTransformer<>(this, future); return AsyncResponseTransformer.SplitResult.>builder() .publisher(transformer) .resultFuture(future) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java index 576626edd5b2..7d3b3a249067 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java @@ -34,7 +34,6 @@ import software.amazon.awssdk.core.exception.SdkClientException; import software.amazon.awssdk.utils.CompletableFutureUtils; import software.amazon.awssdk.utils.Logger; -import software.amazon.awssdk.utils.Validate; import software.amazon.awssdk.utils.async.SimplePublisher; @SdkInternalApi @@ -74,18 +73,9 @@ public class ByteArraySplittingTransformer implements SdkPublisher buffers; - /** - * The buffer size used to buffer the content received from the downstream subscriber - */ - private final long maximumBufferInBytes; - public ByteArraySplittingTransformer(AsyncResponseTransformer> upstreamResponseTransformer, - CompletableFuture> resultFuture, - Long maximumBufferSizeInBytes) { - Validate.notNull(maximumBufferSizeInBytes, "maximumBufferSizeInBytes"); - this.maximumBufferInBytes = Validate.isPositive( - maximumBufferSizeInBytes, "maximumBufferSizeInBytes"); + CompletableFuture> resultFuture) { this.upstreamResponseTransformer = upstreamResponseTransformer; this.resultFuture = resultFuture; this.buffers = new ConcurrentHashMap<>(); @@ -233,7 +223,7 @@ public void onResponse(ResponseT response) { @Override public void onStream(SdkPublisher publisher) { - delegate.onStream(publisher, maximumBufferInBytes); + delegate.onStream(publisher); synchronized (lock) { if (!onStreamCalled) { onStreamCalled = true; diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java index 8631403b97f8..bb96fa9bcb23 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java @@ -90,6 +90,10 @@ public Long minimumPartSizeInBytes() { /** * The maximum memory, in bytes, that the SDK will use to buffer requests content into memory. + *

+ * This setting is not supported and will be ignored when downloading to a byte array, i.e., when providing a + * {@link ByteArrayAsyncResponseTransformer}. + * * @return the value of the configured maximum memory usage. */ public Long apiCallBufferSizeInBytes() { @@ -159,6 +163,9 @@ public interface Builder extends CopyableBuilder * Default value: If not specified, the SDK will use the equivalent of four parts worth of memory, so 32 Mib by default. + *

+ * This setting is not supported and will be ignored when downloading to a byte array, i.e., when providing a + * {@link ByteArrayAsyncResponseTransformer}. * * @param apiCallBufferSizeInBytes the value of the maximum memory usage. * @return an instance of this builder. From 4a927cb3d9b65ce25b77651a4029d88b49f9b819 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Wed, 13 Aug 2025 11:24:42 -0700 Subject: [PATCH 14/16] Address comments and refactor tests --- .../async/ByteArraySplittingTransformer.java | 24 +-- .../ByteArraySplittingTransformerTckTest.java | 49 ++++++ .../s3/multipart/MultipartConfiguration.java | 8 +- .../codegen-resources/customization.config | 2 +- ...ntGetObjectRetryBehaviorWiremockTest.java} | 166 +++--------------- ...3MultipartClientGetObjectWiremockTest.java | 12 +- 6 files changed, 88 insertions(+), 173 deletions(-) create mode 100644 core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformerTckTest.java rename services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/{S3MultipartClientGetObjectToBytesWiremockTest.java => S3MultipartClientGetObjectRetryBehaviorWiremockTest.java} (72%) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java index 7d3b3a249067..845e84fa8ae4 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java @@ -16,8 +16,6 @@ package software.amazon.awssdk.core.internal.async; import java.nio.ByteBuffer; -import java.util.ArrayList; -import java.util.List; import java.util.Map; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentHashMap; @@ -42,7 +40,7 @@ public class ByteArraySplittingTransformer implements SdkPublisher> upstreamResponseTransformer; private final CompletableFuture> resultFuture; private Subscriber> downstreamSubscriber; - private final AtomicInteger onNextSignalsSent = new AtomicInteger(0); + private final AtomicInteger nextPartNumber = new AtomicInteger(1); private final AtomicReference responseT = new AtomicReference<>(); private final SimplePublisher publisherToUpstream = new SimplePublisher<>(); @@ -138,7 +136,7 @@ private boolean doEmit() { } if (outstandingDemand.get() > 0) { demand = outstandingDemand.decrementAndGet(); - downstreamSubscriber.onNext(new IndividualTransformer(onNextSignalsSent.incrementAndGet())); + downstreamSubscriber.onNext(new IndividualTransformer(nextPartNumber.getAndIncrement())); } } return false; @@ -193,25 +191,19 @@ private void handleSubscriptionCancel() { } private final class IndividualTransformer implements AsyncResponseTransformer { - private final int onNextCount; + private final int partNumber; private final ByteArrayAsyncResponseTransformer delegate = new ByteArrayAsyncResponseTransformer<>(); - private CompletableFuture future; - private final List>> delegatePrepareFutures = new ArrayList<>(); - - private IndividualTransformer(int onNextCount) { - this.onNextCount = onNextCount; + private IndividualTransformer(int partNumber) { + this.partNumber = partNumber; } @Override public CompletableFuture prepare() { - future = new CompletableFuture<>(); CompletableFuture> prepare = delegate.prepare(); - CompletableFutureUtils.forwardExceptionTo(prepare, future); - delegatePrepareFutures.add(prepare); - return prepare.thenApply(responseTResponseBytes -> { - buffers.put(onNextCount, responseTResponseBytes.asByteBuffer()); - return responseTResponseBytes.response(); + return prepare.thenApply(responseBytes -> { + buffers.put(partNumber, responseBytes.asByteBuffer()); + return responseBytes.response(); }); } diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformerTckTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformerTckTest.java new file mode 100644 index 000000000000..8c90c899abb3 --- /dev/null +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformerTckTest.java @@ -0,0 +1,49 @@ +/* + * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"). + * You may not use this file except in compliance with the License. + * A copy of the License is located at + * + * http://aws.amazon.com/apache2.0 + * + * or in the "license" file accompanying this file. This file is distributed + * on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either + * express or implied. See the License for the specific language governing + * permissions and limitations under the License. + */ + +package software.amazon.awssdk.core.internal.async; + +import java.util.concurrent.CompletableFuture; +import org.reactivestreams.Publisher; +import org.reactivestreams.tck.PublisherVerification; +import org.reactivestreams.tck.TestEnvironment; +import software.amazon.awssdk.core.ResponseBytes; +import software.amazon.awssdk.core.async.AsyncResponseTransformer; +import software.amazon.awssdk.core.async.SdkPublisher; + +public class ByteArraySplittingTransformerTckTest extends PublisherVerification> { + + public ByteArraySplittingTransformerTckTest() { + super(new TestEnvironment()); + } + + @Override + public Publisher> createPublisher(long l) { + CompletableFuture> future = new CompletableFuture<>(); + AsyncResponseTransformer> upstreamTransformer = AsyncResponseTransformer.toBytes(); + ByteArraySplittingTransformer transformer = new ByteArraySplittingTransformer<>(upstreamTransformer, future); + return SdkPublisher.adapt(transformer).limit(Math.toIntExact(l)); + } + + @Override + public Publisher> createFailedPublisher() { + return null; + } + + @Override + public long maxElementsFromPublisher() { + return Long.MAX_VALUE; + } +} diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java index bb96fa9bcb23..d5d47f0e50f5 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java @@ -42,8 +42,8 @@ * This means the S3 client will only download multiple parts if the object itself was uploaded as a * {@link S3AsyncClient#createMultipartUpload(CreateMultipartUploadRequest) multipart object} *

- * When performing multipart download, retry is only supported for downloading to byte array, i.e., when providing a - * {@link ByteArrayAsyncResponseTransformer} + * When performing multipart download, retry is only supported when using an {@link AsyncResponseTransformer} implementation + * that downloads the object into memory such, as {@link AsyncResponseTransformer#toBytes()} */ @SdkPublicApi public final class MultipartConfiguration implements ToCopyableBuilder { @@ -91,8 +91,8 @@ public Long minimumPartSizeInBytes() { /** * The maximum memory, in bytes, that the SDK will use to buffer requests content into memory. *

- * This setting is not supported and will be ignored when downloading to a byte array, i.e., when providing a - * {@link ByteArrayAsyncResponseTransformer}. + * This setting does not apply if you are using an {@link AsyncResponseTransformer} implementation that downloads the + * object into memory such as {@link AsyncResponseTransformer#toBytes} * * @return the value of the configured maximum memory usage. */ diff --git a/services/s3/src/main/resources/codegen-resources/customization.config b/services/s3/src/main/resources/codegen-resources/customization.config index 44484c5ff0c2..4dee2d9e88d9 100644 --- a/services/s3/src/main/resources/codegen-resources/customization.config +++ b/services/s3/src/main/resources/codegen-resources/customization.config @@ -289,7 +289,7 @@ "useS3ExpressSessionAuth": true, "multipartCustomization": { "multipartConfigurationClass": "software.amazon.awssdk.services.s3.multipart.MultipartConfiguration", - "multipartConfigMethodDoc": "Configuration for multipart operation of this client.

When performing multipart download, retry is only supported for downloading to byte array, i.e., when providing a {@code ByteArrayAsyncResponseTransformer}", + "multipartConfigMethodDoc": "Configuration for multipart operation of this client.

When performing multipart download, retry is only supported when using an {@code AsyncResponseTransformer} implementation that downloads the object into memory, such as {@code AsyncResponseTransformer#toBytes()}", "multipartEnableMethodDoc": "Enables automatic conversion of GET, PUT and COPY methods to their equivalent multipart operation. CRC32 checksum will be enabled for PUT, unless the checksum is specified or checksum validation is disabled.

When performing multipart download, retry is only supported for downloading to byte array, i.e., when providing a {@code ByteArrayAsyncResponseTransformer}", "contextParamEnabledKey": "S3AsyncClientDecorator.MULTIPART_ENABLED_KEY", "contextParamConfigKey": "S3AsyncClientDecorator.MULTIPART_CONFIGURATION_KEY" diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectRetryBehaviorWiremockTest.java similarity index 72% rename from services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java rename to services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectRetryBehaviorWiremockTest.java index 2a29d385999c..a5e2d03363fc 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectToBytesWiremockTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectRetryBehaviorWiremockTest.java @@ -26,9 +26,6 @@ import static com.github.tomakehurst.wiremock.client.WireMock.verify; import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.junit.jupiter.api.Assertions.assertArrayEquals; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartDownloadTestUtils.internalErrorBody; import static software.amazon.awssdk.services.s3.internal.multipart.utils.MultipartDownloadTestUtils.slowdownErrorBody; @@ -55,10 +52,6 @@ import software.amazon.awssdk.core.ResponseBytes; import software.amazon.awssdk.core.async.AsyncResponseTransformer; import software.amazon.awssdk.core.exception.SdkClientException; -import software.amazon.awssdk.core.interceptor.Context; -import software.amazon.awssdk.core.interceptor.ExecutionAttributes; -import software.amazon.awssdk.core.interceptor.ExecutionInterceptor; -import software.amazon.awssdk.http.SdkHttpResponse; import software.amazon.awssdk.http.nio.netty.NettyNioAsyncHttpClient; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.s3.S3AsyncClient; @@ -68,8 +61,7 @@ @WireMockTest @Timeout(value = 30, unit = TimeUnit.SECONDS) -public class S3MultipartClientGetObjectToBytesWiremockTest { - private static final CapturingInterceptor capturingInterceptor = new CapturingInterceptor(); +public class S3MultipartClientGetObjectRetryBehaviorWiremockTest { private static final String BUCKET = "Example-Bucket"; private static final String KEY = "Key"; private static final int MAX_ATTEMPTS = 7; @@ -95,7 +87,6 @@ public static void init() { @BeforeEach public void setup(WireMockRuntimeInfo wm) { - capturingInterceptor.clear(); multipartClient = S3AsyncClient.builder() .region(Region.US_EAST_1) .endpointOverride(URI.create(wm.getHttpBaseUrl())) @@ -103,24 +94,10 @@ public void setup(WireMockRuntimeInfo wm) { .httpClientBuilder(NettyNioAsyncHttpClient.builder().maxConcurrency(100).connectionAcquisitionTimeout(Duration.ofSeconds(100))) .credentialsProvider(StaticCredentialsProvider.create(AwsBasicCredentials.create("key", "secret"))) .overrideConfiguration( - o -> o.retryStrategy(b -> b.maxAttempts(MAX_ATTEMPTS)) - .addExecutionInterceptor(capturingInterceptor)) + o -> o.retryStrategy(b -> b.maxAttempts(MAX_ATTEMPTS))) .build(); } - @Test - public void getObject_concurrentCallsReturn200_shouldSucceed() { - List>> futures = new ArrayList<>(); - - int numRuns = 1000; - for (int i = 0; i < numRuns; i++) { - CompletableFuture> resp = mock200Response(multipartClient, i); - futures.add(resp); - } - - CompletableFuture.allOf(futures.toArray(new CompletableFuture[0])).join(); - } - @Test public void getObject_single500WithinMany200s_shouldRetrySuccessfully() { List>> futures = new ArrayList<>(); @@ -157,17 +134,19 @@ public void getObject_concurrent503s_shouldRetrySuccessfully() { } @Test - public void getObject_5xxErrorResponses_shouldNotReuseInitialRequestId() { + public void getObject_5xxErrorResponses_shouldNotReuseInitialErrorResponseWhenLogging() { String firstRequestId = UUID.randomUUID().toString(); String secondRequestId = UUID.randomUUID().toString(); + int firstErrorStatusCode = 503; + int secondErrorStatusCode = 500; stubFor(any(anyUrl()) .inScenario("errors") .whenScenarioStateIs(Scenario.STARTED) .willReturn(aResponse() .withHeader("x-amz-request-id", firstRequestId) - .withStatus(503) - .withBody(internalErrorBody())) + .withStatus(firstErrorStatusCode) + .withBody(slowdownErrorBody())) .willSetStateTo("SecondAttempt")); stubFor(any(anyUrl()) @@ -175,59 +154,25 @@ public void getObject_5xxErrorResponses_shouldNotReuseInitialRequestId() { .whenScenarioStateIs("SecondAttempt") .willReturn(aResponse() .withHeader("x-amz-request-id", secondRequestId) - .withStatus(500))); + .withStatus(secondErrorStatusCode) + .withBody(internalErrorBody()))); assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes()).join()) .isInstanceOf(CompletionException.class) - .hasCauseInstanceOf(S3Exception.class); - - - List responses = capturingInterceptor.getResponses(); - assertEquals(MAX_ATTEMPTS, responses.size()); - - String actualFirstRequestId = responses.get(0).firstMatchingHeader("x-amz-request-id").orElse(null); - String actualSecondRequestId = responses.get(1).firstMatchingHeader("x-amz-request-id").orElse(null); - - assertNotNull(actualFirstRequestId); - assertNotNull(actualSecondRequestId); - - assertNotEquals(actualFirstRequestId, actualSecondRequestId); - - assertEquals(firstRequestId, actualFirstRequestId); - assertEquals(secondRequestId, actualSecondRequestId); - - assertEquals(503, responses.get(0).statusCode()); - assertEquals(500, responses.get(1).statusCode()); + .hasCauseInstanceOf(S3Exception.class) + .hasMessageNotContaining(firstRequestId) + .hasMessageNotContaining(String.valueOf(firstErrorStatusCode)) + .hasMessageContaining(secondRequestId) + .hasMessageContaining(String.valueOf(secondErrorStatusCode)); verify(MAX_ATTEMPTS, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); verify(0, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY)))); verify(0, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); } - @Test - public void multipartDownload_200Response_shouldSucceed() { - stub200SuccessPart1(); - stub200SuccessPart2(); - stub200SuccessPart3(); - - CompletableFuture> future = - multipartClient.getObject(GetObjectRequest.builder().bucket(BUCKET).key(KEY).build(), - AsyncResponseTransformer.toBytes()); - - ResponseBytes response = future.join(); - byte[] actualBody = response.asByteArray(); - assertArrayEquals(expectedBody, actualBody); - - // Verify that all 3 parts were requested only once - verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); - verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY)))); - verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); - } - - @Test - public void multipartDownload_secondPartNonRetryableError_shouldFail() { + public void multipartDownload_secondPart500ResponseOnly_shouldExhaustRetriesAndFail() { stub200SuccessPart1(); stubError(2, internalErrorBody()); @@ -243,49 +188,6 @@ public void multipartDownload_secondPartNonRetryableError_shouldFail() { verify(0, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); } - @Test - public void multipartDownload_503OnFirstPart_shouldRetrySuccessfully() { - // Stub Part 1 - 503 on first attempt, 200 on retry - String part1Scenario = "part1-retry"; - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))) - .inScenario(part1Scenario) - .whenScenarioStateIs(Scenario.STARTED) - .willReturn(aResponse() - .withStatus(503) - .withHeader("x-amz-request-id", UUID.randomUUID().toString()) - .withBody("\n" + - "\n" + - " SlowDown\n" + - " Please reduce your request rate.\n" + - "")) - .willSetStateTo("retry-attempt")); - - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))) - .inScenario(part1Scenario) - .whenScenarioStateIs("retry-attempt") - .willReturn(aResponse() - .withStatus(200) - .withHeader("x-amz-mp-parts-count", String.valueOf(TOTAL_PARTS)) - .withHeader("x-amz-request-id", UUID.randomUUID().toString()) - .withBody(PART_1_DATA))); - - stub200SuccessPart2(); - stub200SuccessPart3(); - - CompletableFuture> future = - multipartClient.getObject(GetObjectRequest.builder().bucket(BUCKET).key(KEY).build(), - AsyncResponseTransformer.toBytes()); - - ResponseBytes response = future.join(); - byte[] actualBody = response.asByteArray(); - assertArrayEquals(expectedBody, actualBody, "Downloaded body should match expected combined parts"); - - // Verify that part 1 was requested twice (initial 503 + retry) - verify(2, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); - verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY)))); - verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); - } - @Test public void multipartDownload_503OnFirstPartAndSecondPart_shouldRetrySuccessfully() { // Stub Part 1 - 503 on first attempt, 200 on retry @@ -329,20 +231,19 @@ public void multipartDownload_503OnFirstPartAndSecondPart_shouldRetrySuccessfull stub200SuccessPart3(); ResponseBytes response = multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), - AsyncResponseTransformer.toBytes()).join(); + AsyncResponseTransformer.toBytes()).join(); byte[] actualBody = response.asByteArray(); assertArrayEquals(expectedBody, actualBody); - // Verify that part 1 was requested twice (initial 503 + retry) + // Verify that part 1 and 2 were requested twice (initial 503 + retry) verify(2, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY)))); - // Verify that part 2 was requested once (no retry) verify(2, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=2", BUCKET, KEY)))); verify(1, getRequestedFor(urlEqualTo(String.format("/%s/%s?partNumber=3", BUCKET, KEY)))); } @Test - public void getObject_ioExceptionOnly_shouldExhaustRetriesAndFail() { + public void multipartDownload_ioExceptionOnly_shouldExhaustRetriesAndFail() { stubIoError(1); stub200SuccessPart2(); stub200SuccessPart3(); @@ -358,13 +259,10 @@ public void getObject_ioExceptionOnly_shouldExhaustRetriesAndFail() { @Test public void getObject_iOErrorThen200Response_shouldRetrySuccessfully() { - String requestId = UUID.randomUUID().toString(); - stubFor(any(anyUrl()) .inScenario("io-error") .whenScenarioStateIs(Scenario.STARTED) - .willReturn(aResponse() - .withFault(Fault.CONNECTION_RESET_BY_PEER)) + .willReturn(aResponse().withFault(Fault.CONNECTION_RESET_BY_PEER)) .willSetStateTo("retry")); stubFor(any(anyUrl()) @@ -372,7 +270,7 @@ public void getObject_iOErrorThen200Response_shouldRetrySuccessfully() { .whenScenarioStateIs("retry") .willReturn(aResponse() .withStatus(200) - .withHeader("x-amz-request-id", requestId) + .withHeader("x-amz-request-id", UUID.randomUUID().toString()) .withBody("Hello World"))); ResponseBytes response = multipartClient.getObject(GetObjectRequest.builder() @@ -384,13 +282,6 @@ public void getObject_iOErrorThen200Response_shouldRetrySuccessfully() { assertArrayEquals("Hello World".getBytes(StandardCharsets.UTF_8), response.asByteArray()); verify(2, getRequestedFor(urlEqualTo("/" + BUCKET + "/" + KEY + "?partNumber=1"))); - - List responses = capturingInterceptor.getResponses(); - String finalRequestId = responses.get(responses.size() - 1) - .firstMatchingHeader("x-amz-request-id") - .orElse(null); - - assertEquals(requestId, finalRequestId); } private void stubError(int partNumber, String errorBody) { @@ -475,21 +366,4 @@ private CompletableFuture> mockRetryableErrorTh .overrideConfiguration(c -> c.putHeader("RunNum", runId)), AsyncResponseTransformer.toBytes()); } - - static class CapturingInterceptor implements ExecutionInterceptor { - private final List responses = new ArrayList<>(); - - @Override - public void afterTransmission(Context.AfterTransmission context, ExecutionAttributes executionAttributes) { - responses.add(context.httpResponse()); - } - - public List getResponses() { - return new ArrayList<>(responses); - } - - public void clear() { - responses.clear(); - } - } } diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java index fb93ab8ec0ce..5bcf778f93ff 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java @@ -99,7 +99,7 @@ public void setup(WireMockRuntimeInfo wm) @ParameterizedTest @MethodSource("partSizeAndTransformerParams") - void happyPath_shouldReceiveAllBodyPartInCorrectOrder(AsyncResponseTransformerTestSupplier supplier, + public void happyPath_shouldReceiveAllBodyPartInCorrectOrder(AsyncResponseTransformerTestSupplier supplier, int amountOfPartToTest, int partSize) { byte[] expectedBody = util.stubAllParts(BUCKET, KEY, amountOfPartToTest, partSize); @@ -114,7 +114,7 @@ void happyPath_shouldReceiveAllBodyPartInCorrectOrder(AsyncResponseTransform @ParameterizedTest @MethodSource("partSizeAndTransformerParams") - void nonRetryableErrorOnThirdPart_shouldCompleteExceptionallyOnlyPartsGreaterThanTwo( + public void errorOnThirdPart_shouldCompleteExceptionallyOnlyPartsGreaterThanTwo( AsyncResponseTransformerTestSupplier supplier, int amountOfPartToTest, int partSize) { @@ -143,7 +143,7 @@ void nonRetryableErrorOnThirdPart_shouldCompleteExceptionallyOnlyPartsGreate @ParameterizedTest @MethodSource("responseTransformers") - void nonRetryableErrorOnFirstPart_shouldFail(AsyncResponseTransformerTestSupplier supplier) { + public void errorOnFirstPart_shouldFail(AsyncResponseTransformerTestSupplier supplier) { stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))).willReturn( aResponse() .withStatus(400) @@ -156,13 +156,13 @@ void nonRetryableErrorOnFirstPart_shouldFail(AsyncResponseTransformerTestSup @ParameterizedTest @MethodSource("responseTransformers") - public void ioError_shouldFailAndNotRetry() { + public void ioError_shouldFailAndNotRetry(AsyncResponseTransformerTestSupplier supplier) { stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))) .willReturn(aResponse() .withFault(Fault.CONNECTION_RESET_BY_PEER))); assertThatThrownBy(() -> multipartClient.getObject(b -> b.bucket(BUCKET).key(KEY), - AsyncResponseTransformer.toBlockingInputStream()).join()) + supplier.transformer()).join()) .satisfiesAnyOf( throwable -> assertThat(throwable) .hasMessageContaining("The connection was closed during the request"), @@ -248,7 +248,7 @@ private static Stream partSizeAndTransformerParams() { *

* * Retry for multipart download is supported for {@link ByteArrayAsyncResponseTransformer}, tested in - * {@link S3MultipartClientGetObjectToBytesWiremockTest}. + * {@link S3MultipartClientGetObjectRetryBehaviorWiremockTest}. */ private static Stream> responseTransformers() { return Stream.of(new AsyncResponseTransformerTestSupplier.InputStreamArtSupplier(), From 4a8d6e2bf98a73aa945c5b3bba156d3b1a52b0b2 Mon Sep 17 00:00:00 2001 From: hdavidh Date: Thu, 14 Aug 2025 12:29:36 -0700 Subject: [PATCH 15/16] Address comments --- .../async/ByteArraySplittingTransformer.java | 8 ++++---- .../s3/multipart/MultipartConfiguration.java | 4 ++-- .../S3MultipartClientGetObjectWiremockTest.java | 15 +++++++++------ 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java index 845e84fa8ae4..1ec46ede56a9 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/ByteArraySplittingTransformer.java @@ -165,12 +165,12 @@ private void handleSubscriptionCancel() { return; } - CompletableFuture> upstreamPrepareFuture = upstreamResponseTransformer.prepare(); - CompletableFutureUtils.forwardResultTo(upstreamPrepareFuture, resultFuture); + try { + CompletableFuture> upstreamPrepareFuture = upstreamResponseTransformer.prepare(); + CompletableFutureUtils.forwardResultTo(upstreamPrepareFuture, resultFuture); - upstreamResponseTransformer.onResponse(responseT.get()); + upstreamResponseTransformer.onResponse(responseT.get()); - try { buffers.keySet().stream().sorted().forEach(index -> { publisherToUpstream.send(buffers.get(index)).exceptionally(ex -> { resultFuture.completeExceptionally(SdkClientException.create("unexpected error occurred", ex)); diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java index d5d47f0e50f5..f26a22ddd893 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java @@ -164,8 +164,8 @@ public interface Builder extends CopyableBuilder * Default value: If not specified, the SDK will use the equivalent of four parts worth of memory, so 32 Mib by default. *

- * This setting is not supported and will be ignored when downloading to a byte array, i.e., when providing a - * {@link ByteArrayAsyncResponseTransformer}. + * This setting does not apply if you are using an {@link AsyncResponseTransformer} implementation that downloads the + * object into memory such as {@link AsyncResponseTransformer#toBytes} * * @param apiCallBufferSizeInBytes the value of the maximum memory usage. * @return an instance of this builder. diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java index 5bcf778f93ff..6c85b9e06843 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/S3MultipartClientGetObjectWiremockTest.java @@ -60,6 +60,7 @@ import software.amazon.awssdk.core.internal.async.FileAsyncResponseTransformer; import software.amazon.awssdk.core.internal.async.InputStreamResponseTransformer; import software.amazon.awssdk.core.internal.async.PublisherAsyncResponseTransformer; +import software.amazon.awssdk.core.internal.async.SplittingTransformer; import software.amazon.awssdk.http.nio.netty.NettyNioAsyncHttpClient; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.s3.S3AsyncClient; @@ -142,7 +143,7 @@ public void errorOnThirdPart_shouldCompleteExceptionallyOnlyPartsGreaterThan } @ParameterizedTest - @MethodSource("responseTransformers") + @MethodSource("nonRetryableResponseTransformers") public void errorOnFirstPart_shouldFail(AsyncResponseTransformerTestSupplier supplier) { stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))).willReturn( aResponse() @@ -155,8 +156,8 @@ public void errorOnFirstPart_shouldFail(AsyncResponseTransformerTestSupplier } @ParameterizedTest - @MethodSource("responseTransformers") - public void ioError_shouldFailAndNotRetry(AsyncResponseTransformerTestSupplier supplier) { + @MethodSource("nonRetryableResponseTransformers") + public void nonRetryableResponseTransformers_ioErrorOnFirstPart_shouldFailAndNotRetry(AsyncResponseTransformerTestSupplier supplier) { stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", BUCKET, KEY))) .willReturn(aResponse() .withFault(Fault.CONNECTION_RESET_BY_PEER))); @@ -175,7 +176,7 @@ public void ioError_shouldFailAndNotRetry(AsyncResponseTransformerTestSuppli } @ParameterizedTest - @MethodSource("responseTransformers") + @MethodSource("nonRetryableResponseTransformers") public void getObject_single500WithinMany200s_shouldNotRetryError(AsyncResponseTransformerTestSupplier transformerSupplier) { List> futures = new ArrayList<>(); @@ -243,14 +244,16 @@ private static Stream partSizeAndTransformerParams() { /** - * Testing {@link PublisherAsyncResponseTransformer}, {@link InputStreamResponseTransformer}, and + * Testing response transformers that are not retryable when + * {@link AsyncResponseTransformer#split(SplittingTransformerConfiguration)} is invoked and used with + * {@link SplittingTransformer} - {@link PublisherAsyncResponseTransformer}, {@link InputStreamResponseTransformer}, and * {@link FileAsyncResponseTransformer} *

* * Retry for multipart download is supported for {@link ByteArrayAsyncResponseTransformer}, tested in * {@link S3MultipartClientGetObjectRetryBehaviorWiremockTest}. */ - private static Stream> responseTransformers() { + private static Stream> nonRetryableResponseTransformers() { return Stream.of(new AsyncResponseTransformerTestSupplier.InputStreamArtSupplier(), new AsyncResponseTransformerTestSupplier.PublisherArtSupplier(), new AsyncResponseTransformerTestSupplier.FileArtSupplier()); From 4eb9324ee7f0f9c33474339be9661c8373d5596d Mon Sep 17 00:00:00 2001 From: hdavidh Date: Thu, 14 Aug 2025 14:55:46 -0700 Subject: [PATCH 16/16] Remove unused import --- .../awssdk/services/s3/multipart/MultipartConfiguration.java | 1 - 1 file changed, 1 deletion(-) diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java index f26a22ddd893..7370a383b1cf 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/multipart/MultipartConfiguration.java @@ -18,7 +18,6 @@ import software.amazon.awssdk.annotations.SdkPublicApi; import software.amazon.awssdk.core.async.AsyncRequestBody; import software.amazon.awssdk.core.async.AsyncResponseTransformer; -import software.amazon.awssdk.core.internal.async.ByteArrayAsyncResponseTransformer; import software.amazon.awssdk.services.s3.S3AsyncClient; import software.amazon.awssdk.services.s3.S3AsyncClientBuilder; import software.amazon.awssdk.services.s3.model.CopyObjectRequest;