feat: Add support for flexible checksums on streaming payloads #659

dayaffe · 2024-02-07T20:35:26Z

Issue #

awslabs/aws-sdk-swift#1319

Description of changes

FlexibleChecksumsRequestMiddleware
- Sets headers response for enabling chunked encoding
- Checks if a checksum is requested, if so it will either calculate it and set appropriate header (for normal payload), or set the necessary data for to calculate it during chunk writing for streams
- Calculation workflow will be skipped if user provides their own checksum or does not opt-in to use checksums
FlexibleChecksumsResponseMiddleware
- For normal payloads this will validate the checksum
- For streaming payloads stream will becoming a Validating stream that validates the checksum when the stream is read

Also:

Fixes a race condition in FoundationStreamBridge where data could be written to its output out-of-order.

Scope

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

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sichanyoo

Just a couple questions.

Sources/ClientRuntime/Networking/AwsChunked/AwsChunkedBufferedStream.swift

…rt 0.27.0, remove content-md5 todo, add tests

sichanyoo

Will approve once all swift lint warnings get addressed.

sichanyoo

Lgtm, after corrresponding PR in aws-sdk-swift passes all CIs, should be good to go.

Sources/ClientRuntime/Networking/AwsChunked/AwsChunkedBufferedStream.swift

Sources/ClientRuntime/Networking/AwsChunked/AwsChunkedReader.swift

Sources/ClientRuntime/Networking/Http/URLSession/FoundationStreamBridge.swift

Sources/ClientRuntime/Networking/Http/SdkHttpRequest.swift

Sources/ClientRuntime/Networking/AwsChunked/AwsChunkedReader.swift

Sources/ClientRuntime/Networking/AwsChunked/AwsChunkedUtil.swift

Sources/ClientRuntime/Networking/Http/Middlewares/FlexibleChecksumsRequestMiddleware.swift

jbelkins

I have fixed several issues in this feature, both in this PR and in awslabs/aws-sdk-swift#1329.

Some refactoring & performance improvements are required, but will be done in a separate PR.

…ws-chunked-encoding

…thy-swift into day/aws-chunked-encoding

jbelkins · 2024-03-20T23:54:08Z

Sources/ClientRuntime/Networking/AwsChunked/AWSChunkedStream.swift

+import AwsCommonRuntimeKit
+
+/// Reads data from the input stream, chunks it, and passes it out through its output stream.
+class AWSChunkedStream {


This used to be a protocol, I changed it to a concrete stream type that chunks its input and outputs an AWS-chunked stream.

jbelkins · 2024-03-20T23:56:03Z

Sources/ClientRuntime/Networking/AwsChunked/AWSChunkedStream.swift

+        self.outputStream.isSeekable
+    }
+
+    func read(upToCount count: Int) throws -> Data? {


read() and readAsync() will use the input stream and reader to create a new chunk once the output stream has been exhausted. This will prevent chunks from building up in memory well in advance of when they can be transmitted.

jbelkins · 2024-03-20T23:59:54Z

Sources/ClientRuntime/Networking/AwsChunked/AWSChunkedUtil.swift

+        }
+    }
+
+    public func setAwsChunkedBody(


This method now just wraps the body stream in a AWSChunkedStream when the request is chunked.

jbelkins · 2024-03-21T00:00:36Z

Sources/ClientRuntime/Networking/AwsChunked/AWSChunkedUtil.swift

+        }
+    }
+}
+


The chunk method that used to be here is removed; AWSChunkedStream and the CRT HTTP client now have their own code for this.

jbelkins · 2024-03-21T00:01:40Z

Sources/ClientRuntime/Networking/Http/CRT/CRTClientEngine.swift

@@ -165,6 +165,49 @@ public class CRTClientEngine: HTTPClient {
                do {
                    let stream = try connection.makeRequest(requestOptions: requestOptions)
                    try stream.activate()
+                    if request.isChunked {


This block of code now incorporates the code that used to be in the static method for chunking a body.

jbelkins · 2024-03-21T00:04:36Z

Sources/ClientRuntime/Networking/Http/URLSession/FoundationStreamBridge.swift

-    /// Actor used to isolate the stream status from multiple concurrent accesses.
-    actor ReadableStreamStatus {
+    /// A Logger for logging events.
+    private let logger: LogAgent


I put a Logger in this type, and I also fixed the issue where stream writes can get out-of-order.

jbelkins · 2024-03-21T00:05:49Z

Sources/ClientRuntime/Networking/Http/URLSession/URLSessionHTTPClient.swift

@@ -130,7 +130,7 @@ public final class URLSessionHTTPClient: HTTPClient {
        func urlSession(
            _ session: URLSession, dataTask: URLSessionDataTask, didReceive response: URLResponse
        ) async -> URLSession.ResponseDisposition {
-            logger.debug("urlSession(_:dataTask:didReceive:) called")
+            logger.debug("urlSession(_:dataTask:didReceive response:) called")


Logging changes in this type, plus I adjusted the buffer size for streams a bit.

jbelkins · 2024-03-21T00:06:46Z

Sources/ClientRuntime/Networking/Streaming/BufferedStream.swift

@@ -61,6 +61,13 @@ public class BufferedStream: Stream {
    /// Access this value only while `lock` is locked, to prevent simultaneous access.
    private var _buffer: Data

+    /// The number of bytes currently in the buffer, awaiting read.
+    public var bufferCount: Int {


Added this bufferCount property (to see if the stream is holding any data in-memory) and a little refactoring in this type.

jbelkins · 2024-03-21T00:08:28Z

Tests/ClientRuntimeTests/NetworkingTests/FlexibleChecksumsMiddlewareTests.swift

+import SmithyTestUtil
+@testable import ClientRuntime
+
+class FlexibleChecksumsMiddlewareTests: XCTestCase {


I reordered some of the steps in these tests since the middleware doesn't call next() at the very end of execution anymore.

jbelkins · 2024-03-21T00:10:39Z

Sources/ClientRuntime/Networking/Http/Middlewares/FlexibleChecksumsResponseMiddleware.swift

@@ -102,7 +111,7 @@ public struct FlexibleChecksumsResponseMiddleware<OperationStackOutput>: Middlew
            throw ClientError.dataNotFound("Cannot calculate the checksum of an empty body!")
        }

-        return try await next.handle(context: context, input: input)
+        return output


Rearranged where next.handle() is called in this middleware so that multiple GET requests are not made to the server.

dayaffe · 2024-03-21T14:02:26Z

Sources/ClientRuntime/Networking/Http/CRT/CRTClientEngine.swift

+                                    } else {
+                                        let currentChunkBody = awsChunkedStream.chunkedReader.getCurrentChunkBody()
+                                        if !currentChunkBody.isEmpty {
+                                            try await http1Stream.writeChunk(chunk: awsChunkedStream.chunkedReader.getCurrentChunk(), endOfStream: false)


lint warning

dayaffe added 6 commits February 7, 2024 14:06

add support for flexible checksums on Normal payloads

9be20ef

add support for Streaming flexible checksums

bcac65a

remove unnecessary function

b2f19e8

swiftlint

e51eea5

remove duplicate test file

d125440

comment out tests that arent ready

38012af

dayaffe requested review from sichanyoo and jbelkins February 8, 2024 20:41

dayaffe added 4 commits February 8, 2024 16:05

add ability to pass in priority list + tests from SEP

64b4e4f

change filestream to bufferedstream for unit testing

0a461ce

Merge branch 'main' into day/aws-chunked-encoding

66de094

change how response is updated

ee76d94

sichanyoo reviewed Feb 8, 2024

View reviewed changes

Sources/ClientRuntime/Networking/AwsChunked/AwsChunkedBufferedStream.swift Outdated Show resolved Hide resolved

Sources/ClientRuntime/Networking/AwsChunked/AwsChunkedBufferedStream.swift Outdated Show resolved Hide resolved

dayaffe added 9 commits February 9, 2024 16:01

add support for sha1/sha256 to flexible checksum streams, update to c…

dbfd9de

…rt 0.27.0, remove content-md5 todo, add tests

Merge branch 'main' into day/aws-chunked-encoding

efdd75d

Merge branch 'main' into day/aws-chunked-encoding

427e7e4

Merge branch 'main' into day/aws-chunked-encoding

09189f7

crt 0.27 requires logger change

c4bbcfc

fix logger errors

3e1b31d

remove logger variable

a4d26d7

update to crt 0.28.0 to fix typo

6d42b4f

fix tests that use modified MockOutput

b63aa96

sichanyoo requested changes Feb 12, 2024

View reviewed changes

dayaffe added 3 commits February 12, 2024 15:33

fix all swiftlint line is too long

7d5ba6d

fix swiftlint

a440282

last swiftlint

620b313

sichanyoo approved these changes Feb 12, 2024

View reviewed changes

jbelkins reviewed Feb 14, 2024

View reviewed changes

dayaffe added 2 commits February 15, 2024 12:01

try to fix CI by utilizing hasSpaceAvailable

5585d0b

try adding debugging statements

4df1b4c

jbelkins and others added 5 commits March 14, 2024 20:15

Merge branch 'main' into day/aws-chunked-encoding

c183e3a

Fix tests

130f9e8

Fix lint

0ad3c2a

URLSessionHTTPClient logging improvements

75f36b5

Merge branch 'main' into day/aws-chunked-encoding

871d456

jbelkins mentioned this pull request Mar 15, 2024

feat: Add support for flexible checksums on streaming payloads awslabs/aws-sdk-swift#1329

Merged

jbelkins approved these changes Mar 15, 2024

View reviewed changes

jbelkins added 7 commits March 18, 2024 11:32

Merge branch 'main' of github.com:smithy-lang/smithy-swift into day/a…

da906f1

…ws-chunked-encoding

Merge branch 'day/aws-chunked-encoding' of github.com:smithy-lang/smi…

5ac8b03

…thy-swift into day/aws-chunked-encoding

Move chunking logic into new composite AWSChunkedStream

1bd9cd3

Fix tests

443432e

Merge remote-tracking branch 'origin/main' into day/aws-chunked-encoding

8d54360

Cleanup on URLSession send

afc1f81

Chunking paced to sends on URLSession client; CRT client reverted

8c194fc

jbelkins reviewed Mar 20, 2024

View reviewed changes

Refactoring

028a133

jbelkins reviewed Mar 20, 2024

View reviewed changes

jbelkins reviewed Mar 21, 2024

View reviewed changes

dayaffe commented Mar 21, 2024

View reviewed changes

Fix Swiftlint issue

23c5ff3

jbelkins merged commit 4d0ffae into main Mar 21, 2024
12 checks passed

jbelkins deleted the day/aws-chunked-encoding branch March 21, 2024 15:21

AndrewFossAWS pushed a commit that referenced this pull request Mar 27, 2024

feat: Add support for flexible checksums on streaming payloads (#659)

1d0370e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for flexible checksums on streaming payloads #659

feat: Add support for flexible checksums on streaming payloads #659

dayaffe commented Feb 7, 2024 •

edited by jbelkins

Loading

sichanyoo left a comment

sichanyoo left a comment

sichanyoo left a comment •

edited

Loading

jbelkins left a comment

jbelkins Mar 20, 2024

jbelkins Mar 20, 2024

jbelkins Mar 20, 2024

jbelkins Mar 21, 2024

jbelkins Mar 21, 2024

jbelkins Mar 21, 2024

jbelkins Mar 21, 2024

jbelkins Mar 21, 2024

jbelkins Mar 21, 2024

jbelkins Mar 21, 2024

dayaffe Mar 21, 2024

feat: Add support for flexible checksums on streaming payloads #659

feat: Add support for flexible checksums on streaming payloads #659

Conversation

dayaffe commented Feb 7, 2024 • edited by jbelkins Loading

Issue #

Description of changes

Scope

sichanyoo left a comment

Choose a reason for hiding this comment

sichanyoo left a comment

Choose a reason for hiding this comment

sichanyoo left a comment • edited Loading

Choose a reason for hiding this comment

jbelkins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dayaffe commented Feb 7, 2024 •

edited by jbelkins

Loading

sichanyoo left a comment •

edited

Loading