Add `HTTPRequestStateMachine` #386

fabianfett · 2021-06-29T07:52:19Z

This PR adds an explicit HTTPRequestStateMachine. It is intended to be used in two instances in the future:

As a substate machine for a HTTP1ConnectionStateMachine, when the connection is in a request.
As a state machine for a HTTP2RequestHandler that operates on a single http2 stream created by the multiplexer.

It remains to be seen, if we want to handle buffer body parts for backpressure directly in the HTTPExecutingRequest (see also #384) or directly the HTTPRequestStateMachine.

Lukasa · 2021-06-29T13:06:42Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+    }
+
+    fileprivate enum RequestState {
+        enum ExpectedBody {


This is entirely unused.

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

Lukasa · 2021-06-29T13:45:08Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+            return .forwardResponseBodyPart(body, resetReadTimeoutTimer: self.idleReadTimeout)
+
+        case .running(_, .endReceived), .finished:
+            preconditionFailure("How can we sucessfully finish the request, before having received a head")


This comment seems out of place.

Lukasa · 2021-06-29T13:45:22Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+            preconditionFailure("How can we receive a response head before sending a request head ourselves")
+
+        case .running(_, .initialized):
+            preconditionFailure("How can we receive a response body, if we haven't a received a head")


We're receiving an end, technically.

Lukasa · 2021-06-29T13:45:34Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+
+        case .running(.streaming, .receivingBody(let streamState)):
+            preconditionFailure("Unimplemented")
+            #warning("@Fabian: We received response end, before sending our own request's end.")


We've discussed this.

Lukasa · 2021-06-29T13:47:12Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+        }
+
+        if case .endReceived = responseState {
+            preconditionFailure("Invalid state: If we have received everything, we must not schedule further timeout timers")


This seems a bit harsh: it's possible we failed to cancel the timer in time.

Co-authored-by: Cory Benfield <[email protected]>

glbrntt · 2021-06-29T15:59:25Z

Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests.swift

+        // oh the channel reports... we should slow down producing...
+        XCTAssertEqual(state.writabilityChanged(writable: false), .pauseRequestBodyStream)
+
+        // but we issued a .produceMoreRequestBodyData before... Thus, we must accept more produced


I don't follow this, we're no longer writable but can still receive parts on the request stream?

Yes, this might happen because of race conditions. If we receive channelNotWritable in thread A and we need to communicate it to thread B, B may have already created new data. For this reason, we need to accept more data even though we asked the producer to pause.

glbrntt · 2021-06-30T08:23:38Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+    }
+
+    enum Action {
+        enum NextMessageToSend {


This doesn't appear to be used

glbrntt · 2021-06-30T08:35:35Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+        case endReceived
+    }
+
+    enum Action {


FWIW, in the past I've found it slightly awkward when debugging code which relies on a generic Action (rather than one action type per function on the state machine) since it's not clear at the call site what actions may occur as a result of prodding the state machine. I.e. writabilityChanged can only result in three of the 12 defined actions (wait, resumeRequestBodyStream and pauseRequestBodyStream) but that isn't clear when calling writabilityChanged. This is entirely personal preference though.

glbrntt · 2021-06-30T08:39:09Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+    }
+
+    mutating func readEventCaught() -> Action {
+        return .read


Why do we unconditionally read? Does this not just apply to the running state?

glbrntt · 2021-06-30T08:41:06Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+
+    mutating func start() -> Action {
+        guard case .initialized = self.state else {
+            preconditionFailure("Invalid state")


A comment indicating that start() must be called first and exactly once would be useful.

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

Davidde94 · 2021-06-30T09:07:23Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+            case stream
+        }
+
+        enum ProducerControlState: Equatable {


Why not just make this type :String, and you can avoid the CustomStringConvertible below?

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

Lukasa · 2021-07-01T07:29:45Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+
+        /// The request is streaming its request body. `expectedBodyLength` has a value, if the request header contained
+        /// a `"content-length"` header field. It is the request header contained a `"transfer-encoding" = "chunked"`
+        /// header field.


The second sentence here is something I don't understand: can you rewrite it?

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

Lukasa · 2021-07-01T07:30:47Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+        case waitingForHead
+        /// A response head has been received and we are ready to consume more data of the wire
+        case receivingBody(HTTPResponseHead, ConsumerControlState)
+        /// A response end has been received and we are ready to consume more data of the wire


Should be saying "ready to consume more data off the wire" here?

Lukasa · 2021-07-01T07:39:59Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+            self.state = .running(requestState, .receivingBody(responseHead, .downstreamIsConsuming(readPending: true)))
+            return .wait
+        case .running(let requestState, .receivingBody(let responseHead, .downstreamHasDemand)):
+            self.state = .running(requestState, .receivingBody(responseHead, .downstreamHasDemand))


There's no state change here. Is that intentional?

Yes. Added a comment.

Lukasa · 2021-07-01T07:43:17Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+            // situations in which the producer might not have received the plea to pause yet.
+
+            if let expected = expectedBodyLength {
+                if sentBodyBytes + part.readableBytes > expected {


These two if statements can be collapsed.

Lukasa · 2021-07-01T07:45:20Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+                        clearReadTimeoutTimer = true
+                    }
+
+                    return .failRequest(error, .close, clearReadTimeoutTimer: clearReadTimeoutTimer)


Is there any reason this state machine has to keep track of whether we need to clear the read timeout timer? Presumably someone has set the timer, and they know if they have one or not, so should unconditionally clear it.

Lukasa · 2021-07-01T07:49:48Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+            }
+
+            self.state = .finished
+            return .succeedRequest(.none, clearReadTimeoutTimer: false)


Yeah, I don't know why the read timeout isn't getting cleared here. I think whether the read timeout is cleared or reset should not be the responsibility of this state machine: the read timeout should get set when either we've sent our .end or received our first .head, and from that point on it shouldn't be our problem. It adds too many responsibilities to this object.

Alternatively, if you really want it to be composed into this state machine, factor it out into a little state machine of its own and give it an interface that you can call from here. It'll make it much easier to validate its correctness. But honestly I think it's overcomplicating this object.

Lukasa · 2021-07-01T07:54:56Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+            let finalAction: Action.FinalStreamAction
+            switch streamState {
+            case .downstreamIsConsuming(readPending: true):
+                finalAction = .read


I do not understand why the consumer has any control over what we do with the HTTP connection at this point.

I'm not sure I get your point here?

Why does our finalAction depend on what the consumer is doing with the data?

The finalAction depends on whether backpressure was used. If we receive the response end and we have a read event buffered, I think we should make sure this read event is passed forward. This way we ensure that the channel is in a "normal" state (no buffered read event), once we start the next request. I see it as a cleanup job.

glbrntt

Nothing substantive from me. The comments were helpful though, thanks for adding them all!

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

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

Davidde94 · 2021-07-02T09:01:18Z

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

+            case stream
+        }
+
+        enum ProducerControlState: Equatable {


Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift

Lukasa

LGTM, I think I'm happy with this as it stands.

Add HTTPRequestStateMachine

519ca3b

fabianfett requested review from Lukasa, glbrntt and Davidde94 June 29, 2021 07:52

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

Lukasa reviewed Jun 29, 2021

View reviewed changes

Apply suggestions from code review

bcc6021

Co-authored-by: Cory Benfield <[email protected]>

glbrntt reviewed Jun 30, 2021

View reviewed changes

Davidde94 reviewed Jun 30, 2021

View reviewed changes

fabianfett force-pushed the ff-request-state-machine branch 5 times, most recently from 255da7a to b899969 Compare June 30, 2021 17:03

Code review

7e5ad26

fabianfett force-pushed the ff-request-state-machine branch from b899969 to 7e5ad26 Compare June 30, 2021 17:08

fabianfett requested review from Lukasa, glbrntt and Davidde94 July 1, 2021 07:12

Lukasa reviewed Jul 1, 2021

View reviewed changes

glbrntt reviewed Jul 1, 2021

View reviewed changes

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift Show resolved Hide resolved

fabianfett and others added 2 commits July 1, 2021 10:50

Apply suggestions from code review

49bd911

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

Code review

eba6df3

fabianfett requested review from Lukasa and glbrntt July 1, 2021 14:39

Lukasa added the 🔨 semver/patch No public API change. label Jul 2, 2021

Davidde94 approved these changes Jul 2, 2021

View reviewed changes

fabianfett added 3 commits July 2, 2021 11:57

Remove verifyRequest step

3eff1de

Code review

650d14b

Final read events

69539a0

tests

660ad20

fabianfett force-pushed the ff-request-state-machine branch from cd55229 to 660ad20 Compare July 2, 2021 15:46

Lukasa approved these changes Jul 5, 2021

View reviewed changes

fabianfett merged commit cd7c804 into swift-server:main Jul 5, 2021

fabianfett deleted the ff-request-state-machine branch July 5, 2021 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `HTTPRequestStateMachine` #386

Add `HTTPRequestStateMachine` #386

fabianfett commented Jun 29, 2021

Lukasa Jun 29, 2021

Lukasa Jun 29, 2021

Lukasa Jun 29, 2021

Lukasa Jun 29, 2021

Lukasa Jun 29, 2021

glbrntt Jun 29, 2021

fabianfett Jun 30, 2021 •

edited

Loading

glbrntt Jun 30, 2021

glbrntt Jun 30, 2021

glbrntt Jun 30, 2021

glbrntt Jun 30, 2021

Davidde94 Jun 30, 2021

Davidde94 Jul 2, 2021

Lukasa Jul 1, 2021

Lukasa Jul 1, 2021

Lukasa Jul 1, 2021

fabianfett Jul 1, 2021

Lukasa Jul 1, 2021

Lukasa Jul 1, 2021

Lukasa Jul 1, 2021

Lukasa Jul 1, 2021

Lukasa Jul 1, 2021

fabianfett Jul 1, 2021

Lukasa Jul 2, 2021

fabianfett Jul 2, 2021

glbrntt left a comment

Davidde94 Jul 2, 2021

Lukasa left a comment

Add HTTPRequestStateMachine #386

Add HTTPRequestStateMachine #386

Conversation

fabianfett commented Jun 29, 2021

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 Jun 30, 2021 • edited Loading

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

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

Add `HTTPRequestStateMachine` #386

Add `HTTPRequestStateMachine` #386

fabianfett Jun 30, 2021 •

edited

Loading