Merge pull request #2842 from aws/dongie/crt-revert

Revert "Wait until response body or error body received to process re…

Author: dagnir

pom.xml

@@ -119,7 +119,7 @@
         <rxjava.version>2.2.21</rxjava.version>
         <commons-codec.verion>1.15</commons-codec.verion>
         <jmh.version>1.29</jmh.version>
-        <awscrt.version>0.29.2</awscrt.version>
+        <awscrt.version>0.29.1</awscrt.version>
 
         <!--Test dependencies -->
         <junit5.version>5.10.0</junit5.version>

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

@@ -45,13 +45,11 @@ public final class S3CrtResponseHandlerAdapter implements S3MetaRequestResponseH
 
     private final SimplePublisher<ByteBuffer> responsePublisher = new SimplePublisher<>();
 
-    private final SdkHttpResponse.Builder initialHeadersResponse = SdkHttpResponse.builder();
+    private final SdkHttpResponse.Builder respBuilder = SdkHttpResponse.builder();
     private volatile S3MetaRequest metaRequest;
 
     private final PublisherListener<S3MetaRequestProgress> progressListener;
 
-    private volatile boolean responseHandlingInitiated;
-
     public S3CrtResponseHandlerAdapter(CompletableFuture<Void> executeFuture,
                                        SdkAsyncHttpResponseHandler responseHandler,
                                        PublisherListener<S3MetaRequestProgress> progressListener) {
@@ -62,17 +60,17 @@ public S3CrtResponseHandlerAdapter(CompletableFuture<Void> executeFuture,
 
     @Override
     public void onResponseHeaders(int statusCode, HttpHeader[] headers) {
-        // Note, we cannot call responseHandler.onHeaders() here because the response status code and headers may not represent
-        // whether the request has succeeded or not (e.g. if this is for a HeadObject call that CRT calls under the hood). We
-        // need to rely on onResponseBody/onFinished being called to determine this.
-        populateSdkHttpResponse(initialHeadersResponse, statusCode, headers);
+        for (HttpHeader h : headers) {
+            respBuilder.appendHeader(h.getName(), h.getValue());
+        }
+
+        respBuilder.statusCode(statusCode);
+        responseHandler.onHeaders(respBuilder.build());
+        responseHandler.onStream(responsePublisher);
     }
 
     @Override
     public int onResponseBody(ByteBuffer bodyBytesIn, long objectRangeStart, long objectRangeEnd) {
-        // See reasoning in onResponseHeaders for why we call this here and not there.
-        initiateResponseHandling(initialHeadersResponse.build());
-
         if (bodyBytesIn == null) {
             failResponseHandlerAndFuture(new IllegalStateException("ByteBuffer delivered is null"));
             return 0;
@@ -100,10 +98,6 @@ public void onFinished(S3FinishedResponseContext context) {
         if (crtCode != CRT.AWS_CRT_SUCCESS) {
             handleError(context);
         } else {
-            // onResponseBody() is not invoked for responses with no content, so we may not have invoked
-            // SdkAsyncHttpResponseHandler#onHeaders yet.
-            // See also  reasoning in onResponseHeaders for why we call this here and not there.
-            initiateResponseHandling(initialHeadersResponse.build());
             onSuccessfulResponseComplete();
         }
     }
@@ -133,14 +127,10 @@ public void cancelRequest() {
 
     private void handleError(S3FinishedResponseContext context) {
         int crtCode = context.getErrorCode();
-        HttpHeader[] headers = context.getErrorHeaders();
         int responseStatus = context.getResponseStatus();
         byte[] errorPayload = context.getErrorPayload();
 
         if (isErrorResponse(responseStatus) && errorPayload != null) {
-            SdkHttpResponse.Builder errorResponse = populateSdkHttpResponse(SdkHttpResponse.builder(),
-                                                                            responseStatus, headers);
-            initiateResponseHandling(errorResponse.build());
             onErrorResponseComplete(errorPayload);
         } else {
             Throwable cause = context.getCause();
@@ -152,14 +142,6 @@ private void handleError(S3FinishedResponseContext context) {
         }
     }
 
-    private void initiateResponseHandling(SdkHttpResponse response) {
-        if (!responseHandlingInitiated) {
-            responseHandlingInitiated = true;
-            responseHandler.onHeaders(response);
-            responseHandler.onStream(responsePublisher);
-        }
-    }
-
     private void onErrorResponseComplete(byte[] errorPayload) {
         responsePublisher.send(ByteBuffer.wrap(errorPayload))
                          .thenRun(responsePublisher::complete)
@@ -194,17 +176,6 @@ public void onProgress(S3MetaRequestProgress progress) {
         this.progressListener.subscriberOnNext(progress);
     }
 
-    private static SdkHttpResponse.Builder populateSdkHttpResponse(SdkHttpResponse.Builder respBuilder,
-                                                                   int statusCode, HttpHeader[] headers) {
-        if (headers != null) {
-            for (HttpHeader h : headers) {
-                respBuilder.appendHeader(h.getName(), h.getValue());
-            }
-        }
-        respBuilder.statusCode(statusCode);
-        return respBuilder;
-    }
-
     private static class NoOpPublisherListener implements PublisherListener<S3MetaRequestProgress> {
     }
 }

services/s3/src/test/java/software/amazon/awssdk/services/s3/crt/CrtDownloadErrorTest.java

@@ -19,7 +19,6 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -31,6 +30,7 @@
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
@@ -59,7 +59,7 @@ public class S3CrtResponseHandlerAdapterTest {
     @Before
     public void setup() {
         future = new CompletableFuture<>();
-        sdkResponseHandler = spy(new TestResponseHandler());
+        sdkResponseHandler = new TestResponseHandler();
         responseHandlerAdapter = new S3CrtResponseHandlerAdapter(future,
                                                                  sdkResponseHandler,
                                                                  null);
@@ -75,20 +75,17 @@ public void successfulResponse_shouldCompleteFutureSuccessfully() throws Excepti
         int statusCode = 200;
         responseHandlerAdapter.onResponseHeaders(statusCode, httpHeaders);
 
-        stubOnResponseBody();
-
-        responseHandlerAdapter.onFinished(stubResponseContext(0, 0, null));
-        future.get(5, TimeUnit.SECONDS);
-
         SdkHttpResponse actualSdkHttpResponse = sdkResponseHandler.sdkHttpResponse;
         assertThat(actualSdkHttpResponse.statusCode()).isEqualTo(statusCode);
         assertThat(actualSdkHttpResponse.firstMatchingHeader("foo")).contains("1");
         assertThat(actualSdkHttpResponse.firstMatchingHeader("bar")).contains("2");
+        stubOnResponseBody();
 
+        responseHandlerAdapter.onFinished(stubResponseContext(0, 0, null));
+        future.get(5, TimeUnit.SECONDS);
         assertThat(future).isCompleted();
         verify(s3MetaRequest, times(2)).incrementReadWindow(11L);
         verify(s3MetaRequest).close();
-        verify(sdkResponseHandler).onHeaders(any(SdkHttpResponse.class));
     }
 
     @Test
@@ -106,25 +103,24 @@ public void nullByteBuffer_shouldCompleteFutureExceptionally() {
                                                                                                    + "null");
         assertThat(future).isCompletedExceptionally();
         verify(s3MetaRequest).close();
-        verify(sdkResponseHandler).onHeaders(any(SdkHttpResponse.class));
     }
 
     @Test
     public void errorResponse_shouldCompleteFutureSuccessfully() {
         int statusCode = 400;
         responseHandlerAdapter.onResponseHeaders(statusCode, new HttpHeader[0]);
 
-        byte[] errorPayload = "errorResponse".getBytes(StandardCharsets.UTF_8);
-        stubOnResponseBody();
-        responseHandlerAdapter.onFinished(stubResponseContext(1, statusCode, errorPayload));
-
         SdkHttpResponse actualSdkHttpResponse = sdkResponseHandler.sdkHttpResponse;
         assertThat(actualSdkHttpResponse.statusCode()).isEqualTo(400);
         assertThat(actualSdkHttpResponse.headers()).isEmpty();
 
+        byte[] errorPayload = "errorResponse".getBytes(StandardCharsets.UTF_8);
+        stubOnResponseBody();
+
+        responseHandlerAdapter.onFinished(stubResponseContext(1, statusCode, errorPayload));
+
         assertThat(future).isCompleted();
         verify(s3MetaRequest).close();
-        verify(sdkResponseHandler).onHeaders(any(SdkHttpResponse.class));
     }
 
     @Test
@@ -168,7 +164,7 @@ private void stubOnResponseBody() {
         responseHandlerAdapter.onResponseBody(ByteBuffer.wrap("helloworld2".getBytes()), 1, 2);
     }
 
-    private static class TestResponseHandler implements SdkAsyncHttpResponseHandler {
+    private static final class TestResponseHandler implements SdkAsyncHttpResponseHandler {
         private SdkHttpResponse sdkHttpResponse;
         private Throwable error;
         @Override