Add support for asynchronous body stream writing #2939

Joannis · 2023-01-19T12:49:33Z

Fixes Crash during upload #2930 - a crash when users try to write a body from within a task towards the ELF APIs.
Introduces a new API for writing chunked HTTP response bodies
Adds a helper that automatically managed failing and closing streams

… crashes when users try to write a body from within a task towards the ELF APIs

… into feature/jo-async-body-stream # Conflicts: # Tests/VaporTests/PipelineTests.swift

Joannis · 2023-02-21T14:26:19Z

@gwynne what's your verdict on the crash inside Request.Body's AsyncSequence implementation? Does that block this PR or not?

gwynne · 2023-02-21T16:06:44Z

@Joannis It is definitely a blocker. It's obviously a race condition, and not an easy one to hit.

gwynne · 2023-02-21T16:07:47Z

Have you been able to reproduce the failure with Thread Sanitizer enabled at all?

Joannis · 2023-02-21T16:09:21Z

Even though I didn't touch the crashing code?

Joannis · 2023-02-21T16:09:30Z

I'll check with TSAN later.

gwynne · 2023-02-21T16:12:21Z

Odds are very good you either 1) revealed an existing race condition we hadn't happened to trip over yet, or 2) introduced one in the new code that nonetheless happens to manifest deeper in the guts of things (a fairly strong probability pretty much by definition, given it's a precondition that's failing).

gwynne · 2023-02-21T16:12:45Z

(Or, even more painfully, both...)

tib · 2023-02-22T08:46:11Z

I suppose, it's an already existing issue, just had some trouble when using the Request.Body as an AsyncSequence. I'm very much looking forward for this PR & fix. Thank you. 🙏

Joannis · 2023-02-22T09:14:41Z

Found it, it's really stupid and really just a bug in the test

…ll inbound

0xTim

Thanks for this! This looks in a good shape. There are a couple of left over TODOs we need to resolve and all new public APIs will need DocC comments then we're good to merge

0xTim · 2023-03-07T00:13:52Z

Sources/Vapor/HTTP/BodyStream.swift

@@ -51,3 +51,14 @@ extension BodyStreamWriter {
        return promise.futureResult
    }
 }
+
+public protocol AsyncBodyStreamWriter {


Can you add DocC comments to these as they're public? Thanks!

Sources/Vapor/HTTP/Server/HTTPServerResponseEncoder.swift

0xTim · 2023-03-07T00:16:06Z

Sources/Vapor/Response/Response+Body.swift

+        public init(asyncStream: @escaping @Sendable (AsyncBodyStreamWriter) async throws -> (), count: Int, byteBufferAllocator: ByteBufferAllocator = ByteBufferAllocator()) {
+            self.byteBufferAllocator = byteBufferAllocator
+            self.storage = .asyncStream(.init(count: count, callback: asyncStream))
+        }
+
+        public init(asyncStream: @escaping @Sendable (AsyncBodyStreamWriter) async throws -> (), byteBufferAllocator: ByteBufferAllocator = ByteBufferAllocator()) {
+            self.init(asyncStream: asyncStream, count: -1, byteBufferAllocator: byteBufferAllocator)
+        }
+
+        public init(managedAsyncStream: @escaping @Sendable (AsyncBodyStreamWriter) async throws -> (), count: Int, byteBufferAllocator: ByteBufferAllocator = ByteBufferAllocator()) {
+            self.byteBufferAllocator = byteBufferAllocator
+            self.storage = .asyncStream(.init(count: count) { writer in
+                do {
+                    try await managedAsyncStream(writer)
+                    try await writer.write(.end)
+                } catch {
+                    try await writer.write(.error(error))
+                }
+            })
+        }
+
+        public init(managedAsyncStream: @escaping @Sendable (AsyncBodyStreamWriter) async throws -> (), byteBufferAllocator: ByteBufferAllocator = ByteBufferAllocator()) {


New public APIs will need DocC comments

0xTim · 2023-03-07T00:17:11Z

Sources/Vapor/Response/Response+Body.swift

@@ -159,6 +169,31 @@ extension Response {
            self.init(stream: stream, count: -1, byteBufferAllocator: byteBufferAllocator)
        }

+        public init(asyncStream: @escaping @Sendable (AsyncBodyStreamWriter) async throws -> (), count: Int, byteBufferAllocator: ByteBufferAllocator = ByteBufferAllocator()) {


I'm interested to see how Sendable works on 5.5 - but we're ready to bump to 5.6 if needed

Sendable does work in 5.5.

0xTim · 2023-03-07T00:17:38Z

Tests/VaporTests/PipelineTests.swift

@@ -56,6 +57,73 @@ final class PipelineTests: XCTestCase {
        try XCTAssertEqual(channel.readOutbound(as: ByteBuffer.self)?.string, "0\r\n\r\n")
        try XCTAssertNil(channel.readOutbound(as: ByteBuffer.self)?.string)
    }
+
+    #if compiler(>=5.7) && canImport(_Concurrency)


We should test on 5.6 as well

…-stream

0xTim

Thanks! Looks good to go, just a 5.6 compiler issue to fix

VaporBot · 2023-03-09T07:37:32Z

These changes are now available in 4.73.0

This reverts commit 07b8b5d.

Revert "Add support for asynchronous body stream writing (#2939)" This reverts commit 07b8b5d.

makaGeorge · 2023-04-12T11:21:32Z

Hi all, I was looking forward to this feature... Any news on if/when it will be merged into main? Thanks!

tib · 2023-04-12T13:40:37Z

Same here. I'd love to get some updates about this feature. Thank you. 🙏

gwynne · 2023-04-12T14:04:48Z

@Joannis, if you want to open a fresh PR from the branch (and update it to reflect 5.6 now being the minimum Swift version - don't need all the conditionals anymore!), we can re-review and make sure there aren't any outstanding concerns remaining.

Add support for asynchronous body stream writing, thereby also fixing…

3c97f93

… crashes when users try to write a body from within a task towards the ELF APIs

Joannis requested review from 0xTim and gwynne January 19, 2023 12:49

Fix compilation for Swift 5.6 inference

875881f

Joannis linked an issue Jan 19, 2023 that may be closed by this pull request

Response.Body(stream:) is not async/await ready #2929

Closed

Joannis added 4 commits January 31, 2023 10:01

Add support for asynchronous body stream writing, thereby also fixing…

fa426b9

… crashes when users try to write a body from within a task towards the ELF APIs

Fix compilation for Swift 5.6 inference

c672309

Fix older swift compilign tests

d300db7

Merge branch 'feature/jo-async-body-stream' of github.com:vapor/vapor…

d04d1ec

… into feature/jo-async-body-stream # Conflicts: # Tests/VaporTests/PipelineTests.swift

This was referenced Feb 15, 2023

Implement Server-Sent Events #2960

Open

Iterating over a request's body as an AsyncSequence can crash #2961

Closed

Merge branch 'main' into feature/jo-async-body-stream

11bdbba

Fix the test, as you cannot write a response while the request is sti…

6efc1c0

…ll inbound

Joannis self-assigned this Feb 22, 2023

Merge branch 'main' into feature/jo-async-body-stream

afbeaf2

0xTim requested changes Mar 7, 2023

View reviewed changes

Joannis added 2 commits March 7, 2023 15:44

Add docc comments, remove TODOs and converted them into an issue

19e4706

Merge remote-tracking branch 'origin/main' into feature/jo-async-body…

9629929

…-stream

Joannis requested review from 0xTim March 7, 2023 14:46

0xTim added the semver-minor Contains new API label Mar 7, 2023

0xTim approved these changes Mar 7, 2023

View reviewed changes

Joannis and others added 2 commits March 8, 2023 11:08

Be explicit in the return tpe that fails to infer on Swift 5.6

6b5764e

Merge branch 'main' into feature/jo-async-body-stream

168b822

Joannis merged commit 07b8b5d into main Mar 9, 2023

Joannis deleted the feature/jo-async-body-stream branch March 9, 2023 07:37

gwynne added a commit that referenced this pull request Mar 9, 2023

Revert "Add support for asynchronous body stream writing (#2939)"

a41f210

This reverts commit 07b8b5d.

gwynne mentioned this pull request Mar 9, 2023

Revert "Add support for asynchronous body stream writing" #2977

Merged

gwynne added a commit that referenced this pull request Mar 9, 2023

Revert "Add support for asynchronous body stream writing (#2939)"

24414c6

This reverts commit 07b8b5d.

gwynne added a commit that referenced this pull request Mar 9, 2023

Revert "Add support for asynchronous body stream writing" (#2977)

bac0fe3

Revert "Add support for asynchronous body stream writing (#2939)" This reverts commit 07b8b5d.

gwynne restored the feature/jo-async-body-stream branch March 9, 2023 10:10

grahamburgsma mentioned this pull request Jul 5, 2023

Add support for asynchronous body stream writing #2998

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for asynchronous body stream writing #2939

Add support for asynchronous body stream writing #2939

Joannis commented Jan 19, 2023

Joannis commented Feb 21, 2023

gwynne commented Feb 21, 2023

gwynne commented Feb 21, 2023

Joannis commented Feb 21, 2023

Joannis commented Feb 21, 2023

gwynne commented Feb 21, 2023

gwynne commented Feb 21, 2023

tib commented Feb 22, 2023

Joannis commented Feb 22, 2023

0xTim left a comment

0xTim Mar 7, 2023

0xTim Mar 7, 2023

0xTim Mar 7, 2023

Joannis Mar 7, 2023

0xTim Mar 7, 2023

0xTim left a comment

VaporBot commented Mar 9, 2023

makaGeorge commented Apr 12, 2023

tib commented Apr 12, 2023

gwynne commented Apr 12, 2023 •

edited

Add support for asynchronous body stream writing #2939

Add support for asynchronous body stream writing #2939

Conversation

Joannis commented Jan 19, 2023

Joannis commented Feb 21, 2023

gwynne commented Feb 21, 2023

gwynne commented Feb 21, 2023

Joannis commented Feb 21, 2023

Joannis commented Feb 21, 2023

gwynne commented Feb 21, 2023

gwynne commented Feb 21, 2023

tib commented Feb 22, 2023

Joannis commented Feb 22, 2023

0xTim left a comment

Choose a reason for hiding this comment

0xTim Mar 7, 2023

Choose a reason for hiding this comment

0xTim Mar 7, 2023

Choose a reason for hiding this comment

0xTim Mar 7, 2023

Choose a reason for hiding this comment

Joannis Mar 7, 2023

Choose a reason for hiding this comment

0xTim Mar 7, 2023

Choose a reason for hiding this comment

0xTim left a comment

Choose a reason for hiding this comment

VaporBot commented Mar 9, 2023

makaGeorge commented Apr 12, 2023

tib commented Apr 12, 2023

gwynne commented Apr 12, 2023 • edited

gwynne commented Apr 12, 2023 •

edited