Skip to content

Commit 611dfd1

Browse files
zoewanggzoewangg
authored
Fixed an issue where an error was not surfaced if request failed halfway for a GetObject operation. (#5734)
1 parent e0ff3cc commit 611dfd1

File tree

4 files changed

+150
-10
lines changed

4 files changed

+150
-10
lines changed

‎.changes/next-release/bugfix-AWSCRTbasedS3client-906e1ea.json

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
{
2+
"type": "bugfix",
3+
"category": "AWS CRT-based S3 client",
4+
"contributor": "",
5+
"description": "Fixed an issue where an error was not surfaced if request failed halfway for a GetObject operation. See [#5631](https://github.com/aws/aws-sdk-java-v2/issues/5631)"
6+
}

‎services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapter.java

Lines changed: 61 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@
1515

1616
package software.amazon.awssdk.services.s3.internal.crt;
1717

18+
import static software.amazon.awssdk.core.http.HttpResponseHandler.X_AMZN_REQUEST_ID_HEADER_ALTERNATE;
19+
import static software.amazon.awssdk.core.http.HttpResponseHandler.X_AMZ_ID_2_HEADER;
1820
import static software.amazon.awssdk.utils.FunctionalUtils.runAndLogError;
1921

2022
import java.nio.ByteBuffer;
@@ -25,6 +27,9 @@
2527
import java.util.concurrent.TimeoutException;
2628
import software.amazon.awssdk.annotations.SdkInternalApi;
2729
import software.amazon.awssdk.annotations.SdkTestInternalApi;
30+
import software.amazon.awssdk.awscore.exception.AwsErrorDetails;
31+
import software.amazon.awssdk.awscore.exception.AwsServiceException;
32+
import software.amazon.awssdk.core.SdkBytes;
2833
import software.amazon.awssdk.core.async.listener.PublisherListener;
2934
import software.amazon.awssdk.core.exception.SdkClientException;
3035
import software.amazon.awssdk.crt.CRT;
@@ -34,6 +39,7 @@
3439
import software.amazon.awssdk.crt.s3.S3MetaRequestResponseHandler;
3540
import software.amazon.awssdk.http.SdkHttpResponse;
3641
import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler;
42+
import software.amazon.awssdk.services.s3.model.S3Exception;
3743
import software.amazon.awssdk.utils.Logger;
3844
import software.amazon.awssdk.utils.async.SimplePublisher;
3945

@@ -172,21 +178,66 @@ private void handleError(S3FinishedResponseContext context) {
172178
int responseStatus = context.getResponseStatus();
173179
byte[] errorPayload = context.getErrorPayload();
174180

175-
if (isErrorResponse(responseStatus) && errorPayload != null) {
176-
SdkHttpResponse.Builder errorResponse = populateSdkHttpResponse(SdkHttpResponse.builder(),
177-
responseStatus, headers);
178-
initiateResponseHandling(errorResponse.build());
179-
onErrorResponseComplete(errorPayload);
181+
if (isServiceError(responseStatus) && errorPayload != null) {
182+
handleServiceError(responseStatus, headers, errorPayload);
180183
} else {
181-
Throwable cause = context.getCause();
184+
handleIoError(context, crtCode);
185+
}
186+
}
187+
188+
private void handleIoError(S3FinishedResponseContext context, int crtCode) {
189+
Throwable cause = context.getCause();
190+
191+
SdkClientException sdkClientException =
192+
SdkClientException.create("Failed to send the request: " +
193+
CRT.awsErrorString(crtCode), cause);
194+
failResponseHandlerAndFuture(sdkClientException);
195+
}
196+
197+
private void handleServiceError(int responseStatus, HttpHeader[] headers, byte[] errorPayload) {
198+
SdkHttpResponse.Builder errorResponse = populateSdkHttpResponse(SdkHttpResponse.builder(),
199+
responseStatus, headers);
200+
if (requestFailedMidwayOfOtherError(responseStatus)) {
201+
AwsServiceException s3Exception = buildS3Exception(responseStatus, errorPayload, errorResponse);
182202

183203
SdkClientException sdkClientException =
184-
SdkClientException.create("Failed to send the request: " +
185-
CRT.awsErrorString(crtCode), cause);
186-
failResponseHandlerAndFuture(sdkClientException);
204+
SdkClientException.create("Request failed during the transfer due to an error returned from S3");
205+
s3Exception.addSuppressed(sdkClientException);
206+
failResponseHandlerAndFuture(s3Exception);
207+
} else {
208+
initiateResponseHandling(errorResponse.build());
209+
onErrorResponseComplete(errorPayload);
187210
}
188211
}
189212

213+
private static AwsServiceException buildS3Exception(int responseStatus,
214+
byte[] errorPayload,
215+
SdkHttpResponse.Builder errorResponse) {
216+
String requestId = errorResponse.firstMatchingHeader(X_AMZN_REQUEST_ID_HEADER_ALTERNATE)
217+
.orElse(null);
218+
String extendedRequestId = errorResponse.firstMatchingHeader(X_AMZ_ID_2_HEADER)
219+
.orElse(null);
220+
return S3Exception.builder()
221+
.requestId(requestId)
222+
.extendedRequestId(extendedRequestId)
223+
.statusCode(responseStatus)
224+
.message(errorResponse.statusText())
225+
.awsErrorDetails(AwsErrorDetails.builder()
226+
.sdkHttpResponse(errorResponse.build())
227+
.rawResponse(SdkBytes.fromByteArray(errorPayload))
228+
.build())
229+
.build();
230+
}
231+
232+
/**
233+
* Whether request failed midway or it failed due to a different error than the initial response.
234+
* For example, this could happen if an object got deleted after download was initiated (200
235+
* was received).
236+
*/
237+
private boolean requestFailedMidwayOfOtherError(int responseStatus) {
238+
return responseHandlingInitiated && initialHeadersResponse.statusCode() != responseStatus;
239+
}
240+
190241
private void initiateResponseHandling(SdkHttpResponse response) {
191242
if (!responseHandlingInitiated) {
192243
responseHandlingInitiated = true;
@@ -214,7 +265,7 @@ private void failResponseHandlerAndFuture(Throwable exception) {
214265
resultFuture.completeExceptionally(exception);
215266
}
216267

217-
private static boolean isErrorResponse(int responseStatus) {
268+
private static boolean isServiceError(int responseStatus) {
218269
return responseStatus != 0;
219270
}
220271

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtClientWiremockTest.java

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,25 @@
1818
import static com.github.tomakehurst.wiremock.client.WireMock.aResponse;
1919
import static com.github.tomakehurst.wiremock.client.WireMock.any;
2020
import static com.github.tomakehurst.wiremock.client.WireMock.anyUrl;
21+
import static com.github.tomakehurst.wiremock.client.WireMock.get;
22+
import static com.github.tomakehurst.wiremock.client.WireMock.head;
23+
import static com.github.tomakehurst.wiremock.client.WireMock.put;
2124
import static com.github.tomakehurst.wiremock.client.WireMock.stubFor;
25+
import static com.github.tomakehurst.wiremock.client.WireMock.urlEqualTo;
2226
import static org.assertj.core.api.Assertions.assertThat;
27+
import static org.assertj.core.api.Assertions.assertThatThrownBy;
2328
import static org.mockito.ArgumentMatchers.any;
2429
import static org.mockito.Mockito.verify;
30+
import static software.amazon.awssdk.core.http.HttpResponseHandler.X_AMZN_REQUEST_ID_HEADER_ALTERNATE;
31+
import static software.amazon.awssdk.core.http.HttpResponseHandler.X_AMZ_ID_2_HEADER;
2532

33+
import com.github.tomakehurst.wiremock.http.HttpHeader;
34+
import com.github.tomakehurst.wiremock.http.HttpHeaders;
2635
import com.github.tomakehurst.wiremock.junit5.WireMockRuntimeInfo;
2736
import com.github.tomakehurst.wiremock.junit5.WireMockTest;
37+
import com.github.tomakehurst.wiremock.stubbing.Scenario;
2838
import java.net.URI;
39+
import java.nio.charset.StandardCharsets;
2940
import java.util.concurrent.Executor;
3041
import org.junit.jupiter.api.AfterEach;
3142
import org.junit.jupiter.api.BeforeAll;
@@ -34,11 +45,15 @@
3445
import org.mockito.Mockito;
3546
import software.amazon.awssdk.auth.credentials.AwsBasicCredentials;
3647
import software.amazon.awssdk.auth.credentials.StaticCredentialsProvider;
48+
import software.amazon.awssdk.core.ResponseBytes;
49+
import software.amazon.awssdk.core.async.AsyncResponseTransformer;
3750
import software.amazon.awssdk.crt.CrtResource;
3851
import software.amazon.awssdk.crt.Log;
3952
import software.amazon.awssdk.regions.Region;
4053
import software.amazon.awssdk.services.s3.S3AsyncClient;
4154
import software.amazon.awssdk.services.s3.model.CompleteMultipartUploadResponse;
55+
import software.amazon.awssdk.services.s3.model.GetObjectResponse;
56+
import software.amazon.awssdk.services.s3.model.S3Exception;
4257

4358
/**
4459
* Tests to make sure all CRT resources are cleaned up
@@ -72,6 +87,8 @@ public static void setUpBeforeAll() {
7287
public void setup(WireMockRuntimeInfo wiremock) {
7388
s3AsyncClient = S3AsyncClient.crtBuilder()
7489
.region(Region.US_EAST_1)
90+
.minimumPartSizeInBytes(10L)
91+
.initialReadBufferSizeInBytes(5L)
7592
.endpointOverride(URI.create("http://localhost:" + wiremock.getHttpPort()))
7693
.credentialsProvider(
7794
StaticCredentialsProvider.create(AwsBasicCredentials.create("key", "secret")))
@@ -96,6 +113,42 @@ public void completeMultipartUpload_completeResponse() {
96113
assertThat(response.eTag()).isEqualTo(E_TAG);
97114
}
98115

116+
@Test
117+
public void requestFailedMidway_shouldThrowException() {
118+
HttpHeaders httpHeaders = new HttpHeaders(new HttpHeader("content-length", "12345676"),
119+
new HttpHeader("etag", E_TAG));
120+
stubFor(head(anyUrl()).willReturn(aResponse().withStatus(200)
121+
.withHeaders(httpHeaders)));
122+
123+
stubFor(get(anyUrl())
124+
.inScenario("SucceedThenFail")
125+
.whenScenarioStateIs(Scenario.STARTED)
126+
.willSetStateTo("first request")
127+
.willReturn(aResponse()
128+
.withStatus(200)
129+
.withBody("helloworld".getBytes(StandardCharsets.UTF_8))));
130+
131+
stubFor(get(anyUrl())
132+
.inScenario("SucceedThenFail")
133+
.whenScenarioStateIs("first request")
134+
.willSetStateTo("second request")
135+
.willReturn(aResponse()
136+
.withStatus(404)
137+
.withHeader(X_AMZ_ID_2_HEADER, "foo")
138+
.withHeader(X_AMZN_REQUEST_ID_HEADER_ALTERNATE, "bar")
139+
.withBody("".getBytes(StandardCharsets.UTF_8))));
140+
141+
assertThatThrownBy(() ->s3AsyncClient.getObject(
142+
r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes()).join())
143+
.satisfies(throwable -> {
144+
assertThat(throwable).hasRootCauseInstanceOf(S3Exception.class);
145+
S3Exception s3Exception = (S3Exception) throwable.getCause();
146+
assertThat(s3Exception.statusCode()).isEqualTo(404);
147+
assertThat(s3Exception.extendedRequestId()).isEqualTo("foo");
148+
assertThat(s3Exception.requestId()).isEqualTo("bar");
149+
});
150+
}
151+
99152
@Test
100153
void overrideResponseCompletionExecutor_shouldCompleteWithCustomExecutor(WireMockRuntimeInfo wiremock) {
101154

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crt/S3CrtResponseHandlerAdapterTest.java

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,10 +23,14 @@
2323
import static org.mockito.Mockito.times;
2424
import static org.mockito.Mockito.verify;
2525
import static org.mockito.Mockito.when;
26+
import static software.amazon.awssdk.core.http.HttpResponseHandler.X_AMZN_REQUEST_ID_HEADER_ALTERNATE;
27+
import static software.amazon.awssdk.core.http.HttpResponseHandler.X_AMZ_ID_2_HEADER;
2628

2729
import java.nio.ByteBuffer;
2830
import java.nio.charset.StandardCharsets;
2931
import java.time.Duration;
32+
import java.util.ArrayList;
33+
import java.util.List;
3034
import java.util.concurrent.CompletableFuture;
3135
import java.util.concurrent.TimeUnit;
3236
import org.junit.Before;
@@ -43,6 +47,7 @@
4347
import software.amazon.awssdk.crt.s3.S3MetaRequest;
4448
import software.amazon.awssdk.http.SdkHttpResponse;
4549
import software.amazon.awssdk.http.async.SdkAsyncHttpResponseHandler;
50+
import software.amazon.awssdk.services.s3.model.S3Exception;
4651

4752
@RunWith(MockitoJUnitRunner.class)
4853
public class S3CrtResponseHandlerAdapterTest {
@@ -155,6 +160,31 @@ public void requestFailed_shouldCompleteFutureExceptionally() {
155160
verify(s3MetaRequest).close();
156161
}
157162

163+
@Test
164+
public void requestFailedMidwayDueToServerError_shouldCompleteFutureWithS3Exceptionally() {
165+
responseHandlerAdapter.onResponseHeaders(200, new HttpHeader[0]);
166+
responseHandlerAdapter.onResponseBody(ByteBuffer.wrap("helloworld".getBytes(StandardCharsets.UTF_8)), 0, 0);
167+
168+
S3FinishedResponseContext errorContext = stubResponseContext(1, 404, "".getBytes());
169+
List<HttpHeader> headers = new ArrayList<>();
170+
headers.add(new HttpHeader(X_AMZN_REQUEST_ID_HEADER_ALTERNATE, "1234"));
171+
headers.add(new HttpHeader(X_AMZ_ID_2_HEADER, "5678"));
172+
173+
when(errorContext.getErrorHeaders()).thenReturn(headers.toArray(new HttpHeader[0]));
174+
175+
responseHandlerAdapter.onFinished(errorContext);
176+
Throwable actualException = sdkResponseHandler.error;
177+
assertThat(actualException).isInstanceOf(S3Exception.class);
178+
179+
assertThat(((S3Exception) actualException).statusCode()).isEqualTo(404);
180+
assertThat(((S3Exception) actualException).requestId()).isEqualTo("1234");
181+
assertThat(((S3Exception) actualException).extendedRequestId()).isEqualTo("5678");
182+
183+
assertThatThrownBy(() -> future.join()).hasRootCause(actualException);
184+
assertThat(future).isCompletedExceptionally();
185+
verify(s3MetaRequest).close();
186+
}
187+
158188
@Test
159189
public void requestFailedWithCause_shouldCompleteFutureExceptionallyWithCause() {
160190
RuntimeException cause = new RuntimeException("error");

0 commit comments

Comments
 (0)