Add HTTPScheduledRequest and HTTPExecutingRequest #384

fabianfett · 2021-06-28T10:48:25Z

Motivation

In the future we don't want to modify the ChannelPipeline for each request.
Our current API supports generic Response types: HTTPClient.Task<Response>.
- For this reason our current TaskHandler<Delegate: HTTPClientResponseDelegate> is generic over the request's delegate.
If we don't want to modify our ChannelPipeline for each request in the future we need remove the generic response types somehow. The only way to achieve this is by boxing our current API in a protocol, that we use as an existential.

Changes

Introducing three protocols:
- HTTPClientTask (I hate this name. Suggestions are highly welcome. Especially since the potential conflict with the current type HTTPClient.Task<Response>): An abstraction of a request, that handles back-pressure for sending the request body as well as receiving the response body.
- HTTPTaskExecutor a protocol that can be used from the HTTPClientTask abstracting away functionality that will normally be implemented by a ChannelHandler
- HTTPTaskQueuer a protocol that can be used from the HTTPClientTask abstracting away functionality that will normally be implemented by a ConnectionPool
An implementation of the HTTPClientTask called RequestBag. This shows how we can support our current API, with the new HTTPClientTask protocol

Notes for reviewers:

We should first agree on the three new protocols and then progress to the RequestBag implementation. I propose those changes not as two PRs to show the usage of those protocols right away.
This code is not triggered by any existing component. It purely exist to allow follow up prs and will enabled eventually.
For a new async/await compatible API, we would add a new implement that conforms to HTTPClientTask

Sources/AsyncHTTPClient/ConnectionPool/HTTPExecutingRequest.swift

Lukasa · 2021-06-29T14:25:11Z

Sources/AsyncHTTPClient/RequestBag.swift

+    func requestWasQueued(_ queuer: HTTPRequestScheduler) {
+        self.stateLock.withLock {
+            guard case .initialized = self._state else {
+                // There might be a race between `requestWasQueued` and `willExecuteRequest`. For


Why might there be a race?

Lukasa · 2021-06-29T14:25:30Z

Sources/AsyncHTTPClient/RequestBag.swift

+            case .initialized:
+                self._state = .executing(writer, .initialized, .initialized)
+                return true
+            case .queued:


This can be coalesced with the state above.

Lukasa · 2021-06-29T15:11:01Z

Sources/AsyncHTTPClient/RequestBag.swift

+    }
+
+    func didSendRequestPart(_ part: IOData) {
+        guard self.task.eventLoop.inEventLoop else {


Still not done.

Lukasa · 2021-06-29T15:11:06Z

Sources/AsyncHTTPClient/RequestBag.swift

+    }
+
+    func didSendRequest() {
+        guard self.task.eventLoop.inEventLoop else {


Lukasa · 2021-06-29T15:11:10Z

Sources/AsyncHTTPClient/RequestBag.swift

+    func failTask(_ error: Error) {
+        self.task.promise.fail(error)
+
+        guard self.task.eventLoop.inEventLoop else {


Lukasa · 2021-06-29T15:11:22Z

Sources/AsyncHTTPClient/RequestBag.swift

+            }
+        }
+
+        self.delegate.didReceiveError(task: self.task, error)


Do you want to fail the promise first, or after this?

Sources/AsyncHTTPClient/RequestBag.swift

Sources/AsyncHTTPClient/ConnectionPool/HTTPExecutingRequest.swift

glbrntt · 2021-07-02T14:49:33Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPExecutingRequest.swift

+    /// after `requestHeadSent` was called.
+    var requestHead: HTTPRequestHead { get }
+
+    /// The maximal `TimeAmount` that is allowed to pass between reads from the Channel.


Presumably fireChannelRead events rather than issuing read events?

Sources/AsyncHTTPClient/ConnectionPool/HTTPExecutingRequest.swift

glbrntt · 2021-07-02T14:52:18Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPExecutingRequest.swift

+    /// This will be called on the Channel's EventLoop. Do **not block** during your execution!
+    ///
+    /// - Returns: A bool indicating if the request should really be started. Return false if the request has already been cancelled.
+    ///            If the request is cancelled after this method call `executor.cancel()` to stop request execution.


Who's responsible for calling executor.cancel()? It seems like the caller is responsible, if so I'm not sure this comment belongs here.

I tried to be more specific.

glbrntt · 2021-07-02T14:56:15Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPExecutingRequest.swift

+    /// Resume request streaming
+    ///
+    /// This will be called on the Channel's EventLoop. Do **not block** during your execution!
+    func resumeRequestBodyStream()


Why do we need to distinguish between start and resume here? Does it make a difference to the implementor?

glbrntt · 2021-07-02T14:57:28Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPExecutingRequest.swift

+    /// This will be called on the Channel's EventLoop. Do **not block** during your execution!
+    func requestHeadSent(_: HTTPRequestHead)
+
+    /// Start request streaming


Should we document when we expect these to be called (and how often). I.e. can the implementor safely assume that startRequestBodyStream() will be called at most once?

glbrntt · 2021-07-02T15:07:26Z

Sources/AsyncHTTPClient/RequestBag+StateMachine.swift

+    }
+
+    mutating func writeNextRequestPart(_ part: IOData, taskEventLoop: EventLoop) -> WriteAction {
+        // this method is invoked with bodyPart and returns a future that signals that


this seems out of place.

Sources/AsyncHTTPClient/RequestBag+StateMachine.swift

glbrntt · 2021-07-02T15:19:05Z

Sources/AsyncHTTPClient/RequestBag+StateMachine.swift

+                return .consume(byteBuffer)
+            }
+
+            // buffer is empty, wait for more


Doesn't seem like we're waiting for more!

Sources/AsyncHTTPClient/RequestBag+StateMachine.swift

glbrntt · 2021-07-05T14:31:20Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPExecutingRequest.swift

+    func receiveResponseEnd()
+
+    func fail(_ error: Error)


nit: these should be documented too

glbrntt · 2021-07-06T12:46:28Z

Sources/AsyncHTTPClient/RequestBag.swift

+
+    // MARK: - Request -
+
+    private func willExecuteRequest0(_ executor: HTTPRequestExecutor) {


Should we assert we're on the event loop here?

glbrntt · 2021-07-06T12:48:30Z

Sources/AsyncHTTPClient/RequestBag.swift

+    // MARK: - Request -
+
+    private func willExecuteRequest0(_ executor: HTTPRequestExecutor) {
+        guard self.state.willExecuteRequest(executor) else {


nit: a guard only for the else feels like an anti-pattern, if !self.state.willExecuteRequest(executor) { ... } seems more idiomatic.

glbrntt · 2021-07-06T13:00:55Z

Sources/AsyncHTTPClient/RequestBag.swift

+        guard forwardToDelegate else { return }
+
+        self.delegate.didReceiveHead(task: self.task, head)
+            .hop(to: self.task.eventLoop)


I think hop(to:) allocates if you're not already on the correct event loop, a consumeMoreBodyData which did the if-in-event-loop-else dance for us might help save some allocations.

Chatted about offline: A if-in-event-loop-else dance™️ also incurs an allocation, if we are not on the correct eventLoop. The closure context allocates.

glbrntt · 2021-07-06T13:06:14Z

Sources/AsyncHTTPClient/RequestBag.swift

+                    case .failure(let error):
+                        self.fail(error)


Why are we failing here?

fabianfett · 2021-07-06T15:07:10Z

Adds a new error type.

glbrntt

LGTM

Sources/AsyncHTTPClient/HTTPClient.swift

glbrntt · 2021-07-07T10:12:53Z

Sources/AsyncHTTPClient/RequestBag+StateMachine.swift

+        }
+    }
+
+    mutating func receiveResponseHead(_ head: HTTPResponseHead) -> Bool {


Nit: document what the return value represents

Lukasa

You love to see it.

fabianfett · 2021-07-07T11:31:48Z

CI failure because of known flaky test #371.

Co-authored-by: George Barnett <[email protected]>

fabianfett force-pushed the ff-HTTPClientTask branch 2 times, most recently from 3872998 to 461ab39 Compare June 28, 2021 11:37

fabianfett added this to the HTTP/2 support milestone Jun 28, 2021

fabianfett mentioned this pull request Jun 29, 2021

Add HTTPRequestStateMachine #386

Merged

fabianfett changed the title ~~Add HTTPClientTask~~ Add HTTPScheduledRequest and HTTPExecutingRequest Jun 29, 2021

Lukasa requested changes Jun 29, 2021

View reviewed changes

fabianfett force-pushed the ff-HTTPClientTask branch 4 times, most recently from d502d6f to 2a16ade Compare July 1, 2021 10:29

fabianfett requested review from glbrntt and Lukasa July 1, 2021 10:29

glbrntt reviewed Jul 2, 2021

View reviewed changes

fabianfett force-pushed the ff-HTTPClientTask branch 2 times, most recently from 73e703b to dd8ec55 Compare July 5, 2021 12:28

fabianfett requested a review from glbrntt July 5, 2021 12:28

fabianfett force-pushed the ff-HTTPClientTask branch 3 times, most recently from 2d7bbce to 6f45596 Compare July 6, 2021 08:52

glbrntt reviewed Jul 6, 2021

View reviewed changes

fabianfett requested a review from glbrntt July 6, 2021 14:25

fabianfett added the 🆕 semver/minor Adds new public API. label Jul 6, 2021

fabianfett force-pushed the ff-HTTPClientTask branch from b595e3b to 88ceeb5 Compare July 6, 2021 16:16

glbrntt approved these changes Jul 7, 2021

View reviewed changes

Lukasa approved these changes Jul 7, 2021

View reviewed changes

fabianfett and others added 2 commits July 7, 2021 13:32

Add HTTPExecutingRequest

699c5de

Update Sources/AsyncHTTPClient/HTTPClient.swift

3e8f716

Co-authored-by: George Barnett <[email protected]>

Add better docu

d4f640f

fabianfett force-pushed the ff-HTTPClientTask branch from f551059 to d4f640f Compare July 7, 2021 11:32

fabianfett merged commit a9aee26 into swift-server:main Jul 7, 2021

fabianfett deleted the ff-HTTPClientTask branch July 10, 2021 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HTTPScheduledRequest and HTTPExecutingRequest #384

Add HTTPScheduledRequest and HTTPExecutingRequest #384

fabianfett commented Jun 28, 2021 •

edited

Loading

Lukasa Jun 29, 2021

Lukasa Jun 29, 2021

Lukasa Jun 29, 2021

Lukasa Jul 5, 2021

Lukasa Jun 29, 2021

Lukasa Jun 29, 2021

Lukasa Jun 29, 2021

glbrntt Jul 2, 2021

glbrntt Jul 2, 2021

fabianfett Jul 5, 2021

glbrntt Jul 2, 2021

glbrntt Jul 2, 2021

glbrntt Jul 2, 2021

glbrntt Jul 2, 2021

glbrntt Jul 5, 2021

glbrntt Jul 6, 2021

glbrntt Jul 6, 2021

glbrntt Jul 6, 2021

fabianfett Jul 6, 2021

glbrntt Jul 6, 2021

fabianfett commented Jul 6, 2021

glbrntt left a comment

glbrntt Jul 7, 2021

Lukasa left a comment

fabianfett commented Jul 7, 2021


		// MARK: - Request -

		private func willExecuteRequest0(_ executor: HTTPRequestExecutor) {

Add HTTPScheduledRequest and HTTPExecutingRequest #384

Add HTTPScheduledRequest and HTTPExecutingRequest #384

Conversation

fabianfett commented Jun 28, 2021 • edited Loading

Motivation

Changes

Notes for reviewers:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianfett commented Jul 6, 2021

glbrntt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa left a comment

Choose a reason for hiding this comment

fabianfett commented Jul 7, 2021

fabianfett commented Jun 28, 2021 •

edited

Loading