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

[RFC] Add async/await proposal #406

Closed
Closed
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
197 changes: 197 additions & 0 deletions docs/async-await.md
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
Copy link
Collaborator

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 into HPACKHeaders 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 (over HPACKHeaders 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

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?

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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 or HPACKHeaders. 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 and HPACKHeaders cheaper -- I'm not sure how possible this is though.

Copy link
Collaborator

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.


/// The request's body
var body: Body?
fabianfett marked this conversation as resolved.
Show resolved Hide resolved

init(url: String) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only allowing url to be set in the init might be a little too restrictive. This pattern makes a lot of sense for large configuration objects etc. where adding a new init for each new piece of configuration quickly becomes a burden for maintainers.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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 👍

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Specially for the response object it matters because as we create it we know if there will be more bytes or not, so we can immediately construct the right "entity object", e.g. Empty or Strict() or a stream.

Representing the body as an abstract HTTPEntity which may be "strict" or not allows us then to create store the strict bytes via reference to it directly, we don't have to use the stream then at all other than lazy creating it when someone pulls from it.


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 .discardBytes() (but it's annoying))


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 .single(<bytes>) at all since setup and logic costs are comparable between this and an iterator... So mostly sharing as an "are we sure and have we considered alternatives?" note.

Copy link
Member Author

@fabianfett fabianfett Aug 16, 2021

Choose a reason for hiding this comment

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

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 .discardBytes() (but it's annoying))

I'm afraid we will have to do the same here.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably throw if it is known that body is not consumed yet?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should just crash.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Copy link
Contributor

Choose a reason for hiding this comment

The 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 {

Choose a reason for hiding this comment

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

Could there be a convenience API that makes a request a Task? eg.

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 await for the result in the same context.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Swift already has special syntax to create a child task through async let. What you propose would create a detached Task. I think this would probably make it too easy and users would create detached Tasks just because there is a method for it, even if they would actually want a child Task through async let.

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