Skip to content

Commit d4efe6e

Browse files
author
Rishabh Jain
committed
fix(s3): preserve original cause in ParallelMultipartDownloaderSubscriber.onError
onError cancelled the in-flight part requests before completing resultFuture with the failure cause. Each part future is wired to resultFuture via CompletableFutureUtils.forwardExceptionTo, so cancelling first raced a CancellationException onto resultFuture and discarded the original cause; callers observing the download's completion future saw a bare CancellationException with no root cause, and the trigger was never logged. Complete resultFuture with the original throwable first, then cancel the in-flight parts, and log the cause at debug level, matching the sibling ParallelPresignedUrlMultipartDownloaderSubscriber. Add a unit test covering the onError cause-preservation ordering.
1 parent f3ea4fc commit d4efe6e

3 files changed

Lines changed: 81 additions & 1 deletion

File tree

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"type": "bugfix",
3+
"category": "Amazon S3",
4+
"contributor": "rishabhjainps",
5+
"description": "Fixed an issue in multipart download where a failed download could surface a bare CancellationException with no root cause. ParallelMultipartDownloaderSubscriber.onError now completes the result future with the original throwable before cancelling in-flight part requests, matching the sibling ParallelPresignedUrlMultipartDownloaderSubscriber, and logs the cause at debug level."
6+
}

services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/multipart/ParallelMultipartDownloaderSubscriber.java

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -415,9 +415,12 @@ public void onError(Throwable t) {
415415
// Signal received from the publisher this is subscribed to
416416
// (in the case of file download, that's FileAsyncResponseTransformerPublisher)
417417
// Failed state, something really wrong has happened, cancel everything
418+
// Complete the result future with the original cause before cancelling in-flight parts.
419+
// Cancelling first races a CancellationException onto resultFuture and masks t.
420+
log.debug(() -> "Error in parallel multipart download", t);
421+
resultFuture.completeExceptionally(t);
418422
inFlightRequests.values().forEach(future -> future.cancel(true));
419423
inFlightRequests.clear();
420-
resultFuture.completeExceptionally(t);
421424
}
422425

423426
@Override
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
/*
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
16+
package software.amazon.awssdk.services.s3.internal.multipart;
17+
18+
import static org.assertj.core.api.Assertions.assertThat;
19+
import static org.mockito.ArgumentMatchers.any;
20+
import static org.mockito.Mockito.mock;
21+
import static org.mockito.Mockito.when;
22+
23+
import java.util.concurrent.CancellationException;
24+
import java.util.concurrent.CompletableFuture;
25+
import org.junit.jupiter.api.Test;
26+
import org.reactivestreams.Subscription;
27+
import software.amazon.awssdk.core.async.AsyncResponseTransformer;
28+
import software.amazon.awssdk.services.s3.S3AsyncClient;
29+
import software.amazon.awssdk.services.s3.model.GetObjectRequest;
30+
import software.amazon.awssdk.services.s3.model.GetObjectResponse;
31+
32+
/**
33+
* Unit tests for {@link ParallelMultipartDownloaderSubscriber}.
34+
*/
35+
class ParallelMultipartDownloaderSubscriberTest {
36+
37+
@Test
38+
void onError_withInFlightRequests_shouldCompleteResultFutureWithOriginalCause() {
39+
S3AsyncClient s3 = mock(S3AsyncClient.class);
40+
CompletableFuture<GetObjectResponse> resultFuture = new CompletableFuture<>();
41+
GetObjectRequest request = GetObjectRequest.builder()
42+
.bucket("test-bucket")
43+
.key("test-key")
44+
.build();
45+
ParallelMultipartDownloaderSubscriber subscriber =
46+
new ParallelMultipartDownloaderSubscriber(s3, request, resultFuture, 10);
47+
48+
CompletableFuture<GetObjectResponse> firstPartFuture = new CompletableFuture<>();
49+
CompletableFuture<GetObjectResponse> secondPartFuture = new CompletableFuture<>();
50+
when(s3.getObject(any(GetObjectRequest.class), any(AsyncResponseTransformer.class)))
51+
.thenReturn(firstPartFuture, secondPartFuture);
52+
53+
subscriber.onSubscribe(mock(Subscription.class));
54+
subscriber.onNext(mock(AsyncResponseTransformer.class));
55+
firstPartFuture.complete(GetObjectResponse.builder()
56+
.partsCount(3)
57+
.eTag("etag")
58+
.build());
59+
// Second part is now in flight and never completes on its own.
60+
subscriber.onNext(mock(AsyncResponseTransformer.class));
61+
62+
RuntimeException cause = new RuntimeException("original failure");
63+
subscriber.onError(cause);
64+
65+
// The caller-facing future must carry the original cause, not a CancellationException
66+
// raced onto it by the in-flight part cancellation.
67+
Throwable thrown = resultFuture.handle((r, t) -> t).join();
68+
assertThat(thrown).isNotInstanceOf(CancellationException.class);
69+
assertThat(thrown).isSameAs(cause);
70+
}
71+
}

0 commit comments

Comments
 (0)