Fix Trailer based Http Checksum for Async Request body with variable chunk size #3380

joviegas · 2022-08-25T08:37:20Z

Motivation and Context

Fix Trailer based Http Checksum for Async Request body created from File

Modifications

The Non last chunks in ChecksumCalculatingAsyncRequestBody.ChecksumCalculatingSubscriber was not appending the Chunk size and Carriage return to the chunks.
The FileAsynRequest Body was not handled , this sends the data in chunks of 16KB, thus the Content-length is updated to consider additional bytes due to size and Carriage return
Added ChunkBuffer class that will Buffer the incoming bytes until it reaches buffer_size
Added flatMapIterable to wrapped netty stream of data such that we map it to ChunkBuffer
wrapped.flatMapIterable(this::buffer).subscribe(new ChecksumCalculatingSubscriber(s, sdkChecksum, trailerHeader, totalBytes));

Testing

Screenshots (if appropriate)

Integ test with File Async body Greater and less than 16K added both for sync and async.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

License

I confirm that this pull request can be released under the Apache 2 license

dagnir · 2022-08-25T16:11:26Z

...ain/java/software/amazon/awssdk/core/internal/async/ChecksumCalculatingAsyncRequestBody.java

+            ByteBuffer checksumAppendedBuffer = ByteBuffer.allocate(contentChunk.remaining());
+            checksumAppendedBuffer.put(contentChunk);
+            checksumAppendedBuffer.flip();


I don't understand the need for this. Why create a copy of contentChunk?

.../amazon/awssdk/core/internal/interceptor/AsyncRequestBodyHttpChecksumTrailerInterceptor.java

...main/java/software/amazon/awssdk/core/internal/io/AwsUnsignedChunkedEncodingInputStream.java

utils/src/main/java/software/amazon/awssdk/utils/async/ByteBufferingSubscriber.java

joviegas · 2022-09-09T15:48:03Z

...test/java/software/amazon/awssdk/services/AsyncRequestBodyFlexibleChecksumInTrailerTest.java

@@ -28,12 +28,16 @@
 import static org.assertj.core.api.AssertionsForClassTypes.assertThat;
 import static software.amazon.awssdk.http.Header.CONTENT_LENGTH;
 import static software.amazon.awssdk.http.Header.CONTENT_TYPE;
+import static software.amazon.awssdk.testutils.FileUtils.fixedLengthInKbFileWithRandomOrFixedCharacters;


I will raise another PR for converting this to Junit5 .

…d size

.changes/next-release/bugfix-AWSSDKforJavav2-5ecdce1.json

...rc/it/java/software/amazon/awssdk/services/s3/checksum/AsyncHttpChecksumIntegrationTest.java

zoewangg · 2022-09-08T00:38:17Z

...ain/java/software/amazon/awssdk/core/internal/async/ChecksumCalculatingAsyncRequestBody.java

@@ -41,6 +42,8 @@
 @SdkInternalApi
 public class ChecksumCalculatingAsyncRequestBody implements AsyncRequestBody {

+    public static final int DEFAULT_CHUNK_SIZE = 16 * 1024;


Instead of making it a public member in this class, can we create a constant class to put all checksum related chunk size constants?

...ain/java/software/amazon/awssdk/core/internal/async/ChecksumCalculatingAsyncRequestBody.java

services/s3/src/test/java/software/amazon/awssdk/services/s3/utils/ChecksumUtils.java

...ain/java/software/amazon/awssdk/core/internal/async/ChecksumCalculatingAsyncRequestBody.java

…ng currentBuffer

…ut Object

zoewangg · 2022-09-14T19:41:05Z

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

+
+    public List<ByteBuffer> getBufferedList() {
+        if (currentBuffer == null) {
+            throw new IllegalStateException("");


currentBuffer should never be null, right?

zoewangg · 2022-09-14T21:16:22Z

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

+ * Class that will buffer incoming BufferBytes of totalBytes length to chunks of bufferSize*
+ */
+@SdkInternalApi
+public final class ChunkBuffer {


As discussed offline, we should probably make this thread safe

sonarqubecloud · 2022-09-15T01:02:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
26 Code Smells

94.5% Coverage
1.9% Duplication

sonarqubecloud · 2022-09-15T01:02:36Z

Please retry analysis of this Pull-Request directly on SonarCloud.

zoewangg

Can we trigger the test suites if we haven't already

…55c7c4cb3 Pull request: release <- staging/94641a3e-f760-4614-bf42-f4d55c7c4cb3

joviegas requested a review from a team as a code owner August 25, 2022 08:37

joviegas force-pushed the flexibleChecksumBug branch from e0c75c3 to 42a3d05 Compare August 25, 2022 08:46

Fix Trailer based Http Checksum for Async Request body created from File

f6a7a43

joviegas force-pushed the flexibleChecksumBug branch from 42a3d05 to f6a7a43 Compare August 25, 2022 15:34

dagnir reviewed Aug 25, 2022

View reviewed changes

joviegas force-pushed the flexibleChecksumBug branch 5 times, most recently from f3ac7de to ac0e9cf Compare September 4, 2022 23:49

Updated Fix to support AsyncRequestFileBody of variable chunk size

f02605b

joviegas force-pushed the flexibleChecksumBug branch from ac0e9cf to f02605b Compare September 5, 2022 18:18

dagnir reviewed Sep 7, 2022

View reviewed changes

...main/java/software/amazon/awssdk/core/internal/io/AwsUnsignedChunkedEncodingInputStream.java Outdated Show resolved Hide resolved

utils/src/main/java/software/amazon/awssdk/utils/async/ByteBufferingSubscriber.java Outdated Show resolved Hide resolved

joviegas force-pushed the flexibleChecksumBug branch 4 times, most recently from 9662737 to 423686c Compare September 9, 2022 15:33

joviegas commented Sep 9, 2022

View reviewed changes

Reusing .flatMapIterable API of SDKPublisher to create Chunks of fixe…

b662515

…d size

joviegas force-pushed the flexibleChecksumBug branch from 423686c to b662515 Compare September 9, 2022 17:40

zoewangg reviewed Sep 9, 2022

View reviewed changes

joviegas force-pushed the flexibleChecksumBug branch 3 times, most recently from a16a18b to 1dcd041 Compare September 13, 2022 19:54

Adding a separate class ChunkBuffer to handle Multiple thread accessi…

7eba119

…ng currentBuffer

joviegas force-pushed the flexibleChecksumBug branch from 1dcd041 to 7eba119 Compare September 13, 2022 21:41

Creating a SynchronousBuffer for mapper , also handling ZeroByte s3 p…

71aab4f

…ut Object

dagnir approved these changes Sep 14, 2022

View reviewed changes

joviegas changed the title ~~Fix Trailer based Http Checksum for Async Request body created from File~~ Fix Trailer based Http Checksum for Async Request body with variable chunk size Sep 14, 2022

zoewangg reviewed Sep 14, 2022

View reviewed changes

Made ChunkBuffer synchronized

e671beb

zoewangg approved these changes Sep 15, 2022

View reviewed changes

joviegas merged commit 62daf53 into master Sep 15, 2022

joviegas deleted the flexibleChecksumBug branch September 19, 2022 18:13

aws-sdk-java-automation added a commit that referenced this pull request Oct 24, 2024

Merge pull request #3380 from aws/staging/94641a3e-f760-4614-bf42-f4d…

f048671

…55c7c4cb3 Pull request: release <- staging/94641a3e-f760-4614-bf42-f4d55c7c4cb3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix Trailer based Http Checksum for Async Request body with variable chunk size #3380

Fix Trailer based Http Checksum for Async Request body with variable chunk size #3380

Uh oh!

joviegas commented Aug 25, 2022 •

edited

Loading

Uh oh!

dagnir Aug 25, 2022

Uh oh!

joviegas Sep 5, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joviegas Sep 9, 2022

Uh oh!

Uh oh!

Uh oh!

zoewangg Sep 8, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zoewangg Sep 14, 2022

Uh oh!

zoewangg Sep 14, 2022

Uh oh!

sonarqubecloud bot commented Sep 15, 2022

Uh oh!

sonarqubecloud bot commented Sep 15, 2022

Uh oh!

zoewangg left a comment

Uh oh!

Uh oh!

Fix Trailer based Http Checksum for Async Request body with variable chunk size #3380

Fix Trailer based Http Checksum for Async Request body with variable chunk size #3380

Uh oh!

Conversation

joviegas commented Aug 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation and Context

Modifications

Testing

Screenshots (if appropriate)

Types of changes

License

Uh oh!

dagnir Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

joviegas Sep 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joviegas Sep 9, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zoewangg Sep 8, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zoewangg Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

zoewangg Sep 14, 2022

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Sep 15, 2022

Uh oh!

sonarqubecloud bot commented Sep 15, 2022

Uh oh!

zoewangg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joviegas commented Aug 25, 2022 •

edited

Loading