-
Notifications
You must be signed in to change notification settings - Fork 125
[RFC] Add async/await proposal #406
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
Changes from 2 commits
7ec050b
e2f078e
f96567c
110ab5e
992f006
22d8b45
9687247
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,168 @@ | ||
# Proposal: Async-await support | ||
|
||
## Introduction | ||
|
||
With the introduction of [async/await][SE-0296] in Swift 5.5, it is now possible to write asynchronous code without the need for callbacks. | ||
|
||
Language support for [`AsyncSequence`][SE-0298] also allows for writing functions that return values over time. | ||
|
||
We would like to explore how we could offer APIs that make use of these new language features to allow users to run HTTPRequest using these new idioms. | ||
|
||
This proposal describes what these APIs could look like and explores some of the potential usability concerns. | ||
|
||
## Proposed API additions | ||
|
||
### New `AsyncRequest` type | ||
|
||
The proposed new `AsyncRequest` shall be a simple swift structure. | ||
|
||
```swift | ||
struct AsyncRequest { | ||
/// The requests url. | ||
var url: String | ||
|
||
/// The request's HTTPMethod | ||
var method: HTTPMethod | ||
|
||
/// The request's headers | ||
var headers: HTTPHeaders | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was wondering whether Keeping There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ktoso @weissi @PeterAdams-A wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In gRPC did you not just move the overhead to HTTP/1? Ie, made HTTP/2 the default and now have to convert when on HTTP/1? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, exactly. It's a much clearer cut decision for gRPC since http/2 is much more common; so much so that the cost of doing the extra work for http/1 doesn't really matter. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Presumably at the point where this is currently created we don't know which HTTP version we'll be using? Would we need to do some sort of delayed creation? I think we need to provide and easy version to use, even if we also have a fast option. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's right, we need the headers before we know what version we're using. One option is AHC providing its own headers type which could wrap either an Another would be making interop between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could make the third way a protocol with default implementations for both such that if we get the right one it's close to free, and wrong one is just the conversion we'd have to do anyway. |
||
|
||
/// The request's body | ||
var body: Body? | ||
fabianfett marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
init(url: String) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only allowing I don't know if that's justified here since this is likely to be used much more frequently so we should make it easy to use even if the maintenance cost is higher. Moreover I suspect there's much less scope for adding new properties to the request. On the other hand, the "convenience" APIs might cover this just fine. |
||
self.url = url | ||
self.method = .GET | ||
self.headers = .init() | ||
self.body = .none | ||
} | ||
} | ||
``` | ||
|
||
A notable change from the current [`HTTPRequest`][HTTPRequest] is that the url is not of type `URL`. This makes the creation of a request non throwing. Existing issues regarding current API: | ||
|
||
- [HTTPClient.Request.url is a let constant][issue-395] | ||
ktoso marked this conversation as resolved.
Show resolved
Hide resolved
|
||
- [refactor to make request non-throwing](https://github.com/swift-server/async-http-client/pull/56) | ||
- [improve request validation](https://github.com/swift-server/async-http-client/pull/67) | ||
|
||
The url validation will become part of the normal request validation that occurs when the request is scheduled on the `HTTPClient`. If the user supplies a request with an invalid url, the http client, will reject the request. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I like to validate earlier rather than later so personally would mildly push back against this change... but since there's been enough issues about it that I guess it's fair to follow up and change... ok then 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want the URL validation to be part of request creation, you could create a wrapper which initializes a request from a URL object but creates the request using a String. That should give you a reasonable guarantee that HTTPClient will not reject the request due to it being an invalid URL. It would mean parsing the URL twice, but that’s relatively cheap, and there are potentially ways to avoid it if we get shared Strings in the standard library at some point (they are already part of the ABI). If the shared String is owned by a URL object, the URL parser can assume it is already a valid URL and recover the parsed URL object from the owner without parsing the String again or allocating fresh storage. |
||
|
||
In normal try/catch flows this should not change the control flow: | ||
|
||
```swift | ||
do { | ||
var request = AsyncRequest(url: "invalidurl") | ||
try await httpClient.execute(request, deadline: .now() + .seconds(3)) | ||
} catch { | ||
print(error) | ||
} | ||
``` | ||
|
||
If the library code throws from the AsyncRequest creation or the request invocation the user will, in normal use cases, handle the error in the same catch block. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
but we just said it can't? A bit weird phrasing I guess? |
||
|
||
#### Body streaming | ||
|
||
The new AsyncRequest has a new body type, that is wrapper around an internal enum. This allows us to evolve this type for use-cases that we are not aware of today. | ||
|
||
```swift | ||
public struct Body { | ||
fabianfett marked this conversation as resolved.
Show resolved
Hide resolved
|
||
static func bytes<S: Sequence>(_ sequence: S) -> Body where S.Element == UInt8 | ||
|
||
static func stream<S: AsyncSequence>(_ sequence: S) -> Body where S.Element == ByteBuffer | ||
|
||
static func stream<S: AsyncSequence>(_ sequence: S) -> Body where S.Element == UInt8 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do you imagine to store the sequence in the internal enum? I think we would probably need to erase it with some thing like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The way we make it fast in case of So long as the transformer is inlinable we can have it specialized by the caller. We can then wrap it in our type erasing logic (simply a callback of type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A proof of concept has been implemented here... https://github.com/fabianfett/async-http-client/blob/ff-async/Sources/AsyncHTTPClient/AsyncAwait/AsyncRequest.swift#L74-L87 In the static func stream<S: AsyncSequence>(_ sequence: S) -> Body where S.Element == UInt8 {
var iterator = sequence.makeAsyncIterator()
let body = self.init(.asyncSequence { allocator -> IOData? in
var buffer = allocator.buffer(capacity: 1024) // TODO: Magic number
while buffer.writableBytes > 0, let byte = try await iterator.next() {
buffer.writeInteger(byte)
}
if buffer.readableBytes > 0 {
return .byteBuffer(buffer)
}
return nil
})
return body
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah that's what any stream (IO related at least) will have to do and I've mentioned this repeatedly to folks working on IO and streams. The problem with our async story is that it's not clear who and where must do this chunking. So we may double-chunk un-necessarily... Say the "async bytes" abstraction https://developer.apple.com/documentation/foundation/filehandle/3766681-bytes does exactly that. So feeding a request from a file by doing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have a choice, we have to do it. The reason we have to do it is that we have to type-erase the sequence, and the moment we do that the compiler can't see through the loop. This means we can only iterate on the chunks. The better move, I think, would be for Of course, now we get into an argument about what that buffer type is. 😉 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah agreed that we have to do it here. Interesting idea with offering a chunkedBytes to allow more control to users there, might be worth a radar :-) |
||
} | ||
``` | ||
|
||
The main difference to today's `Request.Body` type is the lack of a `StreamWriter` for streaming scenarios. The existing StreamWriter offered the user an API to write into (thus the user was in control of when writing happened). The new `AsyncRequest.Body` uses `AsyncSequence`s to stream requests. By iterating over the provided AsyncSequence, the HTTPClient is in control when writes happen, and can ask for more data efficiently. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't lose this capability actually, we just express it the right way (well, "right" in my brain at least) now :-) One should always accept "some" async sequence. The way this sequence is produced matters not to the consumer of it (this API). Now, the way a "writer" is expressed, is by the end-user creating an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do lose it: gRPC is proposing an alternative API that keeps the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the entire reason behind the yielded values which we made sure to include for exactly this reason. You're saying that it's dependent on the intermediate buffer being full or not, and not on the actual "end that does the write being full or not" but that's just the reality of dealing with buffered streams -- and any stream may be buffered so that's it, you won't get the exact same semantics but these are the semantics you work with in stream APIs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a minor issue implementing let stream = StreamWriter()
stream.write(buffer1)
stream.write(buffer2)
stream.write(nil) I need the class StreamWriter {
var write: (ByteBuffer?) -> ()
let stream: AsyncStream<ByteBuffer>
init() {
var write: ((ByteBuffer?) -> ())?
self.stream = .init { cont in
write = { buffer in
guard let buffer = buffer else { cont.finish(); return }
cont.yield(buffer)
}
}
self.write = write!
}
} I had to force unwrap the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume we'd fix the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't we discussing removing it and replacing with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We know about that limitation and now ticketified the specific case for it: https://bugs.swift.org/browse/SR-15076 Either way, even if offering the StreamWriter API -- that's totally fine -- let's please not forget trying to improve the capabilities of the new concurrency types. Some of them are rough around the edges and need more love :) |
||
|
||
Using the `AsyncSequence` from the Swift standard library as our upload stream mechanism dramatically reduces the learning curve for new users. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I propose to reword the above about "losing" the writer approach, since we still have it -- thanks to AsyncStream, this way this is only simplification without feature loss 👍 |
||
|
||
### New `AsyncResponse` type | ||
fabianfett marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
The `AsyncResponse` looks more similar to the existing `Response` type. The biggest difference is again the `body` property, which is now an `AsyncSequence` of `ByteBuffer`s instead of a single optional `ByteBuffer?`. This will make every response on AsyncHTTPClient streaming by default. | ||
|
||
```swift | ||
public struct AsyncResponse { | ||
/// the used http version | ||
public var version: HTTPVersion | ||
/// the http response status | ||
public var status: HTTPResponseStatus | ||
/// the response headers | ||
public var headers: HTTPHeaders | ||
/// the response payload as an AsyncSequence | ||
public var body: Body | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might be worth exploring an akka http entity inspired design here before we commit to this API. Representing the body as an abstract The entire topic of "there's a stream of bytes, but noone subscribed to it" is quite hellish in general. What is the approach of this implementation about it? If we're doing a proper back-pressured stream it means that not reading it is directly connected to not reading from the wire, which may or may not be what the user intended. In akka we had to invent subscription timeouts to resolve such zombie streams. In case I'm not clear about the case: response has streamed body; we're a nice streaming API and people expect back-pressure via such stream API. If we never consume this stream, what happens? (In Akka's case we were very loud that this is user error and one has to do Note though that I don't yet have good performance intuitions about AsyncSequences... perhaps it is not worth optimizing for knowing that there's a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'm afraid we will have to do the same here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We talked offline a little bit -- I think we're in a better place here, because the request's dropping can cancel (and does, but was not documented in this document), which leaves us without the nasty "hanging stream" issue 👍 The must always consume rule is fine, we'll have to be explicit about it 👍 |
||
} | ||
|
||
extension AsyncResponse { | ||
public struct Body: AsyncSequence { | ||
public typealias Element = ByteBuffer | ||
public typealias AsyncIterator = Iterator | ||
|
||
public struct Iterator: AsyncIteratorProtocol { | ||
public typealias Element = ByteBuffer | ||
|
||
public func next() async throws -> ByteBuffer? | ||
} | ||
|
||
public func makeAsyncIterator() -> Iterator | ||
} | ||
} | ||
``` | ||
|
||
At a later point we could add trailers to the AsyncResponse as effectful properties. | ||
|
||
```swift | ||
public var trailers: HTTPHeaders? { async throws } | ||
``` | ||
ktoso marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
However we will need to make sure that the user has consumed the body stream completely before, calling the trailers, because otherwise we might run into a situation from which we can not progress forward: | ||
|
||
```swift | ||
do { | ||
var request = AsyncRequest(url: "https://swift.org/") | ||
let response = try await httpClient.execute(request, deadline: .now() + .seconds(3)) | ||
|
||
var trailers = try await response.trailers // can not move forward since body must be consumed before. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably throw if it is known that body is not consumed yet? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should just crash. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I'm on the crash side as well. If the user consumes the trailers before the body, it will never be right. Crash seems to be the right tradeoff. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that's true, unlikely it'll be sometimes correct, and even if it is people really must fix it. 👍 |
||
} catch { | ||
print(error) | ||
} | ||
|
||
``` | ||
|
||
### New invocation | ||
fabianfett marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
The new way to invoke a request shall look like this: | ||
fabianfett marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
```swift | ||
extension HTTPClient { | ||
func execute(_ request: AsyncRequest, deadline: NIODeadline) async throws -> AsyncResponse | ||
} | ||
``` | ||
|
||
- **Why do we have a deadline in the function signature?** | ||
Task deadlines are not part of the Swift 5.5 release. However we think that they are an important tool to not overload the http client accidentally. For this reason we will not default them. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While we can't commit to making them happen for Swift 6, it's a clear and important thing we want to support in swift concurrency. So I agree not doing another "our own" thing until then is the right call here. |
||
- **What happened to the Logger?** We will use Task locals to propagate the logger metadata. @slashmo and @ktoso are currently working on this. | ||
- **How does cancellation work?** Cancellation works by cancelling the surrounding task: | ||
|
||
```swift | ||
let task = Task { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could there be a convenience API that makes a request a let task = httpClient.task(with: request)
// …
let reponse = await task.value This is a syntactic sugar that allows the HTTP task to be handled by another function. That is, we don’t need to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Swift already has special syntax to create a child task through |
||
let response = try await httpClient.execute(request, deadline: .distantFuture) | ||
} | ||
|
||
Task.sleep(500 * 1000 * 1000) // wait half a second | ||
fabianfett marked this conversation as resolved.
Show resolved
Hide resolved
|
||
task.cancel() // cancel the task after half a second | ||
``` | ||
|
||
- **What happens with all the other configuration options?** Currently users can configure a TLSConfiguration on a request. This API doesn't expose this option. We hope to create a three layer model in the future. For this reason, we currently don't want to add per request configuration on the request invocation. More info can be found in the issue: [RFC: design suggestion: Make this a "3-tier library"][issue-392] | ||
ktoso marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
[SE-0296]: https://github.com/apple/swift-evolution/blob/main/proposals/0296-async-await.md | ||
[SE-0298]: https://github.com/apple/swift-evolution/blob/main/proposals/0298-asyncsequence.md | ||
[SE-0310]: https://github.com/apple/swift-evolution/blob/main/proposals/0310-effectful-readonly-properties.md | ||
[SE-0314]: https://github.com/apple/swift-evolution/blob/main/proposals/0314-async-stream.md | ||
|
||
[issue-392]: https://github.com/swift-server/async-http-client/issues/392 | ||
[issue-395]: https://github.com/swift-server/async-http-client/issues/395 | ||
|
||
[HTTPRequest]: https://github.com/swift-server/async-http-client/blob/main/Sources/AsyncHTTPClient/HTTPHandler.swift#L96-L318 |
Uh oh!
There was an error while loading. Please reload this page.