Skip to content

Commit 62670e0

Browse files
committed
Fix error propogation
1 parent bd5fbb8 commit 62670e0

2 files changed

Lines changed: 50 additions & 4 deletions

File tree

services-custom/s3-transfer-manager/src/test/java/software/amazon/awssdk/transfer/s3/internal/S3JavaMultipartTransferProgressListenerTest.java

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,48 @@ void unknownContentLength_multiChunk_allCallbacksFire() {
449449
Mockito.verify(transferListenerMock, atLeastOnce()).bytesTransferred(ArgumentMatchers.any());
450450
}
451451

452+
/**
453+
* Verifies that when an unknown-content-length upload fails on the single-chunk path,
454+
* the completionFuture completes exceptionally and transferFailed fires.
455+
* This guards against regressions where the failure path in uploadInOneChunk does not
456+
* propagate the exception to returnFuture, causing the upload to hang indefinitely.
457+
*/
458+
@Test
459+
void unknownContentLength_singleChunk_failurePropagates() {
460+
S3AsyncClient s3Async = s3AsyncClient(true);
461+
462+
stubFor(put(urlPathEqualTo("/" + EXAMPLE_BUCKET + "/" + TEST_KEY))
463+
.willReturn(aResponse().withStatus(500).withBody(ERROR_BODY)));
464+
465+
S3TransferManager tm = new GenericS3TransferManager(s3Async, mock(UploadDirectoryHelper.class),
466+
mock(TransferManagerConfiguration.class),
467+
mock(DownloadDirectoryHelper.class));
468+
CaptureTransferListener transferListener = new CaptureTransferListener();
469+
TransferListener transferListenerMock = mock(TransferListener.class);
470+
471+
BlockingInputStreamAsyncRequestBody body = AsyncRequestBody.forBlockingInputStream(null);
472+
473+
Upload upload = tm.upload(u -> u.putObjectRequest(p -> p.bucket(EXAMPLE_BUCKET).key(TEST_KEY))
474+
.requestBody(body)
475+
.addTransferListener(transferListener)
476+
.addTransferListener(transferListenerMock)
477+
.build());
478+
479+
byte[] data = new byte[1024];
480+
body.writeInputStream(new ByteArrayInputStream(data));
481+
482+
assertThatExceptionOfType(CompletionException.class).isThrownBy(() -> upload.completionFuture().join());
483+
484+
assertTransferListenerCompletion(transferListener);
485+
assertThat(transferListener.isTransferInitiated()).isTrue();
486+
assertThat(transferListener.isTransferComplete()).isFalse();
487+
assertThat(transferListener.getExceptionCaught()).isNotNull();
488+
489+
Mockito.verify(transferListenerMock, times(1)).transferInitiated(ArgumentMatchers.any());
490+
Mockito.verify(transferListenerMock, times(0)).transferComplete(ArgumentMatchers.any());
491+
Mockito.verify(transferListenerMock, timeout(1000).times(1)).transferFailed(ArgumentMatchers.any());
492+
}
493+
452494
private static void assertTransferListenerCompletion(CaptureTransferListener transferListener) {
453495
Duration waitDuration = Duration.ofSeconds(5);
454496
assertTimeoutPreemptively(

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

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -168,14 +168,18 @@ void uploadInOneChunk(PutObjectRequest putObjectRequest,
168168
asyncRequestBody);
169169
CompletableFutureUtils.forwardExceptionTo(returnFuture, putObjectResponseCompletableFuture);
170170

171-
putObjectResponseCompletableFuture.thenAccept(response -> {
171+
putObjectResponseCompletableFuture.whenComplete((response, throwable) -> {
172+
if (throwable != null) {
173+
returnFuture.completeExceptionally(throwable);
174+
return;
175+
}
172176
if (reportProgress) {
173177
asyncRequestBody.contentLength().ifPresent(progressListener::subscriberOnNext);
174178
}
175179
// Always signal completion so that TransferProgressUpdater's endOfStreamFuture completes
176-
// and the TransferListener's transferComplete callback fires.
177-
// For unknown content length we don't know if it wil lbe one chunk or not ahead of time
178-
// and so don't set REPORT_PROGRESS_IN_SINGLE_CHUNK attribute.
180+
// and the TransferListener's transferComplete callback fires. This is needed for the
181+
// unknown-content-length single-chunk path where splitCloseable() bypasses the wrapper
182+
// body that would normally complete endOfStreamFuture via subscriberOnComplete.
179183
progressListener.subscriberOnComplete();
180184
returnFuture.complete(response);
181185
});

0 commit comments

Comments
 (0)