Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Async Request Body actually work #3096

Merged
merged 9 commits into from Nov 7, 2023
Merged

Make Async Request Body actually work #3096

merged 9 commits into from Nov 7, 2023

Conversation

0xTim
Copy link
Member

@0xTim 0xTim commented Nov 2, 2023

The existing implementation of adding an AsyncSequence to Request.Body had two issues:

  • it didn't ensure code was being called from the correct event loop which broke Sendable guarantees and was unsafe
  • it would hit a precondition failure in the implementation if backpressure was triggered because the initial state was not accounted for

This fixes that

Resolves #3088

@0xTim 0xTim requested a review from gwynne as a code owner November 2, 2023 19:31
@0xTim 0xTim added the semver-patch Internal changes only label Nov 2, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #3096 (4dd939f) into main (3bf4e73) will increase coverage by 0.10%.
Report is 2 commits behind head on main.
The diff coverage is 95.83%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3096      +/-   ##
==========================================
+ Coverage   76.23%   76.34%   +0.10%     
==========================================
  Files         211      211              
  Lines        7904     7918      +14     
==========================================
+ Hits         6026     6045      +19     
+ Misses       1878     1873       -5     
Files Coverage Δ
...ces/Vapor/Authentication/AuthenticationCache.swift 84.84% <100.00%> (+0.97%) ⬆️
Sources/Vapor/Authentication/Authenticator.swift 57.89% <ø> (ø)
Sources/Vapor/Authentication/GuardMiddleware.swift 100.00% <ø> (ø)
...rces/Vapor/Authentication/RedirectMiddleware.swift 83.33% <ø> (ø)
Sources/Vapor/Request/Request+BodyStream.swift 96.66% <100.00%> (+0.37%) ⬆️
...ources/Vapor/Concurrency/Request+Concurrency.swift 59.79% <87.50%> (+8.14%) ⬆️

Copy link
Contributor

@MahdiBM MahdiBM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks as good as it could be to me

Comment on lines 86 to 89
@usableFromInline
struct AsyncLazySequence<Base: Sequence>: AsyncSequence {
@usableFromInline typealias Element = Base.Element
@usableFromInline struct AsyncIterator: AsyncIteratorProtocol {
Copy link
Contributor

@MahdiBM MahdiBM Nov 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just use AsyncAlgorithms's var async, but not sure if it's fine to add a dependency to that just for this test.
That also takes Task cancellation into account which we don't want, but this is in tests and that's not going to happen anyway.
https://github.com/apple/swift-async-algorithms/blob/5bbdcc1d37990cca524852c5a3924c02fa2da983/Sources/AsyncAlgorithms/AsyncSyncSequence.swift#L46C5-L46C5

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly I think adding an entire "this uses code from ..." mess etc. etc. and all this format boilerplate is a hell of a lot of overkill for a test to begin with. The logic here is just not involved enough to merit the effort. I mean for pity's sake, look at what happens when you yank all the noise, it's literally nothing but another layer of boilerplate:

struct AsyncLazySequence<Base: Sequence & Sendable>: AsyncSequence, Sendable {
    typealias Element = Base.Element
    struct AsyncIterator: AsyncIteratorProtocol, Sendable {
        var iter: Base.Iterator
        mutating func next() async throws -> Base.Element? { self.iter.next() }
    }
    var base: Base
    func makeAsyncIterator() -> AsyncIterator { .init(iter: self.base.makeIterator()) }
}
extension Sequence where Self: Sendable { var async: AsyncLazySequence<Self> { .init(base: self) } }

(Also can I just point out that the @available annotations are completely meaningless in this context? Like, triply so. And the @inlinable annotations are equally worthless in a test, since it's not optimizing compilation.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I cut it down significantly. There are a few other options and the best way to do this would be Async Algorithm but it's a test and this was the quickest easiest thing!

@0xTim 0xTim enabled auto-merge (squash) November 7, 2023 13:07
@0xTim 0xTim merged commit d682e05 into main Nov 7, 2023
17 checks passed
@0xTim 0xTim deleted the async-request-body branch November 7, 2023 13:14
@penny-for-vapor
Copy link

These changes are now available in 4.86.2

keniwhat pushed a commit to keniwhat/vapor that referenced this pull request Jan 19, 2024
* main: (44 commits)
  Update routing-kit version (vapor#3131)
  Use `singleton` `EventLoopGroup` (vapor#3128)
  Additional tests fixes
  Fix for Apple changing things without warning.
  Add missing availability annotations in URI tests
  Merge pull request from GHSA-r6r4-5pr8-gjcp
  Fix setting public folder for `FileMiddleware` when using bundles (vapor#3113)
  Consistently use the value from `X-Request-Id` as the request's ID when present (vapor#3117)
  Fix encoding and decoding of HTTPHeaders (vapor#3116)
  Add fully async entrypoints (vapor#3114)
  Bring back AsyncCommands (vapor#3109)
  General warnings and tests cleanup (vapor#3107)
  Add public initializer for `XCTHTTPRequest` (vapor#3106)
  [skip ci] Update dependabot.yml
  [skip ci] Fixup sponsors workflow
  Make Async Request Body actually work (vapor#3096)
  Create a thread pool of System.coreCount rather than 64 when initializing an Application (vapor#3092)
  Modernize sponsors automation workflow (vapor#3098)
  Bump the dependencies group with 2 updates (vapor#3099)
  Update README with new Sponsor (vapor#3101)
  ...

# Conflicts:
#	Package.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Internal changes only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Streaming request crash
4 participants