Fix for multipart integ test failure #5176

L-Applin · 2024-05-02T17:30:47Z

The S3TransferManagerDownloadPauseResumeIntegrationTest was failing with S3 Multipart client for 2 reasons.

The downloads were not immediately stopped when calling .pause()
When resuming, the range get would have the whole object size as its content-length, making the TransferProgressUpdater report the wrong number of bytes.

This PR is to fix both of those issues.

Motivation and Context

Errors were raised during integration test execution.

Modifications

Fix for the errors consist in

Forward cancellation from returnFuture in SplittingTransformer
Use content-length for range-get in TransferProgressUpdater, when multipart is enabled.

Testing

Failing integration test run locally.
Added unit test.

- Use content-length for range-get in TransferProgressUpdater, when multipart is enabled.

dagnir

Can we kick off an integ test on the PR as well?

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

dagnir · 2024-05-02T19:18:16Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

@@ -196,7 +208,7 @@ private boolean doEmit() {
 }

 private void handleCancelState() {
- synchronized (this) {
+ synchronized (cancelLock) {


What if !onStreanCalled().get() is no longer true after we make the check?

onStreanCalled will only go from false -> true once and never back to false.

I meant more that just because onStreamCalled.get() == false when we evaluate the condition, it doesn't mean it's still false when we execute the body of the if. Is that going to be an issue?

if (!onStreamCalled.get()) { // value is false // value could have changed to true after we made the check }

Aaah I see what you mean. Looking at it, yep, it might cause a NPE at line 293. Adding synchronization around the if (onStreamCalled.compareAndSet(false, true)) on cancelLock there at line 293 should prevent it.

dagnir · 2024-05-02T19:21:46Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

 publisherToUpstream.send(byteBuffer).whenComplete((r, t) -> {
 if (t != null) {
 handleError(t);
 return;
 }
- subscription.request(1);
+ if (!isCancelled.get()) {


Similar question as above, is it possible this is no longer true after we check it? If so, is it an issue?

same as above, onStreanCalled will only go from false -> true once and never back to false.

Do we need the check here assuming subscription is cancelled properly or will be cancelled at this point?
https://github.com/reactive-streams/reactive-streams-jvm

If a Subscription is cancelled its Subscriber MUST eventually stop being signaled.

Note "eventually" not "right away"

we do need to stop right away in the case of the return future being cancelled, for example when pausing with transfer-manager

What would happen if we don't check it here and continue to send request demand? It should be no-op right?

L-Applin · 2024-05-02T20:01:52Z

Can we kick off an integ test on the PR as well?

Yep, already did

.../main/java/software/amazon/awssdk/transfer/s3/internal/progress/TransferProgressUpdater.java

zoewangg · 2024-05-02T18:13:45Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

@@ -118,6 +121,15 @@ private SplittingTransformer(AsyncResponseTransformer<ResponseT, ResultT> upstre
 Validate.notNull(maximumBufferSizeInBytes, "maximumBufferSizeInBytes");
 this.maximumBufferInBytes = Validate.isPositive(
 maximumBufferSizeInBytes, "maximumBufferSizeInBytes");
+
+ this.resultFuture.whenComplete((r, e) -> {


Let's add a test case for this

Thanks for adding a unit test for this. Can we add a end-to-end functional test (wiremock test) to verify the cancellation behavior?

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

zoewangg · 2024-05-02T21:32:21Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

 publisherToUpstream.send(byteBuffer).whenComplete((r, t) -> {
 if (t != null) {
 handleError(t);
 return;
 }
- subscription.request(1);
+ if (!isCancelled.get()) {


Do we need the check here assuming subscription is cancelled properly or will be cancelled at this point?
https://github.com/reactive-streams/reactive-streams-jvm

If a Subscription is cancelled its Subscriber MUST eventually stop being signaled.

Note "eventually" not "right away"

…zed tests that take tm as argument

zoewangg · 2024-05-02T22:42:46Z

Rerunning integ tests after fixing parameterized tests. They are passing on my machine now at least. 🤞🏼

sonarcloud · 2024-05-03T22:00:39Z

Quality Gate failed

Failed conditions
75.0% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

zoewangg · 2024-05-06T16:35:33Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

 publisherToUpstream.send(byteBuffer).whenComplete((r, t) -> {
 if (t != null) {
 handleError(t);
 return;
 }
- subscription.request(1);
+ if (!isCancelled.get()) {


What would happen if we don't check it here and continue to send request demand? It should be no-op right?

zoewangg · 2024-05-06T16:37:04Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

@@ -232,14 +267,23 @@ private class IndividualTransformer implements AsyncResponseTransformer<Response
 public CompletableFuture<ResponseT> prepare() {
 this.individualFuture = new CompletableFuture<>();
 if (preparedCalled.compareAndSet(false, true)) {
+ if (isCancelled.get()) {


Why do we need this logic here?

zoewangg · 2024-05-06T16:37:43Z

.../sdk-core/src/main/java/software/amazon/awssdk/core/internal/async/SplittingTransformer.java

 individualFuture.whenComplete((r, e) -> {
 if (isCancelled.get()) {
- handleCancelState();
+ handleSubscriptionCancel();


Can you remind me why we need to check isCancelled here for every individual future?

L-Applin added 2 commits May 2, 2024 12:59

- Forward cancellation from returnFuture in SplittingTransformer

0ddb24d

- Use content-length for range-get in TransferProgressUpdater, when multipart is enabled.

handleFutureCancel synchronization

2588752

L-Applin requested a review from a team as a code owner May 2, 2024 17:30

fix checkstyle

059fa2a

dagnir reviewed May 2, 2024

View reviewed changes

fix S3TransferManagerCopyIntegrationTest parameter for TmType

9d70e6a

zoewangg reviewed May 2, 2024

View reviewed changes

Fix integ tests by setting autoCloseArguments to false for parameteri…

86cf6e7

…zed tests that take tm as argument

fix S3TransferManagerCopyIntegrationTest parameter for TmType

f602822

zoewangg reviewed May 6, 2024

View reviewed changes

L-Applin merged commit 1660397 into feature/master/s3mpu May 21, 2024
15 of 19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for multipart integ test failure #5176

Fix for multipart integ test failure #5176

L-Applin commented May 2, 2024

dagnir left a comment

dagnir May 2, 2024

L-Applin May 2, 2024

dagnir May 2, 2024

L-Applin May 3, 2024

dagnir May 2, 2024

L-Applin May 2, 2024

zoewangg May 2, 2024

L-Applin May 3, 2024

zoewangg May 6, 2024

L-Applin commented May 2, 2024

zoewangg May 2, 2024

zoewangg May 6, 2024

zoewangg May 2, 2024

zoewangg commented May 2, 2024

sonarcloud bot commented May 3, 2024

zoewangg May 6, 2024

zoewangg May 6, 2024

zoewangg May 6, 2024

Fix for multipart integ test failure #5176

Fix for multipart integ test failure #5176

Conversation

L-Applin commented May 2, 2024

Motivation and Context

Modifications

Testing

dagnir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

L-Applin commented May 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoewangg commented May 2, 2024

sonarcloud bot commented May 3, 2024

Quality Gate failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment