async/await execute #524
async/await execute #524
Conversation
| guard | ||
| preparedRequest.body.canBeConsumedMultipleTimes, | ||
| let redirectState = redirectState, | ||
| let redirectURL = response.headers.extractRedirectTarget( | ||
| status: response.status, | ||
| originalURL: preparedRequest.url, | ||
| originalScheme: preparedRequest.poolKey.scheme | ||
| ) | ||
| else { | ||
| return response | ||
| } |
There was a problem hiding this comment.
I think this would be easier to read as an if.
| private func followRedirect( | ||
| redirectURL: URL, | ||
| redirectState: RedirectState, | ||
| request: HTTPClientRequest.Prepared, |
There was a problem hiding this comment.
the naming of request feels ambiguous here.
| let (method, headers, body) = transformRequestForRedirect( | ||
| from: request.url, | ||
| method: request.head.method, | ||
| headers: request.head.headers, | ||
| body: request.body, | ||
| to: redirectURL, | ||
| status: response.status | ||
| ) | ||
| var newRequest = HTTPClientRequest(url: redirectURL.absoluteString) | ||
| newRequest.method = method | ||
| newRequest.headers = headers | ||
| newRequest.body = body |
There was a problem hiding this comment.
I think this would look much nicer here and would make the code more readable
var newRequest = originalRequest.followingRedirect(to: redirectURL.absoluteString, redirectStatus: response.status)
| /// There is currently no good way to asynchronously cancel an object that is initiated inside the `body` closure of `with*Continuation`. | ||
| /// As a workaround we use `TransactionCancelHandler` which will take care of the race between instantiation of `Transaction` | ||
| /// in the `body` closure and cancelation from the `onCancel` closure of `with*Continuation`. | ||
| actor TransactionCancelHandler { | ||
| private enum State { | ||
| case initialised | ||
| case register(Transaction) | ||
| case cancelled | ||
| } | ||
|
|
||
| private var state: State = .initialised | ||
|
|
||
| init() {} | ||
|
|
||
| func registerTransaction(_ transaction: Transaction) { | ||
| switch self.state { | ||
| case .initialised: | ||
| self.state = .register(transaction) | ||
| case .cancelled: | ||
| transaction.cancel() | ||
| case .register: | ||
| preconditionFailure("transaction already set") | ||
| } | ||
| } | ||
|
|
||
| func cancel() { | ||
| switch self.state { | ||
| case .register(let bag): | ||
| self.state = .cancelled | ||
| bag.cancel() | ||
| case .cancelled: | ||
| break | ||
| case .initialised: | ||
| self.state = .cancelled | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I think we should probably pull this one out of this function... just put it on the file end with private access.
| responseContinuation: continuation | ||
| ) | ||
|
|
||
| _Concurrency.Task { |
There was a problem hiding this comment.
Comment re naming conflict on Task.
dnadoba
force-pushed
the
dn-async-await-execute
branch
from
6ad7bbc to
9568ecb
Compare
| redirectState: RedirectState? | ||
| ) async throws -> HTTPClientResponse { | ||
| let preparedRequest = try HTTPClientRequest.Prepared(request) | ||
| let response = try await executeWithoutFollowingRedirects(preparedRequest, deadline: deadline, logger: logger) |
There was a problem hiding this comment.
This method has nothing really to do with followingRedirects, but cancellation is its feature. WDYT about executeCancellable?
| status: response.status, | ||
| originalURL: preparedRequest.url, | ||
| originalScheme: preparedRequest.poolKey.scheme | ||
| ) { |
There was a problem hiding this comment.
This conditional is very hard to read. Can we split this into multiple lines with temporary variables to make it clearer?
Additionally, it's not clear that this logic is right. Not all redirects require re-consuming the body: for example, a 302 on POST converts to GET and drops the body, which is intentional.
There was a problem hiding this comment.
Good catch! We now check if the body of the new request can be send multiple times instead of looking at the original body. If we drop the body for the new request, the body will be nil and canBeConsumedMultipleTimes will return true.
| var redirectState = redirectState | ||
| try redirectState.redirect(to: redirectURL.absoluteString) | ||
|
|
||
| return try await self.executeAndFollowRedirectsIfNeeded( |
There was a problem hiding this comment.
Can I propose that we don't recurse through our redirects? Constructing a large call stack here is unnecessary, and it would be much better if we could iterate through them instead.
| }, onCancel: { | ||
| // `HTTPClient.Task` conflicts with Swift Concurrency Task and `Swift.Task` doesn't work | ||
| _Concurrency.Task { | ||
| await cancelHandler.cancel() |
There was a problem hiding this comment.
Do we need cancel to be awaitable? Constructing these child tasks is a bit awkward.
There was a problem hiding this comment.
We don't need it to be awaitable but we use an actor as our synchronisation primitive so all methods are automatically async/awaitable from outside.
I see two options:
- We could make the current
cancel()method private (and rename it) and add a newnonisolated cancel()method which would internal create theTaskand call theprivate cancel()method. - We could also rewrite
TransactionCancelHandlerto use a lock instead and make it a class.
WDYT?
There was a problem hiding this comment.
We can leave it as-is, I guess. I'm always a bit nervous about creating lots of tasks but we can always change it later.
dnadoba
force-pushed
the
dn-async-await-execute
branch
from
dfc66a1 to
0b4b559
Compare
|
@swift-server-bot test this please |
dnadoba
force-pushed
the
dn-async-await-execute
branch
from
011ab0c to
d0b8ac7
Compare
This is the last missing piece to actually execute a
HTTPClientRequestand await aHTTPClientResponse.It brings everything together but first preparing the
HTTPClientRequestand then executing theTransactionusing theHTTPConnectionPool.Manager. We optional follow redirects if the configurations allows it and theHTTPClientRequest.Bodycan be consumed multiple times.