-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
Codecov Report
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
|
There was a problem hiding this 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
@usableFromInline | ||
struct AsyncLazySequence<Base: Sequence>: AsyncSequence { | ||
@usableFromInline typealias Element = Base.Element | ||
@usableFromInline struct AsyncIterator: AsyncIteratorProtocol { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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!
Co-authored-by: Gwynne Raskind <gwynne@vapor.codes>
975e33c
to
6eaef05
Compare
These changes are now available in 4.86.2 |
* 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
The existing implementation of adding an
AsyncSequence
toRequest.Body
had two issues:Sendable
guarantees and was unsafeThis fixes that
Resolves #3088