fix: Ensure all data is written before closing FoundationStreamBridge #713

jbelkins · 2024-05-04T21:07:34Z

Issue #

awslabs/aws-sdk-swift#1457

Description of changes

It was discovered that in certain circumstances, the FoundationStreamBridge can close while there is still data in the buffer left to stream. This potential bug was corrected by ensuring that both the buffer is empty AND the readable stream is closed before closing the output stream. (Before, we closed the output stream when the readable stream closed, without checking if there was data left to be streamed in the buffer.)
If there is unwritten data in the buffer and the Foundation OutputStream has room to receive, the remaining buffer data is written prior to reading the ReadableStream for more. This might increase the number of writes needed, but will ensure that available data is transferred to the Foundation OutputStream promptly.
Modifies the FoundationStreamBridge to take a separate buffer size for the buffer between the bound streams. This aids in testing by helping to create mismatches between the size of the buffer and the size of writes to the output stream.
Modified the FoundationStreamBridgeTests to set random values for both of the FoundationStreamBridge's internal buffers when performing test runs; this helps to create as many variations in operating parameters as possible.
Modified WriteCoordinator's perform() function to rethrow errors thrown by the block.
Refactored FoundationStreamBridge to be more straightforward & easier to follow.
Adds many comments on the content of FoundationStreamBridge to aid developers in working with the code.

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.

… output stream

jbelkins · 2024-05-04T22:05:20Z

Tests/ClientRuntimeTests/NetworkingTests/URLSession/FoundationStreamBridgeTests.swift

+// `FoundationStreamBridge` is not usable on Linux because it uses ObjC-interop features,
+// so this test is disabled there.
+
+#if os(iOS) || os(macOS) || os(watchOS) || os(tvOS) || os(visionOS)


Now that we don't use a Stream & RunLoop in FoundationStreamBridge, we can test it on all the Apple platforms & their simulators.

jbelkins · 2024-05-04T22:05:45Z

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

@@ -410,7 +410,7 @@ public final class URLSessionHTTPClient: HTTPClient {
            // that URLSession can stream its request body from.
            // Allow 16kb of in-memory buffer for request body streaming
            let streamBridge = requestStream.map {
-                FoundationStreamBridge(readableStream: $0, bufferSize: 16_384, logger: logger)
+                FoundationStreamBridge(readableStream: $0, bridgeBufferSize: 16_384, logger: logger)


Just changed the name of the param here.

jbelkins · 2024-05-05T22:43:35Z

Tests/ClientRuntimeTests/NetworkingTests/URLSession/FoundationStreamBridgeTests.swift

-
-        // The max size of the buffer for data being streamed.
-        let maxBufferSize = 2 * maxDataSize
+        let maxDataSize = 65_536  // 64 kb


Made the data for each test run a little bigger than before. Has little effect on the time needed to complete tests.

jbelkins · 2024-05-05T22:44:12Z

Tests/ClientRuntimeTests/NetworkingTests/URLSession/FoundationStreamBridgeTests.swift

@@ -39,7 +33,8 @@ class FoundationStreamBridgeTests: XCTestCase {

        // Create a temp buffer we can use to copy the input stream bytes
        // We are responsible for deallocating it
-        let tempBuffer = UnsafeMutablePointer<UInt8>.allocate(capacity: maxDataSize)
+        let tempBufferSize = maxDataSize
+        let tempBuffer = UnsafeMutablePointer<UInt8>.allocate(capacity: tempBufferSize)


Clarify code a bit by having a tempBufferSize to go with the tempBuffer.

jbelkins · 2024-05-05T22:45:42Z

Tests/ClientRuntimeTests/NetworkingTests/URLSession/FoundationStreamBridgeTests.swift

+            let bridgeBufferSize = Int.random(in: 1...(2 * dataSize))
+
+            // The bound stream buffer may be as small as 1 byte, up to 2x as big as the data size
+            let boundStreamBufferSize = Int.random(in: 1...(2 * dataSize))


Buffer sizes for the bridge buffer and the bound stream buffer are set separately. (Before, these two buffers were set to the same size.)

This helps to create scenarios where either buffer may be partially consumed during streaming.

jbelkins · 2024-05-05T22:46:42Z

Tests/ClientRuntimeTests/NetworkingTests/URLSession/FoundationStreamBridgeTests.swift


            // Fill a data buffer with dataSize random numbers
            let originalData = Data(bytes: randomBuffer, count: dataSize)

            // Create a stream bridge with our original data & open it
            let bufferedStream = BufferedStream(data: originalData, isClosed: true)
-            let subject = FoundationStreamBridge(readableStream: bufferedStream, bufferSize: bufferSize, logger: TestLogger())
+            let subject = FoundationStreamBridge(readableStream: bufferedStream, bridgeBufferSize: bridgeBufferSize, boundStreamBufferSize: boundStreamBufferSize, logger: TestLogger())


Create the FoundationStreamBridge under test with individual settings for bridge buffer size and bound stream buffer size.

jbelkins · 2024-05-05T22:50:17Z

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

+        bridgeBufferSize: Int = 65_536,
+        boundStreamBufferSize: Int? = nil,
+        logger: LogAgent
+    ) {


Added individual parameters for the size of the two buffers:

The "bridge buffer", which holds data between the ReadableStream and the Foundation OutputStream, and

the bound stream buffer, which holds data between the bound-pair OutputStream and InputStream Foundation streams.

jbelkins · 2024-05-05T22:53:01Z

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

-        /// Sets stream status to indicate the stream is empty.
-        func setReadableStreamIsEmpty() async {
-            readableStreamIsEmpty = true
-        }


This flag moved to the FoundationStreamBridge because having it on this actor is redundant - the WriteCoordinator's perform() method enforces exclusive access so having this flag on the Actor is just superfluous.

jbelkins · 2024-05-05T22:54:13Z

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

            }
+            self.task = task
+            _ = try await task.value


Simplified this method, and (on this line) made it rethrow any error that was thrown by its Task.

jbelkins · 2024-05-05T22:55:05Z

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

+///
+/// A pair of Foundation "bound streams" is created.  Data from the `ReadableStream` is transferred into the Foundation
+/// `OutputStream` until the `ReadableStream` is closed and all data has been read from it.  The Foundation
+/// `InputStream` is exposed as a property, and may be used to stream the data to other components.


Lots of added commentary in this file to help guide the reader through how this type works.

dayaffe

These are great changes, but to be sure they don't break anything could you create a corresponding branch in aws-sdk-swift and run the full integration test suite against it?

jbelkins · 2024-05-06T15:34:12Z

These are great changes, but to be sure they don't break anything could you create a corresponding branch in aws-sdk-swift and run the full integration test suite against it?

See awslabs/aws-sdk-swift#1478

sichanyoo · 2024-05-06T16:42:19Z

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

+/// Reads data from a smithy-swift native `ReadableStream` and streams the data through to a Foundation `InputStream`.
+///
+/// A pair of Foundation "bound streams" is created.  Data from the `ReadableStream` is transferred into the Foundation
+/// `OutputStream` until the `ReadableStream` is closed and all data has been read from it.  The Foundation


Q: Line 22 says data is read from ReadableStream to a InputStream. Line 24&25 say data is read from ReadableStream to OutputStream. So is OutputStream a stepping stone (e.g., ReadableStream => OutputStream => InputStream)? If so, why does there need to be one?

Line 22 is supposed to summarize where the data comes in to the FoundationStreamBridge and where it goes out. Line 24/25 are individual steps within the FoundationStreamBridge.

Overall, data flows like this:

Customer events or data > ClientRuntime.ReadableStream > FoundationStreamBridge buffer > Foundation.OutputStream > bound stream buffer > Foundation.InputStream > URLRequest streaming body

We need the OutputStream because a pair of bound streams is how you get data through to the InputStream that we need for streaming our URLRequest body. Put another way, we can't put data into an InputStream unless we do so with a bound OutputStream.

…amBridge (#713)" This reverts commit 5325fcb.

jbelkins added 6 commits May 4, 2024 15:50

fix: Ensure all data is written before closing FoundationStreamBridge…

cad5cb1

… output stream

Merge remote-tracking branch 'origin/main' into jbe/fsb_correct_close

267f8bd

Adjust test

c3f4fed

Change test compile condition to match code

1a619b3

Move stream error access to queue

7b0bd10

Make WriteCoordinator throw errors in perform() block

6e960e3

jbelkins marked this pull request as ready for review May 4, 2024 21:50

jbelkins requested review from dayaffe and sichanyoo May 4, 2024 21:50

jbelkins commented May 4, 2024

View reviewed changes

jbelkins commented May 5, 2024

View reviewed changes

Improved comments

02aaf4a

dayaffe approved these changes May 6, 2024

View reviewed changes

jbelkins mentioned this pull request May 6, 2024

[for testing only] Ensure all data is written before closing FoundationStreamBridge awslabs/aws-sdk-swift#1478

Closed

Merge branch 'main' into jbe/fsb_correct_close

e3efa54

sichanyoo reviewed May 6, 2024

View reviewed changes

sichanyoo self-requested a review May 6, 2024 18:17

sichanyoo approved these changes May 6, 2024

View reviewed changes

jbelkins merged commit 5325fcb into main May 6, 2024
15 checks passed

jbelkins deleted the jbe/fsb_correct_close branch May 6, 2024 18:17

jbelkins added a commit that referenced this pull request May 6, 2024

Revert "fix: Ensure all data is written before closing FoundationStre…

bb18592

…amBridge (#713)" This reverts commit 5325fcb.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Ensure all data is written before closing FoundationStreamBridge #713

fix: Ensure all data is written before closing FoundationStreamBridge #713

jbelkins commented May 4, 2024 •

edited

Loading

jbelkins May 4, 2024

jbelkins May 4, 2024

jbelkins May 5, 2024

jbelkins May 5, 2024

jbelkins May 5, 2024

jbelkins May 5, 2024

jbelkins May 5, 2024 •

edited

Loading

jbelkins May 5, 2024

jbelkins May 5, 2024

jbelkins May 5, 2024

dayaffe left a comment •

edited

Loading

jbelkins commented May 6, 2024

sichanyoo May 6, 2024

jbelkins May 6, 2024

fix: Ensure all data is written before closing FoundationStreamBridge #713

fix: Ensure all data is written before closing FoundationStreamBridge #713

Conversation

jbelkins commented May 4, 2024 • edited Loading

Issue #

Description of changes

Scope

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

jbelkins May 5, 2024 • edited Loading

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 left a comment • edited Loading

Choose a reason for hiding this comment

jbelkins commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbelkins commented May 4, 2024 •

edited

Loading

jbelkins May 5, 2024 •

edited

Loading

dayaffe left a comment •

edited

Loading