-
Notifications
You must be signed in to change notification settings - Fork 117
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
[RFC] Add async/await proposal #406
Changes from 5 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,197 @@ | ||
# 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 `HTTPClientRequest` type | ||
|
||
The proposed new `HTTPClientRequest` shall be a simple swift structure. | ||
|
||
```swift | ||
struct HTTPClientRequest { | ||
/// The requests url. | ||
var url: String | ||
|
||
/// The request's HTTPMethod | ||
var method: HTTPMethod | ||
|
||
/// The request's headers | ||
var headers: HTTPHeaders | ||
|
||
/// 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 [`HTTPClient.Request`][HTTPClient.Request] 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 = HTTPClientRequest(url: "invalidurl") | ||
try await httpClient.execute(request, deadline: .now() + .seconds(3)) | ||
} catch { | ||
print(error) | ||
} | ||
``` | ||
|
||
If the library code throws from the `HTTPClientRequest` creation or the request invocation the user will, in normal use cases, handle the error in the same catch block. | ||
|
||
#### Request body streaming | ||
|
||
The new `HTTPClientRequest` 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 | ||
extension HTTPClientRequest { | ||
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 | ||
} | ||
} | ||
``` | ||
|
||
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 `HTTPClientRequest.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. | ||
|
||
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 `HTTPClientResponse` type | ||
|
||
The `HTTPClientResponse` looks more similar to the existing [`HTTPClient.Response`][HTTPClient.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. As with `HTTPClientRequest`, we dropped the namespacing on `HTTPClient` to allow easier discovery with autocompletion. | ||
|
||
```swift | ||
public struct HTTPClientResponse { | ||
/// 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 HTTPClientResponse { | ||
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 | ||
} | ||
} | ||
``` | ||
|
||
Note: The user must consume the `Body` stream or drop the `HTTPClientResponse`, to ensure that the | ||
internal HTTPClient connection can move forward. Dropping the `HTTPClientResponse` would lead to a | ||
request cancellation which in turn would lead to a close of an exisiting HTTP/1.1 connection. | ||
|
||
At a later point we could add trailers to the `HTTPClientResponse` 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 = HTTPClientRequest(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) | ||
} | ||
``` | ||
|
||
In such a case we can either throw an error or crash. | ||
|
||
### 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: HTTPClientRequest, deadline: NIODeadline) async throws -> HTTPClientResponse | ||
} | ||
``` | ||
|
||
Usage example: | ||
|
||
```swift | ||
var request = HTTPClientRequest(url: "https://swift.org") | ||
request.method = .POST | ||
request.headers = [ | ||
"content-type": "text/plain; charset=UTF-8" | ||
"x-my-fancy-header": "super-awesome" | ||
] | ||
request.body = .sequence("Hello world!".utf8) | ||
|
||
var response = try await client.execute(request, deadline: .now() + .seconds(5)) | ||
|
||
switch response.status { | ||
case .ok: | ||
let body = try await response.body.collect(maxBytes: 1024 * 1024) | ||
default: | ||
throw MyUnexpectedHTTPStatusError | ||
} | ||
``` | ||
|
||
- **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) | ||
} | ||
|
||
await Task.sleep(nanosecond: 500 * 1000 * 1000) // wait half a second | ||
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 | ||
|
||
[HTTPClient.Request]: https://github.com/swift-server/async-http-client/blob/main/Sources/AsyncHTTPClient/HTTPHandler.swift#L96-L318 | ||
[HTTPClient.Response]: https://github.com/swift-server/async-http-client/blob/main/Sources/AsyncHTTPClient/HTTPHandler.swift#L320-L364 |
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.
I was wondering whether
HTTPHeaders
is the right type here. For http/2 connections we'll need to normalise the header names to be lowercased and repackage them intoHPACKHeaders
which isn't cheap (in gRPC we used to use the http2-to-1 codecs and deal with http/1 types; we saw a pretty big increase in performance from using http2 directly, most of which came from not converting the headers). It'd be a shame to have to incur that cost here as well.Keeping
HTTPHeaders
(overHPACKHeaders
or a new AHC-provided headers type) is probably the preferred option at the moment since it's more or less a currency type but doesn't feel like an optimal solution.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.
@ktoso @weissi @PeterAdams-A wdyt?
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.
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 comment
The 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 comment
The 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 comment
The 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
HTTPHeaders
orHPACKHeaders
. This has the advantage of being able to change the backing type without breaking API if we decide to change our bias from http/1 to http/2 or vice versa. Of course this potentially makes things worse by adding a third headers type...Another would be making interop between
HTTPHeaders
andHPACKHeaders
cheaper -- I'm not sure how possible this is though.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.
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.