From 435bf2ed0602a3e0dca2e407c1293699732913ab Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Wed, 14 Feb 2024 11:59:09 -0500 Subject: [PATCH 01/15] Implementation of MultipartDownloaderSubscriber and previous SplittingTransformer PR comments. Wiremock tests. - Use builder and template methods in DelegatingBufferingSubscriber --- .../core/async/AsyncResponseTransformer.java | 2 +- .../internal/async/SplittingTransformer.java | 28 +- .../IndividualPartSubscriberTckTest.java | 1 + .../async/SplittingTransformerTckTest.java | 2 +- .../async/SplittingTransformerTest.java | 10 +- .../MultipartDownloaderSubscriber.java | 101 +++--- .../MultipartDownloaderSubscriberTckTest.java | 118 +++++++ ...ipartDownloaderSubscriberWiremockTest.java | 288 ++++++++++++++++++ ...criber.java => BaseSubscriberAdapter.java} | 54 +++- .../async/DelegatingBufferingSubscriber.java | 54 ++-- .../utils/async/FlatteningSubscriber.java | 16 +- .../DelegatingBufferingSubscriberTckTest.java | 2 +- .../DelegatingBufferingSubscriberTest.java | 71 +++-- 13 files changed, 627 insertions(+), 120 deletions(-) create mode 100644 services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriberTckTest.java create mode 100644 services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriberWiremockTest.java rename utils/src/main/java/software/amazon/awssdk/utils/async/{AbstractFlatteningSubscriber.java => BaseSubscriberAdapter.java} (84%) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java index 4cfe25ed57bf..c089e834e415 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java @@ -125,7 +125,7 @@ default SplitAsyncResponseTransformer split(long bufferSize) SdkPublisher> transformer = SplittingTransformer .builder() .upstreamResponseTransformer(this) - .maximumBufferSize(bufferSize) + .maximumBufferSizeInBytes(bufferSize) .returnFuture(future) .build(); return SplitAsyncResponseTransformer.builder() diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java index 2975327a560f..1dd2e0d6235d 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java @@ -81,7 +81,7 @@ public class SplittingTransformer implements SdkPublisher implements SdkPublisher upstreamResponseTransformer, - Long bufferSize, + Long maximumBufferInBytes, CompletableFuture returnFuture) { this.upstreamResponseTransformer = Validate.paramNotNull(upstreamResponseTransformer, "asyncRequestBody"); this.returnFuture = Validate.paramNotNull(returnFuture, "returnFuture"); - this.maximumBufferSize = Validate.notNull(bufferSize, "bufferSize"); + this.maximumBufferInBytes = Validate.notNull(maximumBufferInBytes, "bufferSize"); } /** @@ -226,10 +226,11 @@ public void onResponse(ResponseT response) { public void onStream(SdkPublisher publisher) { if (onStreamCalled.compareAndSet(false, true)) { log.trace(() -> "calling onStream on the upstream transformer"); - upstreamResponseTransformer.onStream( - upstreamSubscriber -> - publisherToUpstream.subscribe(new DelegatingBufferingSubscriber(maximumBufferSize, upstreamSubscriber)) - ); + upstreamResponseTransformer.onStream(upstreamSubscriber -> publisherToUpstream.subscribe( + DelegatingBufferingSubscriber.builder() + .maximumBufferInBytes(maximumBufferInBytes) + .delegate(upstreamSubscriber) + .build())); } publisher.subscribe(new IndividualPartSubscriber<>(this.individualFuture, response, publisherToUpstream)); } @@ -248,14 +249,14 @@ static class IndividualPartSubscriber implements Subscriber { private final CompletableFuture future; private final T response; - private final SimplePublisher bodyPartPublisher; + private final SimplePublisher publisherToUpstream; private Subscription subscription; IndividualPartSubscriber(CompletableFuture future, T response, SimplePublisher bodyPartPublisher) { this.future = future; this.response = response; - this.bodyPartPublisher = bodyPartPublisher; + this.publisherToUpstream = bodyPartPublisher; } @Override @@ -274,7 +275,7 @@ public void onNext(ByteBuffer byteBuffer) { if (byteBuffer == null) { throw new NullPointerException("onNext must not be called with null byteBuffer"); } - bodyPartPublisher.send(byteBuffer).whenComplete((r, t) -> { + publisherToUpstream.send(byteBuffer).whenComplete((r, t) -> { if (t != null) { handleError(t); return; @@ -285,7 +286,6 @@ public void onNext(ByteBuffer byteBuffer) { @Override public void onError(Throwable t) { - bodyPartPublisher.error(t); handleError(t); } @@ -295,18 +295,18 @@ public void onComplete() { } private void handleError(Throwable t) { + publisherToUpstream.error(t); future.completeExceptionally(t); } } - public static Builder builder() { return new Builder<>(); } public static final class Builder { - private long maximumBufferSize; + private Long maximumBufferSize; private CompletableFuture returnFuture; private AsyncResponseTransformer upstreamResponseTransformer; @@ -336,7 +336,7 @@ public Builder upstreamResponseTransformer( * @param maximumBufferSize the amount of data buffered and the size of the chunk of data * @return an instance of this builder */ - public Builder maximumBufferSize(long maximumBufferSize) { + public Builder maximumBufferSizeInBytes(Long maximumBufferSize) { this.maximumBufferSize = maximumBufferSize; return this; } diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/IndividualPartSubscriberTckTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/IndividualPartSubscriberTckTest.java index 33bcb79da2b4..d1af5807eb11 100644 --- a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/IndividualPartSubscriberTckTest.java +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/IndividualPartSubscriberTckTest.java @@ -18,6 +18,7 @@ import java.nio.ByteBuffer; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.atomic.AtomicInteger; import org.reactivestreams.Subscriber; import org.reactivestreams.Subscription; import org.reactivestreams.tck.SubscriberWhiteboxVerification; diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/SplittingTransformerTckTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/SplittingTransformerTckTest.java index 9a81e931f97c..81f4eb3adfd9 100644 --- a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/SplittingTransformerTckTest.java +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/SplittingTransformerTckTest.java @@ -36,7 +36,7 @@ public Publisher> createPublisher(long SplittingTransformer> transformer = SplittingTransformer.>builder() .upstreamResponseTransformer(upstreamTransformer) - .maximumBufferSize(64 * 1024) + .maximumBufferSizeInBytes(64 * 1024L) .returnFuture(future) .build(); return SdkPublisher.adapt(transformer).limit(Math.toIntExact(l)); diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/SplittingTransformerTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/SplittingTransformerTest.java index 2767e8156dfd..04e25ff642b5 100644 --- a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/SplittingTransformerTest.java +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/SplittingTransformerTest.java @@ -42,7 +42,7 @@ void whenSubscriberCancelSubscription_AllDataSentToTransformer() { SplittingTransformer split = SplittingTransformer.builder() .upstreamResponseTransformer(upstreamTestTransformer) - .maximumBufferSize(1024 * 1024 * 32) + .maximumBufferSizeInBytes(1024 * 1024 * 32L) .returnFuture(future) .build(); split.subscribe(new CancelAfterNTestSubscriber( @@ -59,7 +59,7 @@ void whenSubscriberFailsAttempt_UpstreamTransformerCompletesExceptionally() { SplittingTransformer split = SplittingTransformer.builder() .upstreamResponseTransformer(upstreamTestTransformer) - .maximumBufferSize(1024 * 1024 * 32) + .maximumBufferSizeInBytes(1024 * 1024 * 32L) .returnFuture(future) .build(); split.subscribe(new FailAfterNTestSubscriber(2)); @@ -68,7 +68,7 @@ void whenSubscriberFailsAttempt_UpstreamTransformerCompletesExceptionally() { @Test void whenDataExceedsBufferSize_UpstreamShouldReceiveAllData() { - int evenBufferSize = 16 * 1024; + Long evenBufferSize = 16 * 1024L; // We send 9 split body of 7kb with a buffer size of 16kb. This is to test when uneven body size is used compared to // the buffer size, this test use a body size which does not evenly divides with the buffer size. @@ -79,7 +79,7 @@ void whenDataExceedsBufferSize_UpstreamShouldReceiveAllData() { SplittingTransformer split = SplittingTransformer.builder() .upstreamResponseTransformer(upstreamTestTransformer) - .maximumBufferSize(evenBufferSize) + .maximumBufferSizeInBytes(evenBufferSize) .returnFuture(future) .build(); split.subscribe(new CancelAfterNTestSubscriber( @@ -106,7 +106,7 @@ void whenRequestingMany_allDemandGetsFulfilled() { SplittingTransformer split = SplittingTransformer.builder() .upstreamResponseTransformer(upstreamTestTransformer) - .maximumBufferSize(1024 * 1024 * 32) + .maximumBufferSizeInBytes(1024 * 1024 * 32L) .returnFuture(future) .build(); split.subscribe(new RequestingTestSubscriber(4)); diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java index b6d7ab90f66f..9be9a7b52a76 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java @@ -27,86 +27,115 @@ import software.amazon.awssdk.services.s3.model.GetObjectResponse; import software.amazon.awssdk.utils.Logger; -// [WIP] -// Still work in progress, currently only used to help manual testing, please ignore +/** + * A subscriber implementation that will download all individual parts for a multipart get-object request. It receives the + * individual {@link AsyncResponseTransformer} which will be used to perform the individual part requests. + * This is a 'one-shot' class, it should NOT be reused for more than one multipart download + */ @SdkInternalApi public class MultipartDownloaderSubscriber implements Subscriber> { private static final Logger log = Logger.loggerFor(MultipartDownloaderSubscriber.class); + /** + * The s3 client used to make the individual part requests + */ private final S3AsyncClient s3; + + /** + * The GetObjectRequest that was provided when calling s3.getObject(...). It is copied for each individual request, and the + * copy has the partNumber field updated as more parts are downloaded. + */ private final GetObjectRequest getObjectRequest; - private AtomicBoolean totalPartKnown = new AtomicBoolean(false); - private AtomicInteger totalParts = new AtomicInteger(); - private AtomicInteger completed = new AtomicInteger(0); - private AtomicInteger currentPart = new AtomicInteger(0); + /** + * Set to true when we know the total part number of the object to get. + */ + private final AtomicBoolean totalPartKnown = new AtomicBoolean(false); + + /** + * Once total part is know, this value indicates the total number of parts of the object to get. + */ + private final AtomicInteger totalParts = new AtomicInteger(); + + /** + * The total number of completed parts. A part is considered complete once the completable future associated with its request + * completes successfully. + */ + private final AtomicInteger completedParts = new AtomicInteger(0); + /** + * The subscription received from the publisher this subscriber subscribes to. + */ private Subscription subscription; - private CompletableFuture responseFuture; - private GetObjectResponse returnResponse; + /** + * This future will be completed once this subscriber reaches a terminal state, failed or successfully, and will be + * completed accordingly. + */ + private CompletableFuture future = new CompletableFuture<>(); public MultipartDownloaderSubscriber(S3AsyncClient s3, GetObjectRequest getObjectRequest) { this.s3 = s3; this.getObjectRequest = getObjectRequest; - this.responseFuture = new CompletableFuture<>(); } @Override public void onSubscribe(Subscription s) { - if (this.subscription == null) { - this.subscription = s; + if (this.subscription != null) { + s.cancel(); + return; } - s.request(1); + this.subscription = s; + this.subscription.request(1); } @Override public void onNext(AsyncResponseTransformer asyncResponseTransformer) { - int part = currentPart.incrementAndGet(); - if (totalPartKnown.get()) { - log.info(() -> String.format("total part: %s, current part: %s", totalParts.get(), part)); - } else { - log.info(() -> String.format("part %s", part)); + if (asyncResponseTransformer == null) { + throw new NullPointerException("onNext must not be called with null asyncResponseTransformer"); } - if (totalPartKnown.get() && part > totalParts.get()) { - log.info(() -> "no more parts available, stopping"); + + int nextPartToGet = completedParts.get() + 1; + if (totalPartKnown.get() && nextPartToGet > totalParts.get()) { subscription.cancel(); return; } - GetObjectRequest actualRequest = this.getObjectRequest.copy(req -> req.partNumber(part)); + + GetObjectRequest actualRequest = getObjectRequest.copy(req -> req.partNumber(nextPartToGet)); CompletableFuture future = s3.getObject(actualRequest, asyncResponseTransformer); - future.whenComplete((response, e) -> { - if (e != null) { - responseFuture.completeExceptionally(e); + future.whenComplete((response, error) -> { + if (error != null) { + onError(error); return; } - completed.incrementAndGet(); - returnResponse = response; - log.info(() -> String.format("received '%s'", response.contentRange())); + int totalComplete = completedParts.incrementAndGet(); + log.info(() -> String.format("completed part: %s", totalComplete)); + Integer partCount = response.partsCount(); - if (totalPartKnown.compareAndSet(false, true)) { + if (partCount != null && totalPartKnown.compareAndSet(false, true)) { + log.info(() -> String.format("total parts: %s", partCount)); totalParts.set(partCount); totalPartKnown.set(true); } - log.info(() -> String.format("total parts: %s", partCount)); - if (totalParts.get() > 1) { + if (partCount != null && requiresMultipart()) { subscription.request(1); + } else { + subscription.cancel(); } }); } + private boolean requiresMultipart() { + return totalParts.get() > 1; + } + @Override public void onError(Throwable t) { - responseFuture.completeExceptionally(t); + future.completeExceptionally(t); } @Override public void onComplete() { - responseFuture.complete(returnResponse); + future.complete(null); } - - public CompletableFuture future() { - return responseFuture; - } - } diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriberTckTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriberTckTest.java new file mode 100644 index 000000000000..7fab37f7d71a --- /dev/null +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriberTckTest.java @@ -0,0 +1,118 @@ +/* + * 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 org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.when; + +import java.nio.ByteBuffer; +import java.util.concurrent.CompletableFuture; +import org.mockito.Mockito; +import org.reactivestreams.Subscriber; +import org.reactivestreams.Subscription; +import org.reactivestreams.tck.SubscriberWhiteboxVerification; +import org.reactivestreams.tck.TestEnvironment; +import software.amazon.awssdk.core.async.AsyncResponseTransformer; +import software.amazon.awssdk.core.async.SdkPublisher; +import software.amazon.awssdk.services.s3.S3AsyncClient; +import software.amazon.awssdk.services.s3.model.GetObjectRequest; +import software.amazon.awssdk.services.s3.model.GetObjectResponse; + +public class MultipartDownloaderSubscriberTckTest + extends SubscriberWhiteboxVerification> { + private S3AsyncClient s3mock; + + public MultipartDownloaderSubscriberTckTest() { + super(new TestEnvironment()); + this.s3mock = Mockito.mock(S3AsyncClient.class); + } + + @Override + public Subscriber> + createSubscriber(WhiteboxSubscriberProbe> probe) { + CompletableFuture responseFuture = + CompletableFuture.completedFuture(GetObjectResponse.builder().partsCount(4).build()); + when(s3mock.getObject(any(GetObjectRequest.class), any(AsyncResponseTransformer.class))).thenReturn(responseFuture); + return new MultipartDownloaderSubscriber(s3mock, GetObjectRequest.builder() + .bucket("test-bucket-unused") + .key("test-key-unused") + .build()) { + @Override + public void onError(Throwable throwable) { + super.onError(throwable); + probe.registerOnError(throwable); + } + + @Override + public void onSubscribe(Subscription subscription) { + super.onSubscribe(subscription); + probe.registerOnSubscribe(new SubscriberPuppet() { + @Override + public void triggerRequest(long elements) { + subscription.request(elements); + } + + @Override + public void signalCancel() { + subscription.cancel(); + } + }); + } + + @Override + public void onNext(AsyncResponseTransformer item) { + super.onNext(item); + probe.registerOnNext(item); + } + + @Override + public void onComplete() { + super.onComplete(); + probe.registerOnComplete(); + } + }; + } + + @Override + public AsyncResponseTransformer createElement(int element) { + return new TestAsyncResponseTransformer(); + } + + private static class TestAsyncResponseTransformer implements AsyncResponseTransformer { + private CompletableFuture future; + + @Override + public CompletableFuture prepare() { + this.future = new CompletableFuture<>(); + return this.future; + } + + @Override + public void onResponse(GetObjectResponse response) { + this.future.complete(response); + } + + @Override + public void onStream(SdkPublisher publisher) { + // do nothing, test + } + + @Override + public void exceptionOccurred(Throwable error) { + future.completeExceptionally(error); + } + } +} \ No newline at end of file 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/MultipartDownloaderSubscriberWiremockTest.java new file mode 100644 index 000000000000..eadef03005ee --- /dev/null +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriberWiremockTest.java @@ -0,0 +1,288 @@ +/* + * 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.urlMatching; +import static com.github.tomakehurst.wiremock.client.WireMock.verify; +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.fail; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; +import com.github.tomakehurst.wiremock.junit5.WireMockTest; +import com.google.common.jimfs.Jimfs; +import java.io.IOException; +import java.net.URI; +import java.nio.ByteBuffer; +import java.nio.file.FileSystem; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; +import java.util.UUID; +import java.util.concurrent.CountDownLatch; +import java.util.function.Function; +import java.util.stream.Stream; +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 org.reactivestreams.Subscription; +import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; +import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; +import software.amazon.awssdk.core.ResponseBytes; +import software.amazon.awssdk.core.ResponseInputStream; +import software.amazon.awssdk.core.async.AsyncResponseTransformer; +import software.amazon.awssdk.core.async.ResponsePublisher; +import software.amazon.awssdk.core.async.SplitAsyncResponseTransformer; +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.testutils.InputStreamUtils; + +@WireMockTest +class MultipartDownloaderSubscriberWiremockTest { + + private S3AsyncClient s3AsyncClient; + private String testBucket = "test-bucket"; + private String testKey = "test-key"; + + @BeforeEach + public void init(WireMockRuntimeInfo wiremock) { + s3AsyncClient = S3AsyncClient.builder() + .credentialsProvider(StaticCredentialsProvider.create( + AwsBasicCredentials.create("key", "secret"))) + .region(Region.US_WEST_2) + .endpointOverride(URI.create("http://localhost:" + wiremock.getHttpPort())) + .serviceConfiguration(S3Configuration.builder() + .pathStyleAccessEnabled(true) + .build()) + .build(); + } + + @ParameterizedTest + @MethodSource("partsParamProvider") + void happyPath_ByteArrayAsyncResponseTransformer(int amountOfPartToTest, int partSize) { + for (int i = 1; i <= amountOfPartToTest; i++) { + stubForPart(i, amountOfPartToTest, partSize); + } + + MultipartDownloaderSubscriber subscriber = new MultipartDownloaderSubscriber( + s3AsyncClient, + GetObjectRequest.builder() + .bucket(testBucket) + .key(testKey) + .build()); + + SplitAsyncResponseTransformer> split = + AsyncResponseTransformer.toBytes().split(1024 * 16); + + split.publisher().subscribe(subscriber); + ResponseBytes responseBytes = split.preparedFuture().join(); + + byte[] fullBody = responseBytes.asByteArray(); + byte[] expectedBody = expectedBody(amountOfPartToTest, partSize); + assertArrayEquals(expectedBody, fullBody); + + verifyCorrectAmountOfRequestsMade(amountOfPartToTest); + } + + @ParameterizedTest + @MethodSource("partsParamProvider") + void happyPath_InputStreamResponseTransformer(int amountOfPartToTest, int partSize) { + for (int i = 1; i <= amountOfPartToTest; i++) { + stubForPart(i, amountOfPartToTest, partSize); + } + + MultipartDownloaderSubscriber subscriber = new MultipartDownloaderSubscriber( + s3AsyncClient, + GetObjectRequest.builder() + .bucket(testBucket) + .key(testKey) + .build()); + + SplitAsyncResponseTransformer> split = + AsyncResponseTransformer.toBlockingInputStream().split(1024 * 16); + + split.publisher().subscribe(subscriber); + ResponseInputStream responseInputStream = split.preparedFuture().join(); + + byte[] fullBody = InputStreamUtils.drainInputStream(responseInputStream); + byte[] expectedBody = expectedBody(amountOfPartToTest, partSize); + assertArrayEquals(expectedBody, fullBody); + + verifyCorrectAmountOfRequestsMade(amountOfPartToTest); + + } + + @ParameterizedTest + @MethodSource("partsParamProvider") + void happyPath_PublisherAsyncResponseTransformer(int amountOfPartToTest, int partSize) { + for (int i = 1; i <= amountOfPartToTest; i++) { + stubForPart(i, amountOfPartToTest, partSize); + } + + MultipartDownloaderSubscriber subscriber = new MultipartDownloaderSubscriber( + s3AsyncClient, + GetObjectRequest.builder() + .bucket(testBucket) + .key(testKey) + .build()); + + SplitAsyncResponseTransformer> split = + AsyncResponseTransformer.toPublisher().split(1024 * 16); + + split.publisher().subscribe(subscriber); + ResponsePublisher responsePublisher = split.preparedFuture().join(); + + List bodyBytes = new ArrayList<>(); + CountDownLatch latch = new CountDownLatch(1); + responsePublisher.subscribe(new Subscriber() { + Subscription s; + @Override + public void onSubscribe(Subscription s) { + this.s = s; + s.request(1); + } + + @Override + public void onNext(ByteBuffer byteBuffer) { + while (byteBuffer.remaining() > 0) { + bodyBytes.add(byteBuffer.get()); + } + s.request(1); + } + + @Override + public void onError(Throwable t) { + latch.countDown(); + fail("Unexpected onError during test", t); + } + + @Override + public void onComplete() { + latch.countDown(); + } + }); + + try { + latch.await(); + } catch (InterruptedException e) { + fail("Unexpected thread interruption during test", e); + } + + byte[] fullBody = unbox(bodyBytes.toArray(new Byte[0])); + byte[] expectedBody = expectedBody(amountOfPartToTest, partSize); + assertArrayEquals(expectedBody, fullBody); + + verifyCorrectAmountOfRequestsMade(amountOfPartToTest); + } + + @ParameterizedTest + @MethodSource("partsParamProvider") + void happyPath_FileAsyncResponseTransformer(int amountOfPartToTest, int partSize) { + for (int i = 1; i <= amountOfPartToTest; i++) { + stubForPart(i, amountOfPartToTest, partSize); + } + + MultipartDownloaderSubscriber subscriber = new MultipartDownloaderSubscriber( + s3AsyncClient, + GetObjectRequest.builder() + .bucket(testBucket) + .key(testKey) + .build()); + + FileSystem fs = Jimfs.newFileSystem(); + String filePath = "/tmp-file-" + UUID.randomUUID(); + Path inMemoryFilePath = fs.getPath(filePath); + SplitAsyncResponseTransformer split = + AsyncResponseTransformer.toFile(inMemoryFilePath).split(1024 * 16); + + split.publisher().subscribe(subscriber); + GetObjectResponse response = split.preparedFuture().join(); + + Path savedFile = fs.getPath(filePath); + byte[] expectedBody = expectedBody(amountOfPartToTest, partSize); + byte[] fullBody = null; + try { + fullBody = Files.readAllBytes(savedFile); + } catch (IOException e) { + fail("Unexpected IOException during test", e); + } + + assertArrayEquals(expectedBody, fullBody); + verifyCorrectAmountOfRequestsMade(amountOfPartToTest); + + } + + private static Stream partsParamProvider() { + return Stream.of( + arguments(4, 16), + arguments(1, 1024), + arguments(31, 1243) + ); + } + + private void verifyCorrectAmountOfRequestsMade(int amountOfPartToTest) { + String urlTemplate = ".*partNumber=%d.*"; + for (int i = 1; i <= amountOfPartToTest; i++) { + verify(getRequestedFor(urlMatching(String.format(urlTemplate, i)))); + } + verify(0, getRequestedFor(urlMatching(String.format(urlTemplate, amountOfPartToTest + 1)))); + } + + private void stubForPart(int part, int totalPart, int partSize) { + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%d", testBucket, testKey, part))).willReturn( + aResponse() + .withHeader("x-amz-mp-parts-count", totalPart + "") + .withBody(byteArrayOfLength(partSize, (byte) part)))); + } + + private byte[] expectedBody(int totalPart, int partSize) { + Stream.Builder> s = Stream.builder(); + for (int i = 1; i <= totalPart; i++) { + int j = i; + s.add(Stream.generate(() -> (byte) j).limit(partSize)); + } + return unbox(s.build().flatMap(Function.identity()).toArray(Byte[]::new)); + } + + private byte[] unbox(Byte[] arr) { + byte[] bb = new byte[arr.length]; + int i = 0; + for (Byte b : arr) { + bb[i] = b.byteValue(); + i++; + } + return bb; + } + + private byte[] byteArrayOfLength(int length, byte value) { + byte[] b = new byte[length]; + Arrays.fill(b, value); + return b; + } + +} \ No newline at end of file diff --git a/utils/src/main/java/software/amazon/awssdk/utils/async/AbstractFlatteningSubscriber.java b/utils/src/main/java/software/amazon/awssdk/utils/async/BaseSubscriberAdapter.java similarity index 84% rename from utils/src/main/java/software/amazon/awssdk/utils/async/AbstractFlatteningSubscriber.java rename to utils/src/main/java/software/amazon/awssdk/utils/async/BaseSubscriberAdapter.java index d3c998a204e7..6b434d2d162d 100644 --- a/utils/src/main/java/software/amazon/awssdk/utils/async/AbstractFlatteningSubscriber.java +++ b/utils/src/main/java/software/amazon/awssdk/utils/async/BaseSubscriberAdapter.java @@ -20,12 +20,20 @@ import java.util.concurrent.atomic.AtomicReference; import org.reactivestreams.Subscriber; import org.reactivestreams.Subscription; -import software.amazon.awssdk.annotations.SdkInternalApi; +import software.amazon.awssdk.annotations.SdkProtectedApi; import software.amazon.awssdk.utils.Logger; -@SdkInternalApi -public abstract class AbstractFlatteningSubscriber extends DelegatingSubscriber { - private static final Logger log = Logger.loggerFor(AbstractFlatteningSubscriber.class); +/** + * Base of subscribers that can adapt one type to another. Thess subscriber will receive onNext signal with the {@code U} type, + * but will need to {@link BaseSubscriberAdapter#fulfillDownstreamDemand() fulfill the dpwnstream demand} of the delegate + * subscriber with instance of the {@code T} type. + * + * @param the type that the delegate subscriber demands. + * @param the type sent by the publisher this subscriber is subscribed to. + */ +@SdkProtectedApi +public abstract class BaseSubscriberAdapter extends DelegatingSubscriber { + private static final Logger log = Logger.loggerFor(BaseSubscriberAdapter.class); /** * The amount of unfulfilled demand open against the upstream subscriber. @@ -66,7 +74,7 @@ public abstract class AbstractFlatteningSubscriber extends DelegatingSubsc */ protected Subscription upstreamSubscription; - protected AbstractFlatteningSubscriber(Subscriber subscriber) { + protected BaseSubscriberAdapter(Subscriber subscriber) { super(subscriber); } @@ -76,7 +84,7 @@ protected AbstractFlatteningSubscriber(Subscriber subscriber) { * * @param item the value with which onNext was called. */ - protected abstract void doWithItem(T item); + abstract void doWithItem(T item); /** * This method is called when demand from the downstream subscriber needs to be fulfilled. Called in a loop @@ -202,8 +210,16 @@ private void handleOnNextState() { * Returns true if we need to call onNext downstream. If this is executed outside the handling-state-update condition, the * result is subject to change. */ - protected boolean onNextNeeded() { - return downstreamDemand.get() > 0; + private boolean onNextNeeded() { + return downstreamDemand.get() > 0 && additionalOnNextNeededCheck(); + } + + + /** + * Can be overridden by subclasses to provide additional checks before calling onNext on downstream. + */ + boolean additionalOnNextNeededCheck() { + return true; } /** @@ -218,8 +234,15 @@ private void handleUpstreamDemandState() { /** * Returns true if we need to increase our upstream demand. */ - protected boolean upstreamDemandNeeded() { - return upstreamDemand.get() <= 0 && downstreamDemand.get() > 0; + private boolean upstreamDemandNeeded() { + return upstreamDemand.get() <= 0 && downstreamDemand.get() > 0 && additionalUpstreamDemandNeededCheck(); + } + + /** + * Can be overridden by subclasses to provide additional checks to see if we need to increase our upstream demand. + */ + boolean additionalUpstreamDemandNeededCheck() { + return true; } /** @@ -236,8 +259,15 @@ private void handleOnCompleteState() { * Returns true if we need to call onComplete downstream. If this is executed outside the handling-state-update condition, the * result is subject to change. */ - protected boolean onCompleteNeeded() { - return onCompleteCalledByUpstream && !terminalCallMadeDownstream; + private boolean onCompleteNeeded() { + return onCompleteCalledByUpstream && !terminalCallMadeDownstream && additionalOnCompleteNeededCheck(); + } + + /** + * Can be overridden by subclasses to provide additional checks before calling onComplete on downstream + */ + boolean additionalOnCompleteNeededCheck() { + return true; } /** diff --git a/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java b/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java index 611e6205b64f..68053cdf6152 100644 --- a/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java +++ b/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java @@ -24,13 +24,14 @@ import org.reactivestreams.Subscriber; import org.reactivestreams.Subscription; import software.amazon.awssdk.annotations.SdkProtectedApi; +import software.amazon.awssdk.utils.Validate; @SdkProtectedApi -public class DelegatingBufferingSubscriber extends AbstractFlatteningSubscriber { +public class DelegatingBufferingSubscriber extends BaseSubscriberAdapter { /** * The maximum amount of bytes allowed to be stored in the StoringSubscriber */ - private final long maximumBuffer; + private final long maximumBufferInBytes; /** * Current amount of bytes buffered in the StoringSubscriber @@ -43,17 +44,16 @@ public class DelegatingBufferingSubscriber extends AbstractFlatteningSubscriber< */ private final StoringSubscriber storage = new StoringSubscriber<>(Integer.MAX_VALUE); - public DelegatingBufferingSubscriber(long maximumBuffer, Subscriber subscriber) { - super(subscriber); - this.maximumBuffer = maximumBuffer; + protected DelegatingBufferingSubscriber(Long maximumBufferInBytes, Subscriber delegate) { + super(Validate.notNull(delegate, "delegate must not be null")); + this.maximumBufferInBytes = Validate.notNull(maximumBufferInBytes, "maximumBufferInBytes msut not be null"); } @Override public void onNext(ByteBuffer item) { - if (currentlyBuffered.get() + item.remaining() > maximumBuffer) { + if (currentlyBuffered.get() > 0) { flushStorageToDelegate(); } - // calling super.onNext will eventually fulfill any remaining demand super.onNext(item); } @@ -64,8 +64,8 @@ public void onSubscribe(Subscription subscription) { } @Override - protected void doWithItem(ByteBuffer buffer) { - if (currentlyBuffered.get() + buffer.remaining() > maximumBuffer) { + void doWithItem(ByteBuffer buffer) { + if (currentlyBuffered.get() > 0) { flushStorageToDelegate(); } storage.onNext(buffer.duplicate()); @@ -81,26 +81,24 @@ protected void fulfillDownstreamDemand() { * Returns true if we need to call onNext downstream. */ @Override - protected boolean onNextNeeded() { - return super.onNextNeeded() && storage.peek().isPresent() && storage.peek().get().type() == ON_NEXT; + boolean additionalOnNextNeededCheck() { + return storage.peek().map(event -> event.type() == ON_NEXT).orElse(false); } /** * Returns true if we need to call onComplete downstream. */ @Override - protected boolean onCompleteNeeded() { - boolean emptyStorage = !storage.peek().isPresent(); - boolean storageReceivedOnComplete = storage.peek().isPresent() && storage.peek().get().type() == ON_COMPLETE; - return super.onCompleteNeeded() && (emptyStorage || storageReceivedOnComplete); + boolean additionalOnCompleteNeededCheck() { + return storage.peek().map(event -> event.type() == ON_COMPLETE).orElse(true); } /** * Returns true if we need to increase our upstream demand. */ @Override - protected boolean upstreamDemandNeeded() { - return super.upstreamDemandNeeded() && currentlyBuffered.get() < maximumBuffer; + boolean additionalUpstreamDemandNeededCheck() { + return currentlyBuffered.get() < maximumBufferInBytes; } private void flushStorageToDelegate() { @@ -118,4 +116,26 @@ private void flushStorageToDelegate() { } } + public static Builder builder() { + return new Builder(); + } + + public static final class Builder { + private Long maximumBufferInBytes; + private Subscriber delegate; + + public Builder maximumBufferInBytes(Long maximumBufferInBytes) { + this.maximumBufferInBytes = maximumBufferInBytes; + return this; + } + + public Builder delegate(Subscriber delegate) { + this.delegate = delegate; + return this; + } + + public DelegatingBufferingSubscriber build() { + return new DelegatingBufferingSubscriber(maximumBufferInBytes, delegate); + } + } } diff --git a/utils/src/main/java/software/amazon/awssdk/utils/async/FlatteningSubscriber.java b/utils/src/main/java/software/amazon/awssdk/utils/async/FlatteningSubscriber.java index dcb45e92d50e..5b9fdd7a300d 100644 --- a/utils/src/main/java/software/amazon/awssdk/utils/async/FlatteningSubscriber.java +++ b/utils/src/main/java/software/amazon/awssdk/utils/async/FlatteningSubscriber.java @@ -21,7 +21,7 @@ import software.amazon.awssdk.utils.Validate; @SdkProtectedApi -public class FlatteningSubscriber extends AbstractFlatteningSubscriber, U> { +public class FlatteningSubscriber extends BaseSubscriberAdapter, U> { /** * Items given to us by the upstream subscriber that we will use to fulfill demand of the downstream subscriber. @@ -33,7 +33,7 @@ public FlatteningSubscriber(Subscriber subscriber) { } @Override - protected void doWithItem(Iterable nextItems) { + void doWithItem(Iterable nextItems) { nextItems.forEach(item -> { Validate.notNull(nextItems, "Collections flattened by the flattening subscriber must not contain null."); allItems.add(item); @@ -51,16 +51,16 @@ protected void fulfillDownstreamDemand() { * result is subject to change. */ @Override - protected boolean onNextNeeded() { - return super.onNextNeeded() && !allItems.isEmpty(); + boolean additionalOnNextNeededCheck() { + return !allItems.isEmpty(); } /** * Returns true if we need to increase our upstream demand. */ @Override - protected boolean upstreamDemandNeeded() { - return super.upstreamDemandNeeded() && allItems.isEmpty(); + boolean additionalUpstreamDemandNeededCheck() { + return allItems.isEmpty(); } /** @@ -68,8 +68,8 @@ protected boolean upstreamDemandNeeded() { * result is subject to change. */ @Override - protected boolean onCompleteNeeded() { - return super.onCompleteNeeded() && allItems.isEmpty(); + boolean additionalOnCompleteNeededCheck() { + return allItems.isEmpty(); } @Override diff --git a/utils/src/test/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriberTckTest.java b/utils/src/test/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriberTckTest.java index 363aa93ee782..cdb2ec4a9a41 100644 --- a/utils/src/test/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriberTckTest.java +++ b/utils/src/test/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriberTckTest.java @@ -32,7 +32,7 @@ protected DelegatingBufferingSubscriberTckTest() { @Override public Subscriber createSubscriber(WhiteboxSubscriberProbe probe) { Subscriber delegate = new NoOpSubscriber(); - return new DelegatingBufferingSubscriber(1024, delegate) { + return new DelegatingBufferingSubscriber(1024L, delegate) { @Override public void onSubscribe(Subscription s) { super.onSubscribe(s); diff --git a/utils/src/test/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriberTest.java b/utils/src/test/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriberTest.java index 0f68b7c15f87..9db66ace0755 100644 --- a/utils/src/test/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriberTest.java +++ b/utils/src/test/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriberTest.java @@ -39,17 +39,20 @@ class DelegatingBufferingSubscriberTest { @Test void givenMultipleBufferTotalToBufferSize_ExpectSubscriberGetThemAll() { TestSubscriber testSubscriber = new TestSubscriber(32); - Subscriber subscriber = new DelegatingBufferingSubscriber(32, testSubscriber); + Subscriber subscriber = DelegatingBufferingSubscriber.builder() + .maximumBufferInBytes(32L) + .delegate(testSubscriber) + .build(); SimplePublisher publisher = new SimplePublisher<>(); publisher.subscribe(subscriber); testSubscriber.assertNothingReceived(); for (int i = 0; i < 3; i++) { - ByteBuffer buff = byteArrayWithValue((byte)i, 8); + ByteBuffer buff = byteArrayWithValue((byte) i, 8); publisher.send(buff); } - ByteBuffer buff = byteArrayWithValue((byte)3, 8); + ByteBuffer buff = byteArrayWithValue((byte) 3, 8); publisher.send(buff); assertThat(testSubscriber.onNextCallAmount).isEqualTo(4); assertThat(testSubscriber.totalReceived).isEqualTo(32); @@ -65,13 +68,16 @@ void givenMultipleBufferTotalToBufferSize_ExpectSubscriberGetThemAll() { @Test void givenMultipleBufferLessThenBufferSize_ExpectSubscriberGetThemAll() { TestSubscriber testSubscriber = new TestSubscriber(32); - Subscriber subscriber = new DelegatingBufferingSubscriber(64, testSubscriber); + Subscriber subscriber = DelegatingBufferingSubscriber.builder() + .maximumBufferInBytes(64L) + .delegate(testSubscriber) + .build(); SimplePublisher publisher = new SimplePublisher<>(); publisher.subscribe(subscriber); testSubscriber.assertNothingReceived(); for (int i = 0; i < 4; i++) { - ByteBuffer buff = byteArrayWithValue((byte)i, 8); + ByteBuffer buff = byteArrayWithValue((byte) i, 8); publisher.send(buff); } @@ -83,18 +89,21 @@ void givenMultipleBufferLessThenBufferSize_ExpectSubscriberGetThemAll() { @Test void exceedsBufferInMultipleChunk_BytesReceivedInMultipleBatches() { TestSubscriber testSubscriber = new TestSubscriber(64); - Subscriber subscriber = new DelegatingBufferingSubscriber(32, testSubscriber); + Subscriber subscriber = DelegatingBufferingSubscriber.builder() + .maximumBufferInBytes(32L) + .delegate(testSubscriber) + .build(); SimplePublisher publisher = new SimplePublisher<>(); publisher.subscribe(subscriber); testSubscriber.assertNothingReceived(); for (int i = 0; i < 3; i++) { - ByteBuffer buff = byteArrayWithValue((byte)i, 8); + ByteBuffer buff = byteArrayWithValue((byte) i, 8); publisher.send(buff); } for (int i = 3; i < 8; i++) { - ByteBuffer buff = byteArrayWithValue((byte)i, 8); + ByteBuffer buff = byteArrayWithValue((byte) i, 8); publisher.send(buff); } testSubscriber.assertBytesReceived(8, 64); @@ -109,11 +118,14 @@ void exceedsBufferInMultipleChunk_BytesReceivedInMultipleBatches() { @Test void whenDataExceedsBufferSingle_ExpectAllBytesReceived() { TestSubscriber testSubscriber = new TestSubscriber(256); - Subscriber subscriber = new DelegatingBufferingSubscriber(32, testSubscriber); + Subscriber subscriber = DelegatingBufferingSubscriber.builder() + .maximumBufferInBytes(32L) + .delegate(testSubscriber) + .build(); SimplePublisher publisher = new SimplePublisher<>(); publisher.subscribe(subscriber); - publisher.send(byteArrayWithValue((byte)0, 256)); + publisher.send(byteArrayWithValue((byte) 0, 256)); testSubscriber.assertBytesReceived(1, 256); publisher.complete(); @@ -125,16 +137,19 @@ void whenDataExceedsBufferSingle_ExpectAllBytesReceived() { @Test void multipleBuffer_unevenSizes() { TestSubscriber testSubscriber = new TestSubscriber(59); - Subscriber subscriber = new DelegatingBufferingSubscriber(32, testSubscriber); + Subscriber subscriber = DelegatingBufferingSubscriber.builder() + .maximumBufferInBytes(32L) + .delegate(testSubscriber) + .build(); SimplePublisher publisher = new SimplePublisher<>(); publisher.subscribe(subscriber); testSubscriber.assertNothingReceived(); - publisher.send(byteArrayWithValue((byte)0, 9)); + publisher.send(byteArrayWithValue((byte) 0, 9)); - publisher.send(byteArrayWithValue((byte)1, 20)); + publisher.send(byteArrayWithValue((byte) 1, 20)); - publisher.send(byteArrayWithValue((byte)2, 30)); + publisher.send(byteArrayWithValue((byte) 2, 30)); testSubscriber.assertBytesReceived(3, 59); publisher.complete(); @@ -143,13 +158,13 @@ void multipleBuffer_unevenSizes() { ByteBuffer received = testSubscriber.received; received.position(0); for (int i = 0; i < 9; i++) { - assertThat(received.get()).isEqualTo((byte)0); + assertThat(received.get()).isEqualTo((byte) 0); } for (int i = 0; i < 20; i++) { - assertThat(received.get()).isEqualTo((byte)1); + assertThat(received.get()).isEqualTo((byte) 1); } for (int i = 0; i < 30; i++) { - assertThat(received.get()).isEqualTo((byte)2); + assertThat(received.get()).isEqualTo((byte) 2); } } @@ -157,15 +172,18 @@ void multipleBuffer_unevenSizes() { void stochastic_ExpectAllBytesReceived() { AtomicInteger i = new AtomicInteger(0); int totalSendToMake = 16; - TestSubscriber testSubscriber = new TestSubscriber(512*1024); - Subscriber subscriber = new DelegatingBufferingSubscriber(32*1024, testSubscriber); + TestSubscriber testSubscriber = new TestSubscriber(512 * 1024); + Subscriber subscriber = DelegatingBufferingSubscriber.builder() + .maximumBufferInBytes(32 * 1024L) + .delegate(testSubscriber) + .build(); SimplePublisher publisher = new SimplePublisher<>(); publisher.subscribe(subscriber); ExecutorService executor = Executors.newFixedThreadPool(8); CountDownLatch latch = new CountDownLatch(totalSendToMake); for (int j = 0; j < totalSendToMake; j++) { executor.submit(() -> { - ByteBuffer buffer = byteArrayWithValue((byte) i.incrementAndGet(), 32*1024); + ByteBuffer buffer = byteArrayWithValue((byte) i.incrementAndGet(), 32 * 1024); publisher.send(buffer).whenComplete((res, err) -> { if (err != null) { fail("unexpected error sending data"); @@ -180,18 +198,21 @@ void stochastic_ExpectAllBytesReceived() { fail("Test interrupted while waiting for all submitted task to finish"); } publisher.complete(); - testSubscriber.assertBytesReceived(totalSendToMake, 512*1024); + testSubscriber.assertBytesReceived(totalSendToMake, 512 * 1024); } @Test void publisherError_ExpectSubscriberOnErrorToBeCalled() { TestSubscriber testSubscriber = new TestSubscriber(32); - Subscriber subscriber = new DelegatingBufferingSubscriber(32, testSubscriber); + Subscriber subscriber = DelegatingBufferingSubscriber.builder() + .maximumBufferInBytes(32L) + .delegate(testSubscriber) + .build(); SimplePublisher publisher = new SimplePublisher<>(); publisher.subscribe(subscriber); for (int i = 0; i < 4; i++) { - ByteBuffer buff = byteArrayWithValue((byte)i, 8); + ByteBuffer buff = byteArrayWithValue((byte) i, 8); publisher.send(buff); } publisher.error(new RuntimeException("test exception")); @@ -249,9 +270,9 @@ void assertBytesReceived(int timesOnNextWasCalled, int totalBytesReceived) { void assertAllReceivedInChunk(int chunkSize) { received.position(0); - for (int i = 0; i < totalReceived/chunkSize; i++) { + for (int i = 0; i < totalReceived / chunkSize; i++) { for (int j = 0; j < chunkSize; j++) { - assertThat(received.get(i * chunkSize + j)).isEqualTo((byte)i); + assertThat(received.get(i * chunkSize + j)).isEqualTo((byte) i); } } } From f881541843369049322abf057db6cbf80d5e6300 Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Wed, 14 Feb 2024 17:01:06 -0500 Subject: [PATCH 02/15] fix DelegatingBufferingSubscriber not considering downstream demand --- .../awssdk/utils/async/DelegatingBufferingSubscriber.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java b/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java index 68053cdf6152..c89c95d8e35e 100644 --- a/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java +++ b/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java @@ -65,9 +65,6 @@ public void onSubscribe(Subscription subscription) { @Override void doWithItem(ByteBuffer buffer) { - if (currentlyBuffered.get() > 0) { - flushStorageToDelegate(); - } storage.onNext(buffer.duplicate()); currentlyBuffered.addAndGet(buffer.remaining()); } @@ -104,7 +101,7 @@ boolean additionalUpstreamDemandNeededCheck() { private void flushStorageToDelegate() { long totalBufferRemaining = currentlyBuffered.get(); Optional> next = storage.poll(); - while (totalBufferRemaining > 0) { + while (totalBufferRemaining > 0 && downstreamDemand.get() > 0) { if (!next.isPresent() || next.get().type() != ON_NEXT) { break; } @@ -113,6 +110,7 @@ private void flushStorageToDelegate() { currentlyBuffered.addAndGet(-byteBufferEvent.value().remaining()); subscriber.onNext(byteBufferEvent.value()); next = storage.poll(); + downstreamDemand.decrementAndGet(); } } From d799628b564c9b80337f91cb3ba144a91fa0f6c9 Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Fri, 16 Feb 2024 11:28:26 -0500 Subject: [PATCH 03/15] fix DelegatingBufferingSubscriber not always respecting downstream demand --- .../internal/async/SplittingTransformer.java | 21 +- .../MultipartDownloaderSubscriber.java | 67 ++-- ...ipartDownloaderSubscriberWiremockTest.java | 340 +++++++++--------- .../AsyncResponseTransformerTestSupplier.java | 55 +++ .../utils/async/BaseSubscriberAdapter.java | 12 +- .../async/DelegatingBufferingSubscriber.java | 32 +- 6 files changed, 312 insertions(+), 215 deletions(-) create mode 100644 services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java index 1dd2e0d6235d..1f527ae008e2 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java @@ -111,7 +111,7 @@ private SplittingTransformer(AsyncResponseTransformer upstre CompletableFuture returnFuture) { this.upstreamResponseTransformer = Validate.paramNotNull(upstreamResponseTransformer, "asyncRequestBody"); this.returnFuture = Validate.paramNotNull(returnFuture, "returnFuture"); - this.maximumBufferInBytes = Validate.notNull(maximumBufferInBytes, "bufferSize"); + this.maximumBufferInBytes = Validate.notNull(maximumBufferInBytes, "maximumBufferInBytes"); } /** @@ -154,8 +154,7 @@ public void request(long n) { public void cancel() { if (isCancelled.compareAndSet(false, true)) { log.trace(() -> "Cancelling splitting transformer"); - publisherToUpstream.complete(); - downstreamSubscriber = null; + handleCancelState(); } } } @@ -190,6 +189,14 @@ private boolean doEmit() { return false; } + private synchronized void handleCancelState() { + if (downstreamSubscriber == null) { + return; + } + publisherToUpstream.complete(); + downstreamSubscriber = null; + } + /** * The AsyncResponseTransformer for each of the individual requests that is sent back to the downstreamSubscriber when * requested. A future is created per request that is completed when onComplete is called on the subscriber for that request @@ -209,6 +216,11 @@ public CompletableFuture prepare() { CompletableFutureUtils.forwardExceptionTo(returnFuture, upstreamFuture); CompletableFutureUtils.forwardResultTo(upstreamFuture, returnFuture); } + individualFuture.whenComplete((r, e) -> { + if (isCancelled.get()) { + handleCancelState(); + } + }); return this.individualFuture; } @@ -266,8 +278,7 @@ public void onSubscribe(Subscription s) { return; } this.subscription = s; - // request everything, data will be buffered by the DelegatingBufferingSubscriber - s.request(Long.MAX_VALUE); + s.request(1); } @Override diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java index 9be9a7b52a76..334a67914eec 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java @@ -16,7 +16,6 @@ package software.amazon.awssdk.services.s3.internal.multipart; import java.util.concurrent.CompletableFuture; -import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; import org.reactivestreams.Subscriber; import org.reactivestreams.Subscription; @@ -48,14 +47,10 @@ public class MultipartDownloaderSubscriber implements Subscriber future = new CompletableFuture<>(); + private final CompletableFuture future = new CompletableFuture<>(); + + private final Object lock = new Object(); + + /** + * The etag of the object being downloaded. + */ + private String eTag; public MultipartDownloaderSubscriber(S3AsyncClient s3, GetObjectRequest getObjectRequest) { this.s3 = s3; @@ -96,12 +98,14 @@ public void onNext(AsyncResponseTransformer totalParts.get()) { - subscription.cancel(); - return; + + if (totalParts != null && nextPartToGet > totalParts) { + synchronized (lock) { + subscription.cancel(); + } } - GetObjectRequest actualRequest = getObjectRequest.copy(req -> req.partNumber(nextPartToGet)); + GetObjectRequest actualRequest = nextRequest(nextPartToGet); CompletableFuture future = s3.getObject(actualRequest, asyncResponseTransformer); future.whenComplete((response, error) -> { if (error != null) { @@ -109,26 +113,28 @@ public void onNext(AsyncResponseTransformer String.format("completed part: %s", totalComplete)); + log.trace(() -> String.format("completed part: %s", totalComplete)); + + if (eTag == null) { + this.eTag = response.eTag(); + log.trace(() -> String.format("Multipart object ETag: %s", this.eTag)); + } Integer partCount = response.partsCount(); - if (partCount != null && totalPartKnown.compareAndSet(false, true)) { - log.info(() -> String.format("total parts: %s", partCount)); - totalParts.set(partCount); - totalPartKnown.set(true); + if (partCount != null && totalParts == null) { + log.trace(() -> String.format("total parts: %s", partCount)); + totalParts = partCount; } - if (partCount != null && requiresMultipart()) { - subscription.request(1); - } else { - subscription.cancel(); + synchronized (lock) { + if (totalParts != null && totalParts > 1 && totalComplete < totalParts) { + subscription.request(1); + } else { + subscription.cancel(); + } } }); } - private boolean requiresMultipart() { - return totalParts.get() > 1; - } - @Override public void onError(Throwable t) { future.completeExceptionally(t); @@ -138,4 +144,13 @@ public void onError(Throwable t) { public void onComplete() { future.complete(null); } + + private GetObjectRequest nextRequest(int nextPartToGet) { + return getObjectRequest.copy(req -> { + req.partNumber(nextPartToGet); + if (eTag != null) { + req.ifMatch(eTag); + } + }); + } } 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/MultipartDownloaderSubscriberWiremockTest.java index eadef03005ee..bbc65a9c98b7 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/MultipartDownloaderSubscriberWiremockTest.java @@ -38,9 +38,9 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Random; import java.util.UUID; import java.util.concurrent.CountDownLatch; -import java.util.function.Function; import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.params.ParameterizedTest; @@ -60,14 +60,19 @@ 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.testutils.InputStreamUtils; +import software.amazon.awssdk.services.s3.utils.AsyncResponseTransformerTestSupplier; +import software.amazon.awssdk.utils.IoUtils; +import software.amazon.awssdk.utils.Pair; @WireMockTest class MultipartDownloaderSubscriberWiremockTest { + private final String testBucket = "test-bucket"; + private final String testKey = "test-key"; + private S3AsyncClient s3AsyncClient; - private String testBucket = "test-bucket"; - private String testKey = "test-key"; + private Random random; + private String eTag; @BeforeEach public void init(WireMockRuntimeInfo wiremock) { @@ -80,209 +85,224 @@ public void init(WireMockRuntimeInfo wiremock) { .pathStyleAccessEnabled(true) .build()) .build(); + random = new Random(); + eTag = UUID.randomUUID().toString(); } @ParameterizedTest - @MethodSource("partsParamProvider") - void happyPath_ByteArrayAsyncResponseTransformer(int amountOfPartToTest, int partSize) { - for (int i = 1; i <= amountOfPartToTest; i++) { - stubForPart(i, amountOfPartToTest, partSize); + @MethodSource("argumentsProvider") + void happyPath_allAsyncResponseTransformer(AsyncResponseTransformerTestSupplier supplier, + int amountOfPartToTest, + int partSize) { + byte[] expectedBody = new byte[amountOfPartToTest * partSize]; + for (int i = 0; i < amountOfPartToTest; i++) { + byte[] individualBody = stubForPart(i + 1, amountOfPartToTest, partSize); + System.arraycopy(individualBody, 0, expectedBody, i * partSize, individualBody.length); + } + + AsyncResponseTransformer transformer; + if (supplier.requiresJimfs()) { + FileSystem jimfs = Jimfs.newFileSystem(); + String filePath = "/tmp-file-" + UUID.randomUUID(); + Path inMemoryFilePath = jimfs.getPath(filePath); + transformer = supplier.transformer(inMemoryFilePath); + } else { + transformer = supplier.transformer(null); } - MultipartDownloaderSubscriber subscriber = new MultipartDownloaderSubscriber( + SplitAsyncResponseTransformer split = transformer.split(1024 * 1024 * 64); + Subscriber> subscriber = new MultipartDownloaderSubscriber( s3AsyncClient, GetObjectRequest.builder() .bucket(testBucket) .key(testKey) .build()); - SplitAsyncResponseTransformer> split = - AsyncResponseTransformer.toBytes().split(1024 * 16); - split.publisher().subscribe(subscriber); - ResponseBytes responseBytes = split.preparedFuture().join(); - - byte[] fullBody = responseBytes.asByteArray(); - byte[] expectedBody = expectedBody(amountOfPartToTest, partSize); - assertArrayEquals(expectedBody, fullBody); + T response = split.preparedFuture().join(); + byte[] body = supplier.body(response); + assertArrayEquals(expectedBody, body); verifyCorrectAmountOfRequestsMade(amountOfPartToTest); } - @ParameterizedTest - @MethodSource("partsParamProvider") - void happyPath_InputStreamResponseTransformer(int amountOfPartToTest, int partSize) { - for (int i = 1; i <= amountOfPartToTest; i++) { - stubForPart(i, amountOfPartToTest, partSize); - } - - MultipartDownloaderSubscriber subscriber = new MultipartDownloaderSubscriber( - s3AsyncClient, - GetObjectRequest.builder() - .bucket(testBucket) - .key(testKey) - .build()); - - SplitAsyncResponseTransformer> split = - AsyncResponseTransformer.toBlockingInputStream().split(1024 * 16); - - split.publisher().subscribe(subscriber); - ResponseInputStream responseInputStream = split.preparedFuture().join(); + private byte[] stubForPart(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))).willReturn( + aResponse() + .withHeader("x-amz-mp-parts-count", totalPart + "") + .withHeader("ETag", eTag) + .withBody(body))); + return body; + } - byte[] fullBody = InputStreamUtils.drainInputStream(responseInputStream); - byte[] expectedBody = expectedBody(amountOfPartToTest, partSize); - assertArrayEquals(expectedBody, fullBody); + private static List> transformersSuppliers() { + return Arrays.asList( + new ByteTestArtSupplier(), + new InputStreamArtSupplier(), + new PublisherArtSupplier(), + new FileArtSupplier() + ); + } - verifyCorrectAmountOfRequestsMade(amountOfPartToTest); + 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(16, 16 * 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(); } - @ParameterizedTest - @MethodSource("partsParamProvider") - void happyPath_PublisherAsyncResponseTransformer(int amountOfPartToTest, int partSize) { + + private void verifyCorrectAmountOfRequestsMade(int amountOfPartToTest) { + String urlTemplate = ".*partNumber=%d.*"; for (int i = 1; i <= amountOfPartToTest; i++) { - stubForPart(i, amountOfPartToTest, partSize); + verify(getRequestedFor(urlMatching(String.format(urlTemplate, i)))); } + verify(0, getRequestedFor(urlMatching(String.format(urlTemplate, amountOfPartToTest + 1)))); + } - MultipartDownloaderSubscriber subscriber = new MultipartDownloaderSubscriber( - s3AsyncClient, - GetObjectRequest.builder() - .bucket(testBucket) - .key(testKey) - .build()); - - SplitAsyncResponseTransformer> split = - AsyncResponseTransformer.toPublisher().split(1024 * 16); + private static byte[] unbox(Byte[] arr) { + byte[] bb = new byte[arr.length]; + int i = 0; + for (Byte b : arr) { + bb[i] = b; + i++; + } + return bb; + } - split.publisher().subscribe(subscriber); - ResponsePublisher responsePublisher = split.preparedFuture().join(); - - List bodyBytes = new ArrayList<>(); - CountDownLatch latch = new CountDownLatch(1); - responsePublisher.subscribe(new Subscriber() { - Subscription s; - @Override - public void onSubscribe(Subscription s) { - this.s = s; - s.request(1); - } + private static class ByteTestArtSupplier implements AsyncResponseTransformerTestSupplier> { + @Override + public byte[] body(ResponseBytes response) { + return response.asByteArray(); + } - @Override - public void onNext(ByteBuffer byteBuffer) { - while (byteBuffer.remaining() > 0) { - bodyBytes.add(byteBuffer.get()); - } - s.request(1); - } + @Override + public AsyncResponseTransformer> transformer(Path elem) { + return AsyncResponseTransformer.toBytes(); + } - @Override - public void onError(Throwable t) { - latch.countDown(); - fail("Unexpected onError during test", t); - } + @Override + public String toString() { + return "AsyncResponseTransformer.toBytes"; + } + } - @Override - public void onComplete() { - latch.countDown(); + private static class InputStreamArtSupplier + implements AsyncResponseTransformerTestSupplier> { + @Override + public byte[] body(ResponseInputStream response) { + try { + return IoUtils.toByteArray(response); + } catch (IOException ioe) { + fail("unexpected IOE during test", ioe); + return null; } - }); - - try { - latch.await(); - } catch (InterruptedException e) { - fail("Unexpected thread interruption during test", e); } - byte[] fullBody = unbox(bodyBytes.toArray(new Byte[0])); - byte[] expectedBody = expectedBody(amountOfPartToTest, partSize); - assertArrayEquals(expectedBody, fullBody); + @Override + public AsyncResponseTransformer> transformer(Path elem) { + return AsyncResponseTransformer.toBlockingInputStream(); + } - verifyCorrectAmountOfRequestsMade(amountOfPartToTest); + @Override + public String toString() { + return "AsyncResponseTransformer.toBlockingInputStream"; + } } - @ParameterizedTest - @MethodSource("partsParamProvider") - void happyPath_FileAsyncResponseTransformer(int amountOfPartToTest, int partSize) { - for (int i = 1; i <= amountOfPartToTest; i++) { - stubForPart(i, amountOfPartToTest, partSize); - } + private static class FileArtSupplier implements AsyncResponseTransformerTestSupplier { + private Path path; - MultipartDownloaderSubscriber subscriber = new MultipartDownloaderSubscriber( - s3AsyncClient, - GetObjectRequest.builder() - .bucket(testBucket) - .key(testKey) - .build()); - - FileSystem fs = Jimfs.newFileSystem(); - String filePath = "/tmp-file-" + UUID.randomUUID(); - Path inMemoryFilePath = fs.getPath(filePath); - SplitAsyncResponseTransformer split = - AsyncResponseTransformer.toFile(inMemoryFilePath).split(1024 * 16); + @Override + public byte[] body(GetObjectResponse response) { + try { + return Files.readAllBytes(path); + } catch (IOException ioe) { + fail("unexpected IOE during test", ioe); + return new byte[0]; + } + } - split.publisher().subscribe(subscriber); - GetObjectResponse response = split.preparedFuture().join(); - - Path savedFile = fs.getPath(filePath); - byte[] expectedBody = expectedBody(amountOfPartToTest, partSize); - byte[] fullBody = null; - try { - fullBody = Files.readAllBytes(savedFile); - } catch (IOException e) { - fail("Unexpected IOException during test", e); + @Override + public AsyncResponseTransformer transformer(Path path) { + this.path = path; + return AsyncResponseTransformer.toFile(path); } - assertArrayEquals(expectedBody, fullBody); - verifyCorrectAmountOfRequestsMade(amountOfPartToTest); + @Override + public String toString() { + return "AsyncResponseTransformer.toFile"; + } + @Override + public boolean requiresJimfs() { + return true; + } } - private static Stream partsParamProvider() { - return Stream.of( - arguments(4, 16), - arguments(1, 1024), - arguments(31, 1243) - ); - } + private static class PublisherArtSupplier implements AsyncResponseTransformerTestSupplier> { + @Override + public byte[] body(ResponsePublisher response) { + List buffer = new ArrayList<>(); + CountDownLatch latch = new CountDownLatch(1); + response.subscribe(new Subscriber() { + Subscription s; + + @Override + public void onSubscribe(Subscription s) { + this.s = s; + s.request(1); + } - private void verifyCorrectAmountOfRequestsMade(int amountOfPartToTest) { - String urlTemplate = ".*partNumber=%d.*"; - for (int i = 1; i <= amountOfPartToTest; i++) { - verify(getRequestedFor(urlMatching(String.format(urlTemplate, i)))); - } - verify(0, getRequestedFor(urlMatching(String.format(urlTemplate, amountOfPartToTest + 1)))); - } + @Override + public void onNext(ByteBuffer byteBuffer) { + while (byteBuffer.remaining() > 0) { + buffer.add(byteBuffer.get()); + } + s.request(1); + } - private void stubForPart(int part, int totalPart, int partSize) { - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=%d", testBucket, testKey, part))).willReturn( - aResponse() - .withHeader("x-amz-mp-parts-count", totalPart + "") - .withBody(byteArrayOfLength(partSize, (byte) part)))); - } + @Override + public void onError(Throwable t) { + latch.countDown(); + fail("Unexpected onError during test", t); + } - private byte[] expectedBody(int totalPart, int partSize) { - Stream.Builder> s = Stream.builder(); - for (int i = 1; i <= totalPart; i++) { - int j = i; - s.add(Stream.generate(() -> (byte) j).limit(partSize)); + @Override + public void onComplete() { + latch.countDown(); + } + }); + try { + latch.await(); + } catch (InterruptedException e) { + fail("Unexpected thread interruption during test", e); + } + return unbox(buffer.toArray(new Byte[0])); } - return unbox(s.build().flatMap(Function.identity()).toArray(Byte[]::new)); - } - private byte[] unbox(Byte[] arr) { - byte[] bb = new byte[arr.length]; - int i = 0; - for (Byte b : arr) { - bb[i] = b.byteValue(); - i++; + @Override + public AsyncResponseTransformer> transformer(Path elem) { + return AsyncResponseTransformer.toPublisher(); } - return bb; - } - private byte[] byteArrayOfLength(int length, byte value) { - byte[] b = new byte[length]; - Arrays.fill(b, value); - return b; + @Override + public String toString() { + return "AsyncResponseTransformer.toPublisher"; + } } - } \ No newline at end of file diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java new file mode 100644 index 000000000000..ef8ebe0bae05 --- /dev/null +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java @@ -0,0 +1,55 @@ +/* + * 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.utils; + +import java.nio.file.FileSystem; +import java.nio.file.Path; +import software.amazon.awssdk.core.async.AsyncResponseTransformer; +import software.amazon.awssdk.core.internal.async.FileAsyncResponseTransformer; +import software.amazon.awssdk.services.s3.model.GetObjectResponse; + +/** + * Contains the {@link AsyncResponseTransformer} to be used in a test as well as logic on how to + * retrieve the body content of the request for that specific transformer. + * @param the type returned of the future associated with the {@link AsyncResponseTransformer} + */ +public interface AsyncResponseTransformerTestSupplier { + + /** + * Call this method to retrieve the AsyncResponseTransformer required to perform the test + * @param path + * @return + */ + AsyncResponseTransformer transformer(Path path); + + /** + * Implementation of this method whould retreive the whole body of the request made using the AsyncResponseTransformer + * as a byte array. + * @param response the response the {@link AsyncResponseTransformerTestSupplier#transformer} + * @return + */ + byte[] body(T response); + + /** + * Sonce {@link FileAsyncResponseTransformer} works with file, some test might need to initialize an in-memory + * {@link FileSystem} with jimfs. + * @return true if the test using this class requires setup with jimfs + */ + default boolean requiresJimfs() { + return false; + } + +} diff --git a/utils/src/main/java/software/amazon/awssdk/utils/async/BaseSubscriberAdapter.java b/utils/src/main/java/software/amazon/awssdk/utils/async/BaseSubscriberAdapter.java index 6b434d2d162d..9b86f19d0f7e 100644 --- a/utils/src/main/java/software/amazon/awssdk/utils/async/BaseSubscriberAdapter.java +++ b/utils/src/main/java/software/amazon/awssdk/utils/async/BaseSubscriberAdapter.java @@ -24,8 +24,8 @@ import software.amazon.awssdk.utils.Logger; /** - * Base of subscribers that can adapt one type to another. Thess subscriber will receive onNext signal with the {@code U} type, - * but will need to {@link BaseSubscriberAdapter#fulfillDownstreamDemand() fulfill the dpwnstream demand} of the delegate + * Base of subscribers that can adapt one type to another. This subscriber will receive onNext signal with the {@code U} type, + * but will need to {@link BaseSubscriberAdapter#fulfillDownstreamDemand() fulfill the downstream demand} of the delegate * subscriber with instance of the {@code T} type. * * @param the type that the delegate subscriber demands. @@ -161,7 +161,7 @@ private void addDownstreamDemand(long l) { /** * This is invoked after each downstream request or upstream onNext, onError or onComplete. */ - private void handleStateUpdate() { + protected void handleStateUpdate() { do { // Anything that happens after this if statement and before we set handlingStateUpdate to false is guaranteed to only // happen on one thread. For that reason, we should only invoke onNext, onComplete or onError within that block. @@ -194,7 +194,11 @@ private void handleStateUpdate() { // It's possible we had an important state change between when we decided to release the state update flag, and we // actually released it. If that seems to have happened, try to handle that state change on this thread, because // another thread is not guaranteed to come around and do so. - } while (onNextNeeded() || upstreamDemandNeeded() || onCompleteNeeded() || onErrorNeeded()); + } while (onNextNeeded() || upstreamDemandNeeded() || onCompleteNeeded() || onErrorNeeded() || additionalStateCheck()); + } + + boolean additionalStateCheck() { + return false; } /** diff --git a/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java b/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java index c89c95d8e35e..9e4ce14bb1c6 100644 --- a/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java +++ b/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java @@ -19,7 +19,6 @@ import static software.amazon.awssdk.utils.async.StoringSubscriber.EventType.ON_NEXT; import java.nio.ByteBuffer; -import java.util.Optional; import java.util.concurrent.atomic.AtomicLong; import org.reactivestreams.Subscriber; import org.reactivestreams.Subscription; @@ -51,9 +50,7 @@ protected DelegatingBufferingSubscriber(Long maximumBufferInBytes, Subscriber 0) { - flushStorageToDelegate(); - } + handleStateUpdate(); super.onNext(item); } @@ -71,7 +68,13 @@ void doWithItem(ByteBuffer buffer) { @Override protected void fulfillDownstreamDemand() { - flushStorageToDelegate(); + storage.poll() + .filter(event -> event.type() == ON_NEXT) + .ifPresent(byteBufferEvent -> { + currentlyBuffered.addAndGet(-byteBufferEvent.value().remaining()); + downstreamDemand.decrementAndGet(); + subscriber.onNext(byteBufferEvent.value()); + }); } /** @@ -95,23 +98,12 @@ boolean additionalOnCompleteNeededCheck() { */ @Override boolean additionalUpstreamDemandNeededCheck() { - return currentlyBuffered.get() < maximumBufferInBytes; + return currentlyBuffered.get() <= maximumBufferInBytes; } - private void flushStorageToDelegate() { - long totalBufferRemaining = currentlyBuffered.get(); - Optional> next = storage.poll(); - while (totalBufferRemaining > 0 && downstreamDemand.get() > 0) { - if (!next.isPresent() || next.get().type() != ON_NEXT) { - break; - } - StoringSubscriber.Event byteBufferEvent = next.get(); - totalBufferRemaining -= byteBufferEvent.value().remaining(); - currentlyBuffered.addAndGet(-byteBufferEvent.value().remaining()); - subscriber.onNext(byteBufferEvent.value()); - next = storage.poll(); - downstreamDemand.decrementAndGet(); - } + @Override + boolean additionalStateCheck() { + return downstreamDemand.get() == 0 && currentlyBuffered.get() >= maximumBufferInBytes; } public static Builder builder() { From 5b381e3f8634c967701eac4e787cc8432d2a9aa4 Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Mon, 19 Feb 2024 10:06:10 -0500 Subject: [PATCH 04/15] test for missing byte bug --- .../MultipartDownloadIntegrationTest.java | 91 +++++------ ...ipartDownloaderSubscriberWiremockTest.java | 152 +----------------- .../AsyncResponseTransformerTestSupplier.java | 150 ++++++++++++++++- .../utils/async/BaseSubscriberAdapter.java | 6 +- .../async/DelegatingBufferingSubscriber.java | 13 +- 5 files changed, 201 insertions(+), 211 deletions(-) diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadIntegrationTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadIntegrationTest.java index b95c04036fd5..1122f93676ab 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadIntegrationTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadIntegrationTest.java @@ -23,6 +23,15 @@ import java.nio.ByteBuffer; import java.nio.file.Path; import java.nio.file.Paths; +import java.time.LocalDateTime; +import java.time.LocalTime; +import java.time.format.DateTimeFormatter; +import java.util.Random; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicInteger; +import org.apache.commons.lang3.RandomUtils; +import org.checkerframework.checker.units.qual.A; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.reactivestreams.Subscriber; @@ -54,56 +63,48 @@ class MultipartDownloadIntegrationTest { void init() { this.s3 = S3AsyncClient.builder() .region(Region.US_WEST_2) + .multipartEnabled(true) .credentialsProvider(ProfileCredentialsProvider.create()) .httpClient(NettyNioAsyncHttpClient.create()) .build(); } - @Test + // @Test void testByteAsyncResponseTransformer() { - AsyncResponseTransformer> transformer = - AsyncResponseTransformer.toBytes(); - MultipartDownloaderSubscriber downloaderSubscriber = new MultipartDownloaderSubscriber( - s3, GetObjectRequest.builder().bucket(bucket).key(key).build()); - - SplitAsyncResponseTransformer> split = - transformer.split(1024 * 1024 * 32); - split.publisher().subscribe(downloaderSubscriber); - ResponseBytes res = split.preparedFuture().join(); + CompletableFuture> response = s3.getObject( + r -> r.bucket(bucket).key(key), + AsyncResponseTransformer.toBytes()); + ResponseBytes res = response.join(); log.info(() -> "complete"); byte[] bytes = res.asByteArray(); log.info(() -> String.format("Byte len: %s", bytes.length)); - assertThat(bytes.length).isEqualTo(fileTestSize * 1024 * 1024); + assertThat(bytes).hasSize(fileTestSize * 1024 * 1024); } - @Test + // @Test void testFileAsyncResponseTransformer() { - Path path = Paths.get("/Users/olapplin/Develop/tmp/" + key); - AsyncResponseTransformer transformer = - AsyncResponseTransformer.toFile(path); - - MultipartDownloaderSubscriber downloaderSubscriber = new MultipartDownloaderSubscriber( - s3, GetObjectRequest.builder().bucket(bucket).key(key).build()); - - SplitAsyncResponseTransformer split = transformer.split(1024 * 1024 * 32); - split.publisher().subscribe(downloaderSubscriber); - GetObjectResponse res = split.preparedFuture().join(); + Path path = Paths.get("/Users/olapplin/Develop/tmp", + LocalDateTime.now().format(DateTimeFormatter.BASIC_ISO_DATE) + '-' + key); + CompletableFuture future = s3.getObject( + r -> r.bucket(bucket).key(key), + AsyncResponseTransformer.toFile(path)); + GetObjectResponse res = future.join(); log.info(() -> "complete"); assertTrue(path.toFile().exists()); - assertThat(path.toFile().length()).isEqualTo(fileTestSize * 1024 * 1024); + assertThat(path.toFile()).hasSize(fileTestSize * 1024 * 1024); } // @Test void testPublisherAsyncResponseTransformer() { + CompletableFuture> future = s3.getObject( + r -> r.bucket(bucket).key(key), + AsyncResponseTransformer.toPublisher()); AsyncResponseTransformer> transformer = AsyncResponseTransformer.toPublisher(); - MultipartDownloaderSubscriber downloaderSubscriber = new MultipartDownloaderSubscriber( - s3, GetObjectRequest.builder().bucket(bucket).key(key).build()); - SplitAsyncResponseTransformer> split = - transformer.split(1024 * 1024 * 32); - split.publisher().subscribe(downloaderSubscriber); - split.preparedFuture().whenComplete((res, e) -> { + CountDownLatch latch = new CountDownLatch(1); + AtomicInteger total = new AtomicInteger(0); + future.whenComplete((res, e) -> { log.info(() -> "complete"); res.subscribe(new Subscriber() { Subscription subscription; @@ -111,41 +112,41 @@ void testPublisherAsyncResponseTransformer() { @Override public void onSubscribe(Subscription s) { this.subscription = s; - s.request(Long.MAX_VALUE); + s.request(1); } @Override public void onNext(ByteBuffer byteBuffer) { - log.info(() -> "received " + byteBuffer.remaining()); - subscription.request(Long.MAX_VALUE); + total.addAndGet(byteBuffer.remaining()); + subscription.request(1); } @Override public void onError(Throwable t) { - + fail("unexpected error in test", t); + latch.countDown(); } @Override public void onComplete() { - + latch.countDown(); } }); }); - split.preparedFuture().join(); + try { + latch.await(); + } catch (InterruptedException e) { + throw new RuntimeException(e); + } + assertThat(total).hasValue(fileTestSize * 1024 * 1024); } // @Test void testBlockingInputStreamResponseTransformer() { - AsyncResponseTransformer> transformer = - AsyncResponseTransformer.toBlockingInputStream(); - - MultipartDownloaderSubscriber downloaderSubscriber = new MultipartDownloaderSubscriber( - s3, GetObjectRequest.builder().bucket(bucket).key(key).build()); - - SplitAsyncResponseTransformer> split = - transformer.split(1024 * 1024 * 32); - split.publisher().subscribe(downloaderSubscriber); - ResponseInputStream res = split.preparedFuture().join(); + CompletableFuture> future = s3.getObject( + r -> r.bucket(bucket).key(key), + AsyncResponseTransformer.toBlockingInputStream()); + ResponseInputStream res = future.join(); log.info(() -> "complete"); int total = 0; try { 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/MultipartDownloaderSubscriberWiremockTest.java index bbc65a9c98b7..1676755be129 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/MultipartDownloaderSubscriberWiremockTest.java @@ -23,37 +23,27 @@ import static com.github.tomakehurst.wiremock.client.WireMock.urlMatching; import static com.github.tomakehurst.wiremock.client.WireMock.verify; import static org.junit.jupiter.api.Assertions.assertArrayEquals; -import static org.junit.jupiter.api.Assertions.fail; import static org.junit.jupiter.params.provider.Arguments.arguments; import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; import com.github.tomakehurst.wiremock.junit5.WireMockTest; import com.google.common.jimfs.Jimfs; -import java.io.IOException; import java.net.URI; -import java.nio.ByteBuffer; import java.nio.file.FileSystem; -import java.nio.file.Files; import java.nio.file.Path; -import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Random; import java.util.UUID; -import java.util.concurrent.CountDownLatch; import java.util.stream.Stream; 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 org.reactivestreams.Subscription; import software.amazon.awssdk.auth.credentials.AwsBasicCredentials; import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider; -import software.amazon.awssdk.core.ResponseBytes; -import software.amazon.awssdk.core.ResponseInputStream; import software.amazon.awssdk.core.async.AsyncResponseTransformer; -import software.amazon.awssdk.core.async.ResponsePublisher; import software.amazon.awssdk.core.async.SplitAsyncResponseTransformer; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.s3.S3AsyncClient; @@ -61,7 +51,6 @@ 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.IoUtils; import software.amazon.awssdk.utils.Pair; @WireMockTest @@ -139,10 +128,10 @@ private byte[] stubForPart(int part, int totalPart, int partSize) { private static List> transformersSuppliers() { return Arrays.asList( - new ByteTestArtSupplier(), - new InputStreamArtSupplier(), - new PublisherArtSupplier(), - new FileArtSupplier() + new AsyncResponseTransformerTestSupplier.ByteTestArtSupplier(), + new AsyncResponseTransformerTestSupplier.InputStreamArtSupplier(), + new AsyncResponseTransformerTestSupplier.PublisherArtSupplier(), + new AsyncResponseTransformerTestSupplier.FileArtSupplier() ); } @@ -165,7 +154,6 @@ private static Stream argumentsProvider() { return sb.build(); } - private void verifyCorrectAmountOfRequestsMade(int amountOfPartToTest) { String urlTemplate = ".*partNumber=%d.*"; for (int i = 1; i <= amountOfPartToTest; i++) { @@ -173,136 +161,4 @@ private void verifyCorrectAmountOfRequestsMade(int amountOfPartToTest) { } verify(0, getRequestedFor(urlMatching(String.format(urlTemplate, amountOfPartToTest + 1)))); } - - private static byte[] unbox(Byte[] arr) { - byte[] bb = new byte[arr.length]; - int i = 0; - for (Byte b : arr) { - bb[i] = b; - i++; - } - return bb; - } - - private static class ByteTestArtSupplier implements AsyncResponseTransformerTestSupplier> { - @Override - public byte[] body(ResponseBytes response) { - return response.asByteArray(); - } - - @Override - public AsyncResponseTransformer> transformer(Path elem) { - return AsyncResponseTransformer.toBytes(); - } - - @Override - public String toString() { - return "AsyncResponseTransformer.toBytes"; - } - } - - private static class InputStreamArtSupplier - implements AsyncResponseTransformerTestSupplier> { - @Override - public byte[] body(ResponseInputStream response) { - try { - return IoUtils.toByteArray(response); - } catch (IOException ioe) { - fail("unexpected IOE during test", ioe); - return null; - } - } - - @Override - public AsyncResponseTransformer> transformer(Path elem) { - return AsyncResponseTransformer.toBlockingInputStream(); - } - - @Override - public String toString() { - return "AsyncResponseTransformer.toBlockingInputStream"; - } - } - - private static class FileArtSupplier implements AsyncResponseTransformerTestSupplier { - private Path path; - - @Override - public byte[] body(GetObjectResponse response) { - try { - return Files.readAllBytes(path); - } catch (IOException ioe) { - fail("unexpected IOE during test", ioe); - return new byte[0]; - } - } - - @Override - public AsyncResponseTransformer transformer(Path path) { - this.path = path; - return AsyncResponseTransformer.toFile(path); - } - - @Override - public String toString() { - return "AsyncResponseTransformer.toFile"; - } - - @Override - public boolean requiresJimfs() { - return true; - } - } - - private static class PublisherArtSupplier implements AsyncResponseTransformerTestSupplier> { - @Override - public byte[] body(ResponsePublisher response) { - List buffer = new ArrayList<>(); - CountDownLatch latch = new CountDownLatch(1); - response.subscribe(new Subscriber() { - Subscription s; - - @Override - public void onSubscribe(Subscription s) { - this.s = s; - s.request(1); - } - - @Override - public void onNext(ByteBuffer byteBuffer) { - while (byteBuffer.remaining() > 0) { - buffer.add(byteBuffer.get()); - } - s.request(1); - } - - @Override - public void onError(Throwable t) { - latch.countDown(); - fail("Unexpected onError during test", t); - } - - @Override - public void onComplete() { - latch.countDown(); - } - }); - try { - latch.await(); - } catch (InterruptedException e) { - fail("Unexpected thread interruption during test", e); - } - return unbox(buffer.toArray(new Byte[0])); - } - - @Override - public AsyncResponseTransformer> transformer(Path elem) { - return AsyncResponseTransformer.toPublisher(); - } - - @Override - public String toString() { - return "AsyncResponseTransformer.toPublisher"; - } - } } \ No newline at end of file diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java index ef8ebe0bae05..9e1d03a896ab 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java @@ -15,11 +15,25 @@ package software.amazon.awssdk.services.s3.utils; +import static org.junit.jupiter.api.Assertions.fail; + +import java.io.IOException; +import java.nio.ByteBuffer; import java.nio.file.FileSystem; +import java.nio.file.Files; import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import org.reactivestreams.Subscriber; +import org.reactivestreams.Subscription; +import software.amazon.awssdk.core.ResponseBytes; +import software.amazon.awssdk.core.ResponseInputStream; import software.amazon.awssdk.core.async.AsyncResponseTransformer; +import software.amazon.awssdk.core.async.ResponsePublisher; import software.amazon.awssdk.core.internal.async.FileAsyncResponseTransformer; import software.amazon.awssdk.services.s3.model.GetObjectResponse; +import software.amazon.awssdk.utils.IoUtils; /** * Contains the {@link AsyncResponseTransformer} to be used in a test as well as logic on how to @@ -28,6 +42,141 @@ */ public interface AsyncResponseTransformerTestSupplier { + class ByteTestArtSupplier implements AsyncResponseTransformerTestSupplier> { + + @Override + public byte[] body(ResponseBytes response) { + return response.asByteArray(); + } + + @Override + public AsyncResponseTransformer> transformer(Path elem) { + return AsyncResponseTransformer.toBytes(); + } + + @Override + public String toString() { + return "AsyncResponseTransformer.toBytes"; + } + } + + class InputStreamArtSupplier implements AsyncResponseTransformerTestSupplier> { + + @Override + public byte[] body(ResponseInputStream response) { + try { + return IoUtils.toByteArray(response); + } catch (IOException ioe) { + fail("unexpected IOE during test", ioe); + return null; + } + } + + @Override + public AsyncResponseTransformer> transformer(Path elem) { + return AsyncResponseTransformer.toBlockingInputStream(); + } + + @Override + public String toString() { + return "AsyncResponseTransformer.toBlockingInputStream"; + } + } + + class FileArtSupplier implements AsyncResponseTransformerTestSupplier { + + private Path path; + + @Override + public byte[] body(GetObjectResponse response) { + try { + return Files.readAllBytes(path); + } catch (IOException ioe) { + fail("unexpected IOE during test", ioe); + return new byte[0]; + } + } + + @Override + public AsyncResponseTransformer transformer(Path path) { + this.path = path; + return AsyncResponseTransformer.toFile(path); + } + + @Override + public String toString() { + return "AsyncResponseTransformer.toFile"; + } + + @Override + public boolean requiresJimfs() { + return true; + } + } + + class PublisherArtSupplier implements AsyncResponseTransformerTestSupplier> { + + @Override + public byte[] body(ResponsePublisher response) { + List buffer = new ArrayList<>(); + CountDownLatch latch = new CountDownLatch(1); + response.subscribe(new Subscriber() { + Subscription s; + + @Override + public void onSubscribe(Subscription s) { + this.s = s; + s.request(1); + } + + @Override + public void onNext(ByteBuffer byteBuffer) { + while (byteBuffer.remaining() > 0) { + buffer.add(byteBuffer.get()); + } + s.request(1); + } + + @Override + public void onError(Throwable t) { + latch.countDown(); + fail("Unexpected onError during test", t); + } + + @Override + public void onComplete() { + latch.countDown(); + } + }); + try { + latch.await(); + } catch (InterruptedException e) { + fail("Unexpected thread interruption during test", e); + } + return unbox(buffer.toArray(new Byte[0])); + } + + private byte[] unbox(Byte[] arr) { + byte[] bb = new byte[arr.length]; + int i = 0; + for (Byte b : arr) { + bb[i] = b; + i++; + } + return bb; + } + + @Override + public AsyncResponseTransformer> transformer(Path elem) { + return AsyncResponseTransformer.toPublisher(); + } + + @Override + public String toString() { + return "AsyncResponseTransformer.toPublisher"; + } + } + /** * Call this method to retrieve the AsyncResponseTransformer required to perform the test * @param path @@ -51,5 +200,4 @@ public interface AsyncResponseTransformerTestSupplier { default boolean requiresJimfs() { return false; } - } diff --git a/utils/src/main/java/software/amazon/awssdk/utils/async/BaseSubscriberAdapter.java b/utils/src/main/java/software/amazon/awssdk/utils/async/BaseSubscriberAdapter.java index 9b86f19d0f7e..ea8acfb9d594 100644 --- a/utils/src/main/java/software/amazon/awssdk/utils/async/BaseSubscriberAdapter.java +++ b/utils/src/main/java/software/amazon/awssdk/utils/async/BaseSubscriberAdapter.java @@ -194,11 +194,7 @@ protected void handleStateUpdate() { // It's possible we had an important state change between when we decided to release the state update flag, and we // actually released it. If that seems to have happened, try to handle that state change on this thread, because // another thread is not guaranteed to come around and do so. - } while (onNextNeeded() || upstreamDemandNeeded() || onCompleteNeeded() || onErrorNeeded() || additionalStateCheck()); - } - - boolean additionalStateCheck() { - return false; + } while (onNextNeeded() || upstreamDemandNeeded() || onCompleteNeeded() || onErrorNeeded()); } /** diff --git a/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java b/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java index 9e4ce14bb1c6..c2341698ed28 100644 --- a/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java +++ b/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java @@ -48,12 +48,6 @@ protected DelegatingBufferingSubscriber(Long maximumBufferInBytes, Subscriber= maximumBufferInBytes; + return true; } public static Builder builder() { From 9fe08cc1cdb0d38d9654334f420a9ae6d75440bb Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Mon, 19 Feb 2024 10:26:37 -0500 Subject: [PATCH 05/15] test for missing byte bug --- .../awssdk/utils/async/DelegatingBufferingSubscriber.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java b/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java index c2341698ed28..509c7dee7da6 100644 --- a/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java +++ b/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java @@ -23,10 +23,13 @@ import org.reactivestreams.Subscriber; import org.reactivestreams.Subscription; import software.amazon.awssdk.annotations.SdkProtectedApi; +import software.amazon.awssdk.utils.Logger; import software.amazon.awssdk.utils.Validate; @SdkProtectedApi public class DelegatingBufferingSubscriber extends BaseSubscriberAdapter { + private static final Logger log = Logger.loggerFor(DelegatingBufferingSubscriber.class); + /** * The maximum amount of bytes allowed to be stored in the StoringSubscriber */ @@ -45,7 +48,7 @@ public class DelegatingBufferingSubscriber extends BaseSubscriberAdapter delegate) { super(Validate.notNull(delegate, "delegate must not be null")); - this.maximumBufferInBytes = Validate.notNull(maximumBufferInBytes, "maximumBufferInBytes msut not be null"); + this.maximumBufferInBytes = Validate.notNull(maximumBufferInBytes, "maximumBufferInBytes must not be null"); } @Override @@ -67,6 +70,7 @@ protected void fulfillDownstreamDemand() { .ifPresent(byteBufferEvent -> { currentlyBuffered.addAndGet(-byteBufferEvent.value().remaining()); downstreamDemand.decrementAndGet(); + log.trace(() -> "demand: " + downstreamDemand.get()); subscriber.onNext(byteBufferEvent.value()); }); } From 04ebe165b1cf110b2a785fc43484b6fa5dd10f4e Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Mon, 19 Feb 2024 12:30:37 -0500 Subject: [PATCH 06/15] fix merge --- .../amazon/awssdk/core/async/AsyncResponseTransformer.java | 2 +- .../awssdk/core/internal/async/SplittingTransformer.java | 4 ++-- .../MultipartDownloaderSubscriberWiremockTest.java | 6 +++++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java index 80217ec40d28..b8554cab8211 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/async/AsyncResponseTransformer.java @@ -129,7 +129,7 @@ default SplitAsyncResponseTransformer split(SplittingTransfo SdkPublisher> transformer = SplittingTransformer .builder() .upstreamResponseTransformer(this) - .maximumBufferSize(splitConfig.bufferSize()) + .maximumBufferSizeInBytes(splitConfig.bufferSize()) .returnFuture(future) .build(); return SplitAsyncResponseTransformer.builder() diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java index dd1220c47ad5..42d236612c59 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java @@ -328,8 +328,8 @@ private Builder() { /** * The {@link AsyncResponseTransformer} that will receive the data from each of the individually published - * {@link IndividualTransformer}, usually intended to be the one on which the {@link AsyncResponseTransformer#split(long)} - * method was called. + * {@link IndividualTransformer}, usually intended to be the one on which the + * {@link AsyncResponseTransformer#split(SplittingTransformerConfiguration)} )} method was called. * * @param upstreamResponseTransformer the {@code AsyncResponseTransformer} that was split. * @return an instance of this builder 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/MultipartDownloaderSubscriberWiremockTest.java index 1676755be129..f1d4a157061e 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/MultipartDownloaderSubscriberWiremockTest.java @@ -43,6 +43,7 @@ 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.async.SplitAsyncResponseTransformer; import software.amazon.awssdk.regions.Region; @@ -99,7 +100,10 @@ void happyPath_allAsyncResponseTransformer(AsyncResponseTransformerTestSuppl transformer = supplier.transformer(null); } - SplitAsyncResponseTransformer split = transformer.split(1024 * 1024 * 64); + SplitAsyncResponseTransformer split = transformer.split( + SplittingTransformerConfiguration.builder() + .bufferSize(1024 * 1024 * 64L) + .build()); Subscriber> subscriber = new MultipartDownloaderSubscriber( s3AsyncClient, GetObjectRequest.builder() From 8663510ae532d706e49c222c59a6fb4c3b42f02c Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Mon, 19 Feb 2024 13:56:21 -0500 Subject: [PATCH 07/15] remove buffering to see if byte length error is still triggered --- .../core/internal/async/SplittingTransformer.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java index 42d236612c59..0555087d2d60 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java @@ -241,10 +241,12 @@ public void onStream(SdkPublisher publisher) { if (onStreamCalled.compareAndSet(false, true)) { log.trace(() -> "calling onStream on the upstream transformer"); upstreamResponseTransformer.onStream(upstreamSubscriber -> publisherToUpstream.subscribe( - DelegatingBufferingSubscriber.builder() - .maximumBufferInBytes(maximumBufferInBytes) - .delegate(upstreamSubscriber) - .build())); + // DelegatingBufferingSubscriber.builder() + // .maximumBufferInBytes(maximumBufferInBytes) + // .delegate(upstreamSubscriber) + // .build()) + upstreamSubscriber + )); } publisher.subscribe(new IndividualPartSubscriber<>(this.individualFuture, response, publisherToUpstream)); } From 09b50eeb318e006db381bcb4b64145c9f68fbc1e Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Wed, 21 Feb 2024 16:33:00 -0500 Subject: [PATCH 08/15] missing byte investigation --- .../internal/async/SplittingTransformer.java | 53 ++++---- .../MultipartDownloaderSubscriber.java | 1 + .../MultipartDownloadIntegrationTest.java | 12 +- ...ipartDownloaderSubscriberWiremockTest.java | 118 +++++++++++++++--- .../AsyncResponseTransformerTestSupplier.java | 31 +++-- 5 files changed, 159 insertions(+), 56 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java index 0555087d2d60..d482bf89d901 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java @@ -25,7 +25,7 @@ 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.CompletableFutureUtils; +import software.amazon.awssdk.core.retry.RetryUtils; import software.amazon.awssdk.utils.Logger; import software.amazon.awssdk.utils.Validate; import software.amazon.awssdk.utils.async.DelegatingBufferingSubscriber; @@ -54,11 +54,6 @@ public class SplittingTransformer implements SdkPublisher upstreamResponseTransformer; - /** - * Set to true once {@code .prepare()} is called on the upstreamResponseTransformer - */ - private final AtomicBoolean preparedCalled = new AtomicBoolean(false); - /** * Set to true once {@code .onResponse()} is called on the upstreamResponseTransformer */ @@ -191,12 +186,14 @@ private boolean doEmit() { return false; } - private synchronized void handleCancelState() { - if (downstreamSubscriber == null) { - return; + private void handleCancelState() { + synchronized (this) { + if (downstreamSubscriber == null) { + return; + } + publisherToUpstream.complete(); + downstreamSubscriber = null; } - publisherToUpstream.complete(); - downstreamSubscriber = null; } /** @@ -205,19 +202,21 @@ private synchronized void handleCancelState() { * body publisher. */ private class IndividualTransformer implements AsyncResponseTransformer { - + private final Logger trLog = Logger.loggerFor(IndividualTransformer.class); private ResponseT response; private CompletableFuture individualFuture; @Override public CompletableFuture prepare() { + trLog.info(() -> "prepare"); this.individualFuture = new CompletableFuture<>(); - if (preparedCalled.compareAndSet(false, true)) { - log.trace(() -> "calling prepare on the upstream transformer"); - CompletableFuture upstreamFuture = upstreamResponseTransformer.prepare(); - CompletableFutureUtils.forwardExceptionTo(returnFuture, upstreamFuture); - CompletableFutureUtils.forwardResultTo(upstreamFuture, returnFuture); - } + CompletableFuture upstreamFuture = upstreamResponseTransformer.prepare(); + upstreamFuture.whenComplete((res, err) -> { + trLog.info(() -> String.format("upstreamFuture completed: res: %s. error: %s", res, err)); + if (res != null) { + returnFuture.complete(res); + } + }); individualFuture.whenComplete((r, e) -> { if (isCancelled.get()) { handleCancelState(); @@ -228,6 +227,7 @@ public CompletableFuture prepare() { @Override public void onResponse(ResponseT response) { + trLog.info(() -> "onResponse"); if (onResponseCalled.compareAndSet(false, true)) { log.trace(() -> "calling onResponse on the upstream transformer"); // todo: should we send back the first response to the upstreamResponseTransformer as-is? @@ -238,14 +238,15 @@ public void onResponse(ResponseT response) { @Override public void onStream(SdkPublisher publisher) { + trLog.info(() -> "onStream"); if (onStreamCalled.compareAndSet(false, true)) { log.trace(() -> "calling onStream on the upstream transformer"); upstreamResponseTransformer.onStream(upstreamSubscriber -> publisherToUpstream.subscribe( - // DelegatingBufferingSubscriber.builder() - // .maximumBufferInBytes(maximumBufferInBytes) - // .delegate(upstreamSubscriber) - // .build()) - upstreamSubscriber + DelegatingBufferingSubscriber.builder() + .maximumBufferInBytes(maximumBufferInBytes) + .delegate(upstreamSubscriber) + .build() + // upstreamSubscriber )); } publisher.subscribe(new IndividualPartSubscriber<>(this.individualFuture, response, publisherToUpstream)); @@ -253,6 +254,7 @@ public void onStream(SdkPublisher publisher) { @Override public void exceptionOccurred(Throwable error) { + trLog.info(() -> "exceptionOccurred"); individualFuture.completeExceptionally(error); upstreamResponseTransformer.exceptionOccurred(error); } @@ -262,6 +264,7 @@ public void exceptionOccurred(Throwable error) { * the Subscriber for each of the individual request's ByteBuffer publisher */ static class IndividualPartSubscriber implements Subscriber { + private static final Logger partLogger = Logger.loggerFor(IndividualPartSubscriber.class); private final CompletableFuture future; private final T response; @@ -277,6 +280,7 @@ static class IndividualPartSubscriber implements Subscriber { @Override public void onSubscribe(Subscription s) { + partLogger.info(() -> "onSubscribe"); if (this.subscription != null) { s.cancel(); return; @@ -290,6 +294,7 @@ public void onNext(ByteBuffer byteBuffer) { if (byteBuffer == null) { throw new NullPointerException("onNext must not be called with null byteBuffer"); } + partLogger.info(() -> "onNext"); publisherToUpstream.send(byteBuffer).whenComplete((r, t) -> { if (t != null) { handleError(t); @@ -301,11 +306,13 @@ public void onNext(ByteBuffer byteBuffer) { @Override public void onError(Throwable t) { + partLogger.info(() -> "onError"); handleError(t); } @Override public void onComplete() { + partLogger.info(() -> "onComplete"); future.complete(response); } diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java index 334a67914eec..5c11efd558da 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java @@ -93,6 +93,7 @@ public void onSubscribe(Subscription s) { @Override public void onNext(AsyncResponseTransformer asyncResponseTransformer) { + log.info(() -> "onNext " + completedParts.get()); if (asyncResponseTransformer == null) { throw new NullPointerException("onNext must not be called with null asyncResponseTransformer"); } diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadIntegrationTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadIntegrationTest.java index ec92800673a4..d0d972f9ab61 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadIntegrationTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadIntegrationTest.java @@ -59,21 +59,19 @@ class MultipartDownloadIntegrationTest { static final String key = String.format("debug-test-%smb", fileTestSize); private S3AsyncClient s3; - private final SplittingTransformerConfiguration splitConfig = SplittingTransformerConfiguration.builder() - .bufferSize(1024 * 1024 * 32L) - .build(); @BeforeEach void init() { this.s3 = S3AsyncClient.builder() .region(Region.US_WEST_2) .multipartEnabled(true) + .multipartConfiguration(c -> c.apiCallBufferSizeInBytes(1024L * 32)) .credentialsProvider(ProfileCredentialsProvider.create()) .httpClient(NettyNioAsyncHttpClient.create()) .build(); } - // @Test + @Test void testByteAsyncResponseTransformer() { CompletableFuture> response = s3.getObject( r -> r.bucket(bucket).key(key), @@ -85,7 +83,7 @@ void testByteAsyncResponseTransformer() { assertThat(bytes).hasSize(fileTestSize * 1024 * 1024); } - // @Test + @Test void testFileAsyncResponseTransformer() { Path path = Paths.get("/Users/olapplin/Develop/tmp", LocalDateTime.now().format(DateTimeFormatter.BASIC_ISO_DATE) + '-' + key); @@ -98,7 +96,7 @@ void testFileAsyncResponseTransformer() { assertThat(path.toFile()).hasSize(fileTestSize * 1024 * 1024); } - // @Test + @Test void testPublisherAsyncResponseTransformer() { CompletableFuture> future = s3.getObject( r -> r.bucket(bucket).key(key), @@ -145,7 +143,7 @@ public void onComplete() { assertThat(total).hasValue(fileTestSize * 1024 * 1024); } - // @Test + @Test void testBlockingInputStreamResponseTransformer() { CompletableFuture> future = s3.getObject( r -> r.bucket(bucket).key(key), 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/MultipartDownloaderSubscriberWiremockTest.java index f1d4a157061e..356f946622cf 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/MultipartDownloaderSubscriberWiremockTest.java @@ -16,21 +16,23 @@ 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.exactly; 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.urlMatching; import static com.github.tomakehurst.wiremock.client.WireMock.verify; +import static com.github.tomakehurst.wiremock.stubbing.Scenario.STARTED; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.params.provider.Arguments.arguments; import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo; import com.github.tomakehurst.wiremock.junit5.WireMockTest; -import com.google.common.jimfs.Jimfs; import java.net.URI; -import java.nio.file.FileSystem; -import java.nio.file.Path; +import java.time.Duration; +import java.time.temporal.ChronoUnit; import java.util.Arrays; import java.util.List; import java.util.Random; @@ -51,6 +53,7 @@ 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.model.S3Exception; import software.amazon.awssdk.services.s3.utils.AsyncResponseTransformerTestSupplier; import software.amazon.awssdk.utils.Pair; @@ -74,6 +77,7 @@ public void init(WireMockRuntimeInfo wiremock) { .serviceConfiguration(S3Configuration.builder() .pathStyleAccessEnabled(true) .build()) + .overrideConfiguration(b -> b.retryPolicy(p -> p.numRetries(5))) .build(); random = new Random(); eTag = UUID.randomUUID().toString(); @@ -81,7 +85,7 @@ public void init(WireMockRuntimeInfo wiremock) { @ParameterizedTest @MethodSource("argumentsProvider") - void happyPath_allAsyncResponseTransformer(AsyncResponseTransformerTestSupplier supplier, + void happyPath_shouldReceiveAllBodyPArtInCorrectOrder(AsyncResponseTransformerTestSupplier supplier, int amountOfPartToTest, int partSize) { byte[] expectedBody = new byte[amountOfPartToTest * partSize]; @@ -90,16 +94,7 @@ void happyPath_allAsyncResponseTransformer(AsyncResponseTransformerTestSuppl System.arraycopy(individualBody, 0, expectedBody, i * partSize, individualBody.length); } - AsyncResponseTransformer transformer; - if (supplier.requiresJimfs()) { - FileSystem jimfs = Jimfs.newFileSystem(); - String filePath = "/tmp-file-" + UUID.randomUUID(); - Path inMemoryFilePath = jimfs.getPath(filePath); - transformer = supplier.transformer(inMemoryFilePath); - } else { - transformer = supplier.transformer(null); - } - + AsyncResponseTransformer transformer = supplier.transformer(); SplitAsyncResponseTransformer split = transformer.split( SplittingTransformerConfiguration.builder() .bufferSize(1024 * 1024 * 64L) @@ -119,6 +114,97 @@ void happyPath_allAsyncResponseTransformer(AsyncResponseTransformerTestSuppl verifyCorrectAmountOfRequestsMade(amountOfPartToTest); } + @ParameterizedTest + @MethodSource("argumentsProvider") + void retryableErrorOnFirstRequest_shouldRetryAndSucceed(AsyncResponseTransformerTestSupplier supplier, + int amountOfPartToTest, + int partSize) { + + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", testBucket, testKey))) + .inScenario("RETRY") + .whenScenarioStateIs(STARTED) + .willSetStateTo("SUCCESS") + .willReturn( + aResponse() + .withStatus(500) + .withBody(" InternalErrortest error msg") + .withHeader("x-amzn-ErrorType", "InternalError"))); + + byte[] expectedBody = new byte[amountOfPartToTest * partSize]; + for (int i = 0; i < amountOfPartToTest; i++) { + byte[] individualBody = stubForPartSuccess(i + 1, amountOfPartToTest, partSize); + System.arraycopy(individualBody, 0, expectedBody, i * partSize, individualBody.length); + } + + AsyncResponseTransformer transformer = supplier.transformer(); + SplitAsyncResponseTransformer split = transformer.split( + SplittingTransformerConfiguration.builder() + .bufferSize(1024 * 1024 * 64L) + .build()); + Subscriber> subscriber = new MultipartDownloaderSubscriber( + s3AsyncClient, + GetObjectRequest.builder() + .bucket(testBucket) + .key(testKey) + .build()); + + split.publisher().subscribe(subscriber); + T response = split.preparedFuture().join(); + byte[] body = supplier.body(response); + assertArrayEquals(expectedBody, body); + verify(exactly(amountOfPartToTest + 1), getRequestedFor(urlMatching(".*partNumber=\\d+.*"))); + } + + @ParameterizedTest + @MethodSource("argumentsProvider") + void nonRetryableErrorOnFirstRequest_shouldCompleteExceptionally(AsyncResponseTransformerTestSupplier supplier, + int amountOfPartToTest, + int partSize) { + stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", testBucket, testKey))) + .willReturn( + aResponse() + .withStatus(409) + .withBody(" OperationAbortedtest error msg") + .withHeader("x-amzn-ErrorType", "OperationAborted"))); + + AsyncResponseTransformer transformer = supplier.transformer(); + SplitAsyncResponseTransformer split = transformer.split( + SplittingTransformerConfiguration.builder() + .bufferSize(1024 * 1024 * 64L) + .build()); + Subscriber> subscriber = new MultipartDownloaderSubscriber( + s3AsyncClient, + GetObjectRequest.builder() + .bucket(testBucket) + .key(testKey) + .build()); + split.publisher().subscribe(subscriber); + + split.preparedFuture().join(); + + assertThat(split.preparedFuture()) + .isCompletedExceptionally() + .withFailMessage("test error msg"); + + verify(exactly(1), getRequestedFor(urlMatching(".*partNumber=\\d+.*"))); + verify(exactly(0), getRequestedFor(urlMatching(".*partNumber=[^1].*"))); + + } + + private 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") + .whenScenarioStateIs("SUCCESS") + .willReturn( + aResponse() + .withHeader("x-amz-mp-parts-count", totalPart + "") + .withHeader("ETag", eTag) + .withBody(body))); + return body; + } + private byte[] stubForPart(int part, int totalPart, int partSize) { byte[] body = new byte[partSize]; random.nextBytes(body); @@ -139,6 +225,10 @@ private static List> transformersSupplie ); } + private static Stream transformerArguments() { + return transformersSuppliers().stream().map(Arguments::arguments); + } + private static Stream argumentsProvider() { // amount of part, individual part size List> partSizes = Arrays.asList( diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java index 9e1d03a896ab..1cf96664a137 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java @@ -17,6 +17,7 @@ import static org.junit.jupiter.api.Assertions.fail; +import com.google.common.jimfs.Jimfs; import java.io.IOException; import java.nio.ByteBuffer; import java.nio.file.FileSystem; @@ -24,6 +25,7 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.List; +import java.util.UUID; import java.util.concurrent.CountDownLatch; import org.reactivestreams.Subscriber; import org.reactivestreams.Subscription; @@ -36,8 +38,9 @@ import software.amazon.awssdk.utils.IoUtils; /** - * Contains the {@link AsyncResponseTransformer} to be used in a test as well as logic on how to - * retrieve the body content of the request for that specific transformer. + * Contains the {@link AsyncResponseTransformer} to be used in a test as well as logic on how to retrieve the body content of the + * request for that specific transformer. + * * @param the type returned of the future associated with the {@link AsyncResponseTransformer} */ public interface AsyncResponseTransformerTestSupplier { @@ -50,7 +53,7 @@ public byte[] body(ResponseBytes response) { } @Override - public AsyncResponseTransformer> transformer(Path elem) { + public AsyncResponseTransformer> transformer() { return AsyncResponseTransformer.toBytes(); } @@ -73,7 +76,7 @@ public byte[] body(ResponseInputStream response) { } @Override - public AsyncResponseTransformer> transformer(Path elem) { + public AsyncResponseTransformer> transformer() { return AsyncResponseTransformer.toBlockingInputStream(); } @@ -98,9 +101,11 @@ public byte[] body(GetObjectResponse response) { } @Override - public AsyncResponseTransformer transformer(Path path) { - this.path = path; - return AsyncResponseTransformer.toFile(path); + public AsyncResponseTransformer transformer() { + FileSystem jimfs = Jimfs.newFileSystem(); + String filePath = "/tmp-file-" + UUID.randomUUID(); + this.path = jimfs.getPath(filePath); + return AsyncResponseTransformer.toFile(this.path); } @Override @@ -167,7 +172,7 @@ private byte[] unbox(Byte[] arr) { } @Override - public AsyncResponseTransformer> transformer(Path elem) { + public AsyncResponseTransformer> transformer() { return AsyncResponseTransformer.toPublisher(); } @@ -179,14 +184,15 @@ public String toString() { /** * Call this method to retrieve the AsyncResponseTransformer required to perform the test - * @param path + * * @return */ - AsyncResponseTransformer transformer(Path path); + AsyncResponseTransformer transformer(); /** - * Implementation of this method whould retreive the whole body of the request made using the AsyncResponseTransformer - * as a byte array. + * Implementation of this method whould retreive the whole body of the request made using the AsyncResponseTransformer as a + * byte array. + * * @param response the response the {@link AsyncResponseTransformerTestSupplier#transformer} * @return */ @@ -195,6 +201,7 @@ public String toString() { /** * Sonce {@link FileAsyncResponseTransformer} works with file, some test might need to initialize an in-memory * {@link FileSystem} with jimfs. + * * @return true if the test using this class requires setup with jimfs */ default boolean requiresJimfs() { From 4feb2931a1d889c32c4025f6aba66c5b0797001e Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Thu, 22 Feb 2024 12:06:17 -0500 Subject: [PATCH 09/15] testing errors with InputStream and Publisher --- .../internal/async/SplittingTransformer.java | 25 +-- .../MultipartDownloaderSubscriber.java | 5 + .../multipart/MultipartDownloadTestUtil.java | 98 +++++++++++ ...ipartDownloaderSubscriberWiremockTest.java | 157 +++++------------- .../AsyncResponseTransformerTestSupplier.java | 11 +- .../async/DelegatingBufferingSubscriber.java | 2 +- 6 files changed, 169 insertions(+), 129 deletions(-) create mode 100644 services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadTestUtil.java diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java index d482bf89d901..dc98c94a61c5 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java @@ -26,6 +26,7 @@ import software.amazon.awssdk.core.async.AsyncResponseTransformer; import software.amazon.awssdk.core.async.SdkPublisher; import software.amazon.awssdk.core.retry.RetryUtils; +import software.amazon.awssdk.utils.CompletableFutureUtils; import software.amazon.awssdk.utils.Logger; import software.amazon.awssdk.utils.Validate; import software.amazon.awssdk.utils.async.DelegatingBufferingSubscriber; @@ -54,6 +55,8 @@ public class SplittingTransformer implements SdkPublisher upstreamResponseTransformer; + private final AtomicBoolean preparedCalled = new AtomicBoolean(false); + /** * Set to true once {@code .onResponse()} is called on the upstreamResponseTransformer */ @@ -191,7 +194,9 @@ private void handleCancelState() { if (downstreamSubscriber == null) { return; } - publisherToUpstream.complete(); + publisherToUpstream.complete().whenComplete((v, t) -> { + downstreamSubscriber.onComplete(); + }); downstreamSubscriber = null; } } @@ -210,13 +215,12 @@ private class IndividualTransformer implements AsyncResponseTransformer prepare() { trLog.info(() -> "prepare"); this.individualFuture = new CompletableFuture<>(); - CompletableFuture upstreamFuture = upstreamResponseTransformer.prepare(); - upstreamFuture.whenComplete((res, err) -> { - trLog.info(() -> String.format("upstreamFuture completed: res: %s. error: %s", res, err)); - if (res != null) { - returnFuture.complete(res); + if (preparedCalled.compareAndSet(false, true)) { + CompletableFuture upstreamFuture = upstreamResponseTransformer.prepare(); + if (!returnFuture.isDone()) { + CompletableFutureUtils.forwardResultTo(upstreamFuture, returnFuture); } - }); + } individualFuture.whenComplete((r, e) -> { if (isCancelled.get()) { handleCancelState(); @@ -246,7 +250,6 @@ public void onStream(SdkPublisher publisher) { .maximumBufferInBytes(maximumBufferInBytes) .delegate(upstreamSubscriber) .build() - // upstreamSubscriber )); } publisher.subscribe(new IndividualPartSubscriber<>(this.individualFuture, response, publisherToUpstream)); @@ -254,8 +257,9 @@ public void onStream(SdkPublisher publisher) { @Override public void exceptionOccurred(Throwable error) { - trLog.info(() -> "exceptionOccurred"); - individualFuture.completeExceptionally(error); + trLog.info(() -> "exceptionOccurred: " + error.toString()); + + publisherToUpstream.error(error); upstreamResponseTransformer.exceptionOccurred(error); } } @@ -294,7 +298,6 @@ public void onNext(ByteBuffer byteBuffer) { if (byteBuffer == null) { throw new NullPointerException("onNext must not be called with null byteBuffer"); } - partLogger.info(() -> "onNext"); publisherToUpstream.send(byteBuffer).whenComplete((r, t) -> { if (t != null) { handleError(t); diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java index 5c11efd558da..bf7339e49475 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java @@ -24,6 +24,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; /** @@ -146,6 +147,10 @@ public void onComplete() { future.complete(null); } + public CompletableFuture future() { + return this.future; + } + private GetObjectRequest nextRequest(int nextPartToGet) { return getObjectRequest.copy(req -> { req.partNumber(nextPartToGet); 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 new file mode 100644 index 000000000000..708972b6b0d7 --- /dev/null +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadTestUtil.java @@ -0,0 +1,98 @@ +/* + * 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.urlMatching; +import static com.github.tomakehurst.wiremock.client.WireMock.verify; + +import java.util.Arrays; +import java.util.List; +import java.util.Random; +import software.amazon.awssdk.services.s3.utils.AsyncResponseTransformerTestSupplier; + +public class MultipartDownloadTestUtil { + + 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(); + + public MultipartDownloadTestUtil(String testBucket, String testKey, String eTag) { + this.testBucket = testBucket; + this.testKey = testKey; + this.eTag = eTag; + } + + public static List> transformersSuppliers() { + return Arrays.asList( + new AsyncResponseTransformerTestSupplier.ByteTestArtSupplier(), + new AsyncResponseTransformerTestSupplier.InputStreamArtSupplier(), + new AsyncResponseTransformerTestSupplier.PublisherArtSupplier(), + new AsyncResponseTransformerTestSupplier.FileArtSupplier() + ); + } + + public byte[] stubAllParts(String testBucket, String testKey, int amountOfPartToTest, int partSize) { + byte[] expectedBody = new byte[amountOfPartToTest * partSize]; + for (int i = 0; i < amountOfPartToTest; i++) { + byte[] individualBody = stubForPart(testBucket, testKey, i + 1, amountOfPartToTest, partSize); + System.arraycopy(individualBody, 0, expectedBody, i * partSize, individualBody.length); + } + return expectedBody; + } + + public byte[] stubForPart(String testBucket, String testKey,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))).willReturn( + aResponse() + .withHeader("x-amz-mp-parts-count", totalPart + "") + .withHeader("ETag", eTag) + .withBody(body))); + return body; + } + + public void verifyCorrectAmountOfRequestsMade(int amountOfPartToTest) { + String urlTemplate = ".*partNumber=%d.*"; + for (int i = 1; i <= amountOfPartToTest; i++) { + verify(getRequestedFor(urlMatching(String.format(urlTemplate, i)))); + } + 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; + } +} 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/MultipartDownloaderSubscriberWiremockTest.java index 356f946622cf..97cef38771d0 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/MultipartDownloaderSubscriberWiremockTest.java @@ -16,23 +16,21 @@ 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.exactly; 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.urlMatching; import static com.github.tomakehurst.wiremock.client.WireMock.verify; -import static com.github.tomakehurst.wiremock.stubbing.Scenario.STARTED; -import static org.assertj.core.api.Assertions.assertThat; +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.time.Duration; -import java.time.temporal.ChronoUnit; import java.util.Arrays; import java.util.List; import java.util.Random; @@ -53,7 +51,6 @@ 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.model.S3Exception; import software.amazon.awssdk.services.s3.utils.AsyncResponseTransformerTestSupplier; import software.amazon.awssdk.utils.Pair; @@ -64,8 +61,7 @@ class MultipartDownloaderSubscriberWiremockTest { private final String testKey = "test-key"; private S3AsyncClient s3AsyncClient; - private Random random; - private String eTag; + private MultipartDownloadTestUtil util; @BeforeEach public void init(WireMockRuntimeInfo wiremock) { @@ -77,27 +73,20 @@ public void init(WireMockRuntimeInfo wiremock) { .serviceConfiguration(S3Configuration.builder() .pathStyleAccessEnabled(true) .build()) - .overrideConfiguration(b -> b.retryPolicy(p -> p.numRetries(5))) .build(); - random = new Random(); - eTag = UUID.randomUUID().toString(); + util = new MultipartDownloadTestUtil(testBucket, testKey, UUID.randomUUID().toString()); } @ParameterizedTest @MethodSource("argumentsProvider") void happyPath_shouldReceiveAllBodyPArtInCorrectOrder(AsyncResponseTransformerTestSupplier supplier, - int amountOfPartToTest, - int partSize) { - byte[] expectedBody = new byte[amountOfPartToTest * partSize]; - for (int i = 0; i < amountOfPartToTest; i++) { - byte[] individualBody = stubForPart(i + 1, amountOfPartToTest, partSize); - System.arraycopy(individualBody, 0, expectedBody, i * partSize, individualBody.length); - } - + int amountOfPartToTest, + int partSize) { + byte[] expectedBody = util.stubAllParts(testBucket, testKey, amountOfPartToTest, partSize); AsyncResponseTransformer transformer = supplier.transformer(); SplitAsyncResponseTransformer split = transformer.split( SplittingTransformerConfiguration.builder() - .bufferSize(1024 * 1024 * 64L) + .bufferSize(1024 * 32L) .build()); Subscriber> subscriber = new MultipartDownloaderSubscriber( s3AsyncClient, @@ -111,35 +100,22 @@ void happyPath_shouldReceiveAllBodyPArtInCorrectOrder(AsyncResponseTransform byte[] body = supplier.body(response); assertArrayEquals(expectedBody, body); - verifyCorrectAmountOfRequestsMade(amountOfPartToTest); + util.verifyCorrectAmountOfRequestsMade(amountOfPartToTest); } @ParameterizedTest @MethodSource("argumentsProvider") - void retryableErrorOnFirstRequest_shouldRetryAndSucceed(AsyncResponseTransformerTestSupplier supplier, - int amountOfPartToTest, - int partSize) { - - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", testBucket, testKey))) - .inScenario("RETRY") - .whenScenarioStateIs(STARTED) - .willSetStateTo("SUCCESS") - .willReturn( - aResponse() - .withStatus(500) - .withBody(" InternalErrortest error msg") - .withHeader("x-amzn-ErrorType", "InternalError"))); - - byte[] expectedBody = new byte[amountOfPartToTest * partSize]; - for (int i = 0; i < amountOfPartToTest; i++) { - byte[] individualBody = stubForPartSuccess(i + 1, amountOfPartToTest, partSize); - System.arraycopy(individualBody, 0, expectedBody, i * partSize, individualBody.length); - } - + void errorOnFirstRequest_shouldCompleteExceptionally(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(); - SplitAsyncResponseTransformer split = transformer.split( + SplitAsyncResponseTransformer split = transformer.split( SplittingTransformerConfiguration.builder() - .bufferSize(1024 * 1024 * 64L) + .bufferSize(1024 * 32L) .build()); Subscriber> subscriber = new MultipartDownloaderSubscriber( s3AsyncClient, @@ -149,28 +125,26 @@ void retryableErrorOnFirstRequest_shouldRetryAndSucceed(AsyncResponseTransfo .build()); split.publisher().subscribe(subscriber); - T response = split.preparedFuture().join(); - byte[] body = supplier.body(response); - assertArrayEquals(expectedBody, body); - verify(exactly(amountOfPartToTest + 1), getRequestedFor(urlMatching(".*partNumber=\\d+.*"))); + assertThatThrownBy(() -> split.preparedFuture().join()) + .hasMessageContaining("test error message"); } @ParameterizedTest @MethodSource("argumentsProvider") - void nonRetryableErrorOnFirstRequest_shouldCompleteExceptionally(AsyncResponseTransformerTestSupplier supplier, - int amountOfPartToTest, - int partSize) { - stubFor(get(urlEqualTo(String.format("/%s/%s?partNumber=1", testBucket, testKey))) - .willReturn( - aResponse() - .withStatus(409) - .withBody(" OperationAbortedtest error msg") - .withHeader("x-amzn-ErrorType", "OperationAborted"))); - + void errorOnThirdRequest_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(); - SplitAsyncResponseTransformer split = transformer.split( + SplitAsyncResponseTransformer split = transformer.split( SplittingTransformerConfiguration.builder() - .bufferSize(1024 * 1024 * 64L) + .bufferSize(1024 * 32L) .build()); Subscriber> subscriber = new MultipartDownloaderSubscriber( s3AsyncClient, @@ -178,55 +152,17 @@ void nonRetryableErrorOnFirstRequest_shouldCompleteExceptionally(AsyncRespon .bucket(testBucket) .key(testKey) .build()); - split.publisher().subscribe(subscriber); - - split.preparedFuture().join(); - assertThat(split.preparedFuture()) - .isCompletedExceptionally() - .withFailMessage("test error msg"); - - verify(exactly(1), getRequestedFor(urlMatching(".*partNumber=\\d+.*"))); - verify(exactly(0), getRequestedFor(urlMatching(".*partNumber=[^1].*"))); - - } - - private 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") - .whenScenarioStateIs("SUCCESS") - .willReturn( - aResponse() - .withHeader("x-amz-mp-parts-count", totalPart + "") - .withHeader("ETag", eTag) - .withBody(body))); - return body; - } - - private byte[] stubForPart(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))).willReturn( - aResponse() - .withHeader("x-amz-mp-parts-count", totalPart + "") - .withHeader("ETag", eTag) - .withBody(body))); - return body; - } - - private static List> transformersSuppliers() { - return Arrays.asList( - new AsyncResponseTransformerTestSupplier.ByteTestArtSupplier(), - new AsyncResponseTransformerTestSupplier.InputStreamArtSupplier(), - new AsyncResponseTransformerTestSupplier.PublisherArtSupplier(), - new AsyncResponseTransformerTestSupplier.FileArtSupplier() - ); - } - - private static Stream transformerArguments() { - return transformersSuppliers().stream().map(Arguments::arguments); + if (partSize > 1) { + split.publisher().subscribe(subscriber); + assertThatThrownBy(() -> { + T res = split.preparedFuture().join(); + supplier.body(res); + }).hasMessageContaining("test error message"); + } else { + T res = split.preparedFuture().join(); + assertNotNull(supplier.body(res)); + } } private static Stream argumentsProvider() { @@ -248,11 +184,4 @@ private static Stream argumentsProvider() { return sb.build(); } - private void verifyCorrectAmountOfRequestsMade(int amountOfPartToTest) { - String urlTemplate = ".*partNumber=%d.*"; - for (int i = 1; i <= amountOfPartToTest; i++) { - verify(getRequestedFor(urlMatching(String.format(urlTemplate, i)))); - } - verify(0, getRequestedFor(urlMatching(String.format(urlTemplate, amountOfPartToTest + 1)))); - } } \ No newline at end of file diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java index 1cf96664a137..c87e40ad07f3 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/AsyncResponseTransformerTestSupplier.java @@ -19,6 +19,7 @@ import com.google.common.jimfs.Jimfs; import java.io.IOException; +import java.io.UncheckedIOException; import java.nio.ByteBuffer; import java.nio.file.FileSystem; import java.nio.file.Files; @@ -27,6 +28,7 @@ import java.util.List; import java.util.UUID; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicReference; import org.reactivestreams.Subscriber; import org.reactivestreams.Subscription; import software.amazon.awssdk.core.ResponseBytes; @@ -70,8 +72,7 @@ public byte[] body(ResponseInputStream response) { try { return IoUtils.toByteArray(response); } catch (IOException ioe) { - fail("unexpected IOE during test", ioe); - return null; + throw new UncheckedIOException(ioe); } } @@ -125,6 +126,7 @@ class PublisherArtSupplier implements AsyncResponseTransformerTestSupplier response) { List buffer = new ArrayList<>(); CountDownLatch latch = new CountDownLatch(1); + AtomicReference error = new AtomicReference<>(); response.subscribe(new Subscriber() { Subscription s; @@ -144,8 +146,8 @@ public void onNext(ByteBuffer byteBuffer) { @Override public void onError(Throwable t) { + error.set(t); latch.countDown(); - fail("Unexpected onError during test", t); } @Override @@ -158,6 +160,9 @@ public void onComplete() { } catch (InterruptedException e) { fail("Unexpected thread interruption during test", e); } + if (error.get() != null) { + throw new RuntimeException(error.get()); + } return unbox(buffer.toArray(new Byte[0])); } diff --git a/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java b/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java index 509c7dee7da6..9e29737d444b 100644 --- a/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java +++ b/utils/src/main/java/software/amazon/awssdk/utils/async/DelegatingBufferingSubscriber.java @@ -96,7 +96,7 @@ boolean additionalOnCompleteNeededCheck() { */ @Override boolean additionalUpstreamDemandNeededCheck() { - return true; + return currentlyBuffered.get() < maximumBufferInBytes; } public static Builder builder() { From 29f01acf0f959b6732928b9dc97c542d9cdc84c8 Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Wed, 28 Feb 2024 10:45:55 -0500 Subject: [PATCH 10/15] checkstyle --- .../awssdk/core/internal/async/SplittingTransformer.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java index dc98c94a61c5..6ddd7de8c4be 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java @@ -25,7 +25,6 @@ import software.amazon.awssdk.core.SplittingTransformerConfiguration; import software.amazon.awssdk.core.async.AsyncResponseTransformer; import software.amazon.awssdk.core.async.SdkPublisher; -import software.amazon.awssdk.core.retry.RetryUtils; import software.amazon.awssdk.utils.CompletableFutureUtils; import software.amazon.awssdk.utils.Logger; import software.amazon.awssdk.utils.Validate; @@ -268,7 +267,7 @@ public void exceptionOccurred(Throwable error) { * the Subscriber for each of the individual request's ByteBuffer publisher */ static class IndividualPartSubscriber implements Subscriber { - private static final Logger partLogger = Logger.loggerFor(IndividualPartSubscriber.class); + private static final Logger PART_LOGGER = Logger.loggerFor(IndividualPartSubscriber.class); private final CompletableFuture future; private final T response; @@ -284,7 +283,7 @@ static class IndividualPartSubscriber implements Subscriber { @Override public void onSubscribe(Subscription s) { - partLogger.info(() -> "onSubscribe"); + PART_LOGGER.info(() -> "onSubscribe"); if (this.subscription != null) { s.cancel(); return; @@ -309,13 +308,13 @@ public void onNext(ByteBuffer byteBuffer) { @Override public void onError(Throwable t) { - partLogger.info(() -> "onError"); + PART_LOGGER.info(() -> "onError"); handleError(t); } @Override public void onComplete() { - partLogger.info(() -> "onComplete"); + PART_LOGGER.info(() -> "onComplete"); future.complete(response); } From 7db5e9c337e16a86f17c491c679eccce23fa6a84 Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Wed, 28 Feb 2024 11:07:26 -0500 Subject: [PATCH 11/15] checkstyle --- .../s3/internal/multipart/MultipartDownloaderSubscriber.java | 1 - 1 file changed, 1 deletion(-) diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java index bf7339e49475..65f272f4c0b8 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java @@ -24,7 +24,6 @@ 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; /** From b0a20db05c958ee8e6cedac3d361a262c5f7d1f5 Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Wed, 28 Feb 2024 11:47:33 -0500 Subject: [PATCH 12/15] prevent test memory heap error --- .../multipart/MultipartDownloaderSubscriberWiremockTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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/MultipartDownloaderSubscriberWiremockTest.java index 97cef38771d0..84c66d8edfac 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/MultipartDownloaderSubscriberWiremockTest.java @@ -175,7 +175,7 @@ private static Stream argumentsProvider() { Pair.of(1, 1024 * 1024), Pair.of(4, 1024 * 1024), Pair.of(1, 4 * 1024 * 1024), - Pair.of(16, 16 * 1024 * 1024), + Pair.of(4, 6 * 1024 * 1024), Pair.of(7, 5 * 3752) ); From 1a86cb4695c16292cf5337e0b2f3ff135b51f469 Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Thu, 29 Feb 2024 10:53:44 -0500 Subject: [PATCH 13/15] fix merge --- .../internal/async/SplittingTransformer.java | 13 +++++----- .../MultipartDownloaderSubscriber.java | 2 +- ...ipartDownloaderSubscriberWiremockTest.java | 25 ++++++++----------- 3 files changed, 18 insertions(+), 22 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java index ec604b03cb03..44afb47fe6fb 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java @@ -219,7 +219,7 @@ private class IndividualTransformer implements AsyncResponseTransformer prepare() { - trLog.info(() -> "prepare"); + trLog.trace(() -> "prepare"); this.individualFuture = new CompletableFuture<>(); if (preparedCalled.compareAndSet(false, true)) { CompletableFuture upstreamFuture = upstreamResponseTransformer.prepare(); @@ -237,7 +237,7 @@ public CompletableFuture prepare() { @Override public void onResponse(ResponseT response) { - trLog.info(() -> "onResponse"); + trLog.trace(() -> "onResponse"); if (onResponseCalled.compareAndSet(false, true)) { log.trace(() -> "calling onResponse on the upstream transformer"); upstreamResponseTransformer.onResponse(response); @@ -247,7 +247,7 @@ public void onResponse(ResponseT response) { @Override public void onStream(SdkPublisher publisher) { - trLog.info(() -> "onStream"); + trLog.trace(() -> "onStream"); if (onStreamCalled.compareAndSet(false, true)) { log.trace(() -> "calling onStream on the upstream transformer"); upstreamResponseTransformer.onStream(upstreamSubscriber -> publisherToUpstream.subscribe( @@ -262,7 +262,7 @@ public void onStream(SdkPublisher publisher) { @Override public void exceptionOccurred(Throwable error) { - trLog.info(() -> "exceptionOccurred: " + error.toString()); + trLog.trace(() -> "exceptionOccurred: " + error.toString()); publisherToUpstream.error(error); upstreamResponseTransformer.exceptionOccurred(error); @@ -289,7 +289,7 @@ static class IndividualPartSubscriber implements Subscriber { @Override public void onSubscribe(Subscription s) { - PART_LOGGER.info(() -> "onSubscribe"); + PART_LOGGER.trace(() -> "onSubscribe"); if (this.subscription != null) { s.cancel(); return; @@ -300,6 +300,7 @@ public void onSubscribe(Subscription s) { @Override public void onNext(ByteBuffer byteBuffer) { + PART_LOGGER.trace(() -> "onNext"); if (byteBuffer == null) { throw new NullPointerException("onNext must not be called with null byteBuffer"); } @@ -314,7 +315,7 @@ public void onNext(ByteBuffer byteBuffer) { @Override public void onError(Throwable t) { - PART_LOGGER.info(() -> "onError"); + PART_LOGGER.trace(() -> "onError"); handleError(t); } diff --git a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java index 65f272f4c0b8..579e04b802b4 100644 --- a/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java +++ b/services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloaderSubscriber.java @@ -93,7 +93,7 @@ public void onSubscribe(Subscription s) { @Override public void onNext(AsyncResponseTransformer asyncResponseTransformer) { - log.info(() -> "onNext " + completedParts.get()); + log.trace(() -> "onNext " + completedParts.get()); if (asyncResponseTransformer == null) { throw new NullPointerException("onNext must not be called with null asyncResponseTransformer"); } 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/MultipartDownloaderSubscriberWiremockTest.java index 84c66d8edfac..6780bd38864e 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/MultipartDownloaderSubscriberWiremockTest.java @@ -17,11 +17,8 @@ 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.urlMatching; -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; @@ -33,7 +30,6 @@ import java.net.URI; import java.util.Arrays; import java.util.List; -import java.util.Random; import java.util.UUID; import java.util.stream.Stream; import org.junit.jupiter.api.BeforeEach; @@ -45,7 +41,6 @@ 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.async.SplitAsyncResponseTransformer; import software.amazon.awssdk.regions.Region; import software.amazon.awssdk.services.s3.S3AsyncClient; import software.amazon.awssdk.services.s3.S3Configuration; @@ -84,9 +79,9 @@ void happyPath_shouldReceiveAllBodyPArtInCorrectOrder(AsyncResponseTransform int partSize) { byte[] expectedBody = util.stubAllParts(testBucket, testKey, amountOfPartToTest, partSize); AsyncResponseTransformer transformer = supplier.transformer(); - SplitAsyncResponseTransformer split = transformer.split( + AsyncResponseTransformer.SplitResult split = transformer.split( SplittingTransformerConfiguration.builder() - .bufferSize(1024 * 32L) + .bufferSizeInBytes(1024 * 32L) .build()); Subscriber> subscriber = new MultipartDownloaderSubscriber( s3AsyncClient, @@ -96,7 +91,7 @@ void happyPath_shouldReceiveAllBodyPArtInCorrectOrder(AsyncResponseTransform .build()); split.publisher().subscribe(subscriber); - T response = split.preparedFuture().join(); + T response = split.resultFuture().join(); byte[] body = supplier.body(response); assertArrayEquals(expectedBody, body); @@ -113,9 +108,9 @@ void errorOnFirstRequest_shouldCompleteExceptionally(AsyncResponseTransforme .withStatus(400) .withBody("400test error message"))); AsyncResponseTransformer transformer = supplier.transformer(); - SplitAsyncResponseTransformer split = transformer.split( + AsyncResponseTransformer.SplitResult split = transformer.split( SplittingTransformerConfiguration.builder() - .bufferSize(1024 * 32L) + .bufferSizeInBytes(1024 * 32L) .build()); Subscriber> subscriber = new MultipartDownloaderSubscriber( s3AsyncClient, @@ -125,7 +120,7 @@ void errorOnFirstRequest_shouldCompleteExceptionally(AsyncResponseTransforme .build()); split.publisher().subscribe(subscriber); - assertThatThrownBy(() -> split.preparedFuture().join()) + assertThatThrownBy(() -> split.resultFuture().join()) .hasMessageContaining("test error message"); } @@ -142,9 +137,9 @@ void errorOnThirdRequest_shouldCompleteExceptionallyOnlyPartsGreaterThanTwo( .withStatus(400) .withBody("400test error message"))); AsyncResponseTransformer transformer = supplier.transformer(); - SplitAsyncResponseTransformer split = transformer.split( + AsyncResponseTransformer.SplitResult split = transformer.split( SplittingTransformerConfiguration.builder() - .bufferSize(1024 * 32L) + .bufferSizeInBytes(1024 * 32L) .build()); Subscriber> subscriber = new MultipartDownloaderSubscriber( s3AsyncClient, @@ -156,11 +151,11 @@ void errorOnThirdRequest_shouldCompleteExceptionallyOnlyPartsGreaterThanTwo( if (partSize > 1) { split.publisher().subscribe(subscriber); assertThatThrownBy(() -> { - T res = split.preparedFuture().join(); + T res = split.resultFuture().join(); supplier.body(res); }).hasMessageContaining("test error message"); } else { - T res = split.preparedFuture().join(); + T res = split.resultFuture().join(); assertNotNull(supplier.body(res)); } } From aa0e063eb967d48bc75e9f498cc25893623f9722 Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Thu, 29 Feb 2024 11:39:24 -0500 Subject: [PATCH 14/15] handle cancel signal correctly, and make IndividualPartSubscriber non-static to do so. - removed trace/debug logging --- .../internal/async/SplittingTransformer.java | 43 +++++++++++-------- .../IndividualPartSubscriberTckTest.java | 12 +++++- 2 files changed, 34 insertions(+), 21 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java index 44afb47fe6fb..97d9d08b2146 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java @@ -200,10 +200,22 @@ private void handleCancelState() { if (downstreamSubscriber == null) { return; } + if (!onStreamCalled.get()) { + // we never subscribe publisherToUpstream to the upstream, it would not complete + downstreamSubscriber = null; + return; + } publisherToUpstream.complete().whenComplete((v, t) -> { - downstreamSubscriber.onComplete(); + if (downstreamSubscriber == null) { + return; + } + if (t != null) { + downstreamSubscriber.onError(t); + } else { + downstreamSubscriber.onComplete(); + } + downstreamSubscriber = null; }); - downstreamSubscriber = null; } } @@ -213,13 +225,11 @@ private void handleCancelState() { * body publisher. */ private class IndividualTransformer implements AsyncResponseTransformer { - private final Logger trLog = Logger.loggerFor(IndividualTransformer.class); private ResponseT response; private CompletableFuture individualFuture; @Override public CompletableFuture prepare() { - trLog.trace(() -> "prepare"); this.individualFuture = new CompletableFuture<>(); if (preparedCalled.compareAndSet(false, true)) { CompletableFuture upstreamFuture = upstreamResponseTransformer.prepare(); @@ -237,7 +247,6 @@ public CompletableFuture prepare() { @Override public void onResponse(ResponseT response) { - trLog.trace(() -> "onResponse"); if (onResponseCalled.compareAndSet(false, true)) { log.trace(() -> "calling onResponse on the upstream transformer"); upstreamResponseTransformer.onResponse(response); @@ -247,7 +256,9 @@ public void onResponse(ResponseT response) { @Override public void onStream(SdkPublisher publisher) { - trLog.trace(() -> "onStream"); + if (downstreamSubscriber == null) { + return; + } if (onStreamCalled.compareAndSet(false, true)) { log.trace(() -> "calling onStream on the upstream transformer"); upstreamResponseTransformer.onStream(upstreamSubscriber -> publisherToUpstream.subscribe( @@ -257,13 +268,11 @@ public void onStream(SdkPublisher publisher) { .build() )); } - publisher.subscribe(new IndividualPartSubscriber<>(this.individualFuture, response, publisherToUpstream)); + publisher.subscribe(new IndividualPartSubscriber<>(this.individualFuture, response)); } @Override public void exceptionOccurred(Throwable error) { - trLog.trace(() -> "exceptionOccurred: " + error.toString()); - publisherToUpstream.error(error); upstreamResponseTransformer.exceptionOccurred(error); } @@ -272,24 +281,19 @@ public void exceptionOccurred(Throwable error) { /** * the Subscriber for each of the individual request's ByteBuffer publisher */ - static class IndividualPartSubscriber implements Subscriber { - private static final Logger PART_LOGGER = Logger.loggerFor(IndividualPartSubscriber.class); + class IndividualPartSubscriber implements Subscriber { private final CompletableFuture future; private final T response; - private final SimplePublisher publisherToUpstream; private Subscription subscription; - IndividualPartSubscriber(CompletableFuture future, T response, - SimplePublisher bodyPartPublisher) { + IndividualPartSubscriber(CompletableFuture future, T response) { this.future = future; this.response = response; - this.publisherToUpstream = bodyPartPublisher; } @Override public void onSubscribe(Subscription s) { - PART_LOGGER.trace(() -> "onSubscribe"); if (this.subscription != null) { s.cancel(); return; @@ -300,7 +304,6 @@ public void onSubscribe(Subscription s) { @Override public void onNext(ByteBuffer byteBuffer) { - PART_LOGGER.trace(() -> "onNext"); if (byteBuffer == null) { throw new NullPointerException("onNext must not be called with null byteBuffer"); } @@ -309,19 +312,21 @@ public void onNext(ByteBuffer byteBuffer) { handleError(t); return; } + if (isCancelled.get()) { + handleCancelState(); + return; + } subscription.request(1); }); } @Override public void onError(Throwable t) { - PART_LOGGER.trace(() -> "onError"); handleError(t); } @Override public void onComplete() { - PART_LOGGER.info(() -> "onComplete"); future.complete(response); } diff --git a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/IndividualPartSubscriberTckTest.java b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/IndividualPartSubscriberTckTest.java index d1af5807eb11..a72a3ab7aa1f 100644 --- a/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/IndividualPartSubscriberTckTest.java +++ b/core/sdk-core/src/test/java/software/amazon/awssdk/core/internal/async/IndividualPartSubscriberTckTest.java @@ -23,6 +23,8 @@ import org.reactivestreams.Subscription; import org.reactivestreams.tck.SubscriberWhiteboxVerification; import org.reactivestreams.tck.TestEnvironment; +import software.amazon.awssdk.core.ResponseBytes; +import software.amazon.awssdk.core.async.AsyncResponseTransformer; import software.amazon.awssdk.utils.async.SimplePublisher; public class IndividualPartSubscriberTckTest extends SubscriberWhiteboxVerification { @@ -35,9 +37,15 @@ protected IndividualPartSubscriberTckTest() { @Override public Subscriber createSubscriber(WhiteboxSubscriberProbe probe) { - CompletableFuture future = new CompletableFuture<>(); + CompletableFuture future = new CompletableFuture<>(); SimplePublisher publisher = new SimplePublisher<>(); - return new SplittingTransformer.IndividualPartSubscriber(future, 0, publisher) { + SplittingTransformer> transformer = + SplittingTransformer.>builder() + .upstreamResponseTransformer(AsyncResponseTransformer.toBytes()) + .maximumBufferSizeInBytes(32L) + .resultFuture(new CompletableFuture<>()) + .build(); + return transformer.new IndividualPartSubscriber(future, ByteBuffer.wrap(new byte[0])) { @Override public void onSubscribe(Subscription s) { super.onSubscribe(s); From 7855c6e126cdadbebbb1360c9bb1cd36977220cb Mon Sep 17 00:00:00 2001 From: Olivier Lepage-Applin Date: Mon, 4 Mar 2024 09:46:13 -0500 Subject: [PATCH 15/15] attempt fix thread hanging for PublisherAsyncResponseTransformer --- .../awssdk/core/internal/async/SplittingTransformer.java | 4 ---- .../multipart/MultipartDownloadIntegrationTest.java | 6 +----- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java index 97d9d08b2146..cfee38e46780 100644 --- a/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java +++ b/core/sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java @@ -312,10 +312,6 @@ public void onNext(ByteBuffer byteBuffer) { handleError(t); return; } - if (isCancelled.get()) { - handleCancelState(); - return; - } subscription.request(1); }); } diff --git a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadIntegrationTest.java b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadIntegrationTest.java index 8edc28b2764a..214faa4c7c81 100644 --- a/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadIntegrationTest.java +++ b/services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/multipart/MultipartDownloadIntegrationTest.java @@ -58,9 +58,6 @@ class MultipartDownloadIntegrationTest { static final String key = String.format("debug-test-%smb", fileTestSize); private S3AsyncClient s3; - private final SplittingTransformerConfiguration splitConfig = SplittingTransformerConfiguration.builder() - .bufferSizeInBytes(1024 * 1024 * 32L) - .build(); @BeforeEach void init() { @@ -103,8 +100,6 @@ void testPublisherAsyncResponseTransformer() { CompletableFuture> future = s3.getObject( r -> r.bucket(bucket).key(key), AsyncResponseTransformer.toPublisher()); - AsyncResponseTransformer> transformer = - AsyncResponseTransformer.toPublisher(); CountDownLatch latch = new CountDownLatch(1); AtomicInteger total = new AtomicInteger(0); @@ -133,6 +128,7 @@ public void onError(Throwable t) { @Override public void onComplete() { + log.info(() -> "complete subscriber"); latch.countDown(); } });