[ConnectionPool] HTTP1StateMachine #416

fabianfett · 2021-09-06T16:16:59Z

Follow up to #415, #417.

For the new HTTPConnectionPool a new StateMachine is handy.

Lukasa · 2021-09-07T11:29:05Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift

@@ -375,6 +375,11 @@ extension HTTPConnectionPool {
            self.connections[index].lease()
        }

+        mutating func parkConnection(at index: Int) -> (Connection.ID, EventLoop) {
+            assert(self.connections[index].isIdle)


Is there any reason this shouldn't be a precondition?

Lukasa · 2021-09-07T11:31:31Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+            }
+        }
+
+        mutating func executeRequestOnPreferredEventLoop(_ request: Request, eventLoop: EventLoop) -> Action {


Any reason this isn't private?

Lukasa · 2021-09-07T11:31:43Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+            )
+        }
+
+        mutating func executeRequestOnRequiredEventLoop(_ request: Request, eventLoop: EventLoop) -> Action {


Any reason this isn't private?

Lukasa · 2021-09-07T11:35:22Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+            let requestAction: StateMachine.RequestAction = .scheduleRequestTimeout(
+                request.connectionDeadline,
+                for: requestID,
+                on: request.preferredEventLoop


Do we need to look this up here? We have an event loop in scope already.

Lukasa · 2021-09-07T11:35:33Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+            let requestAction: StateMachine.RequestAction = .scheduleRequestTimeout(
+                request.connectionDeadline,
+                for: requestID,
+                on: request.preferredEventLoop


Do we need to look this up here? We have an event loop in scope already.

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

Lukasa · 2021-09-07T11:42:48Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+                return .none
+            }
+
+            assert(self.state == .running, "If we are shutting down, we must not have any idle connections")


Any reason this isn't a precondition?

Lukasa · 2021-09-07T11:44:50Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+
+            // If we have remaining request queued, we should fail all of them with a cancelled
+            // error.
+            let waitingRequests = self.queue.removeAll().map { ($0, $0.id) }


The intermediate tuple here seems a bit excessive: do we really need it?

I create the tuple here to keep consistency between the two request actions:

case failRequest(Request, Error, cancelTimeout: Request.ID?) case failRequests([(Request, cancelTimeout: Request.ID?)], Error)

If we fail multiple requests at the same time, we will always need to cancel their timeouts. So I preferred the beauty in having same looking Actions.

Chatted offline: Fixed now

Lukasa · 2021-09-07T11:49:15Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+                return .none
+
+            case .shutDown:
+                preconditionFailure("It the pool is already shutdown, all connections must have been torn down.")


Nit:

Suggested change

preconditionFailure("It the pool is already shutdown, all connections must have been torn down.")

preconditionFailure("If the pool is already shutdown, all connections must have been torn down.")

Lukasa · 2021-09-07T12:46:11Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift

+        enum HTTPTypeStateMachine {
+            case http1(HTTP1StateMachine)
+
+            case _modifying


We almost certainly shouldn't need this case. I know I haven't provided this feedback much prior to here, so don't feel compelled to remove it now, but we should probably go through these state machines and rip out the _modifying states wherever we have then. Recent Swift versions do not CoW here.

glbrntt

(Partial review, will finish tomorrow.)

glbrntt · 2021-09-08T16:33:58Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift

@@ -375,6 +375,11 @@ extension HTTPConnectionPool {
            self.connections[index].lease()
        }

+        mutating func parkConnection(at index: Int) -> (Connection.ID, EventLoop) {


This isn't actually a mutating func

glbrntt · 2021-09-08T16:52:29Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+            //   - 29 failed attempts: ~60s (max out)
+
+            let start = Double(TimeAmount.milliseconds(100).nanoseconds)
+            let backoffNanoseconds = Int64(start * pow(1.25, Double(self.failedConsecutiveConnectionAttempts)))


Suggested change

let backoffNanoseconds = Int64(start * pow(1.25, Double(self.failedConsecutiveConnectionAttempts)))

let backoffNanoseconds = Int64(start * pow(1.25, Double(attempt)))

glbrntt · 2021-09-08T16:53:00Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+            return .none
+        }
+
+        private mutating func calculateBackoff(for attempt: Int) -> TimeAmount {


Don't think this is actually mutating?

Lukasa · 2021-09-09T09:03:22Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+            }
+
+            // if we are not at max connections, we may want to create a new connection
+            if self.connections.startingGeneralPurposeConnections >= self.queue.count {


Is this counting the wrong thing? This seems to risk starting a general purpose connection because we have lots of loop specific work, but without consulting what that loop specific work is.

Lukasa · 2021-09-09T09:04:07Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+            at index: Int,
+            context: HTTP1Connections.FailedConnectionContext
+        ) -> Action {
+            if context.connectionsStartingForUseCase < self.queue.count(for: nil) {


Perhaps file an issue for future enhancement (no sense gluing it into this PR), but I don't love this spelling. Probably self.queue.generalPurposeCount would be clearer.

glbrntt · 2021-09-09T08:37:37Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+        private var connections: HTTP1Connections
+        private var failedConsecutiveConnectionAttempts: Int = 0
+
+        private var queue: RequestQueue


nit: requests or requestQueue provides a little more context

glbrntt · 2021-09-09T08:54:15Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

+        }
+
+        mutating func failedToCreateNewConnection(_ error: Error, connectionID: Connection.ID) -> Action {
+            defer {


Minor, but I find it a bit counter-intuitive to defer the increment here. It means that the first time we call this function we'll pass 0 to calculateBackoff which says that for 1 failed attempt backoff will 100ms. While that's true it's out of sync with the value of attempt we pass to it.

I think it'd be clearer to not defer the increment and to reframe calculateBackoff to be in terms failed attempts (rather than just "attempts") and just subtract 1 from the attempts when computing the unjittered backoff.

glbrntt · 2021-09-09T09:08:20Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift

+            case http1(HTTP1StateMachine)
+        }
+
+        var state: HTTPTypeStateMachine


Assuming HTTPTypeStateMachine will only have http1 and http2 cases (at least for now!) then httpVersion/httpType or similar might be a clearer name here since we're really just delegating to the state machine each case holds.

glbrntt · 2021-09-09T09:10:19Z

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift

+            guard !self.isShuttingDown else {
+                preconditionFailure("Shutdown must only be called once")


nit: precondition(!self.isShuttingDown, "...")

glbrntt · 2021-09-09T09:15:29Z

Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift

+            // in 10% of the cases, we require an explicit EventLoop.
+//            let elRequired = (0..<10).randomElement().flatMap { $0 == 0 ? true : false }!


What's the deal here? (also in testBestConnectionIsPicked)

glbrntt

LGTM!

Lukasa

fabianfett requested review from Lukasa and glbrntt September 6, 2021 16:17

fabianfett force-pushed the ff-pool-state-http1 branch 3 times, most recently from 23fadc0 to 395573d Compare September 7, 2021 10:00

Lukasa requested changes Sep 7, 2021

View reviewed changes

fabianfett force-pushed the ff-pool-state-http1 branch from 395573d to bdd23e1 Compare September 7, 2021 14:03

fabianfett added this to the HTTP/2 support milestone Sep 8, 2021

fabianfett force-pushed the ff-pool-state-http1 branch 2 times, most recently from 993fd7c to 1d99def Compare September 8, 2021 09:47

fabianfett added 3 commits September 8, 2021 17:47

HTTP1StateMachine

aef1d92

Code review part 1

3bcf1c5

Code review

0890c4b

fabianfett force-pushed the ff-pool-state-http1 branch from 1d99def to 0890c4b Compare September 8, 2021 16:25

fabianfett requested a review from Lukasa September 8, 2021 16:25

glbrntt reviewed Sep 8, 2021

View reviewed changes

Code review

9fc3530

fabianfett mentioned this pull request Sep 8, 2021

Add new HTTPConnectionPool #418

Closed

Lukasa reviewed Sep 9, 2021

View reviewed changes

glbrntt reviewed Sep 9, 2021

View reviewed changes

Code review

6eda4da

fabianfett requested review from Lukasa and glbrntt September 9, 2021 11:39

glbrntt approved these changes Sep 9, 2021

View reviewed changes

Lukasa approved these changes Sep 9, 2021

View reviewed changes

fabianfett merged commit 4d726ba into swift-server:main Sep 9, 2021

fabianfett added the 🆕 semver/minor Adds new public API. label Sep 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ConnectionPool] HTTP1StateMachine #416

[ConnectionPool] HTTP1StateMachine #416

fabianfett commented Sep 6, 2021 •

edited

Loading

Lukasa Sep 7, 2021

Lukasa Sep 7, 2021

Lukasa Sep 7, 2021

Lukasa Sep 7, 2021

Lukasa Sep 7, 2021

Lukasa Sep 7, 2021

Lukasa Sep 7, 2021

fabianfett Sep 8, 2021 •

edited

Loading

fabianfett Sep 8, 2021

Lukasa Sep 7, 2021

Lukasa Sep 7, 2021

glbrntt left a comment

glbrntt Sep 8, 2021

glbrntt Sep 8, 2021

glbrntt Sep 8, 2021

Lukasa Sep 9, 2021

Lukasa Sep 9, 2021

glbrntt Sep 9, 2021

glbrntt Sep 9, 2021

glbrntt Sep 9, 2021

glbrntt Sep 9, 2021

glbrntt Sep 9, 2021

glbrntt left a comment

Lukasa left a comment

	preconditionFailure("It the pool is already shutdown, all connections must have been torn down.")
	preconditionFailure("If the pool is already shutdown, all connections must have been torn down.")

	let backoffNanoseconds = Int64(start * pow(1.25, Double(self.failedConsecutiveConnectionAttempts)))
	let backoffNanoseconds = Int64(start * pow(1.25, Double(attempt)))

		guard !self.isShuttingDown else {
		preconditionFailure("Shutdown must only be called once")

		// in 10% of the cases, we require an explicit EventLoop.
		// let elRequired = (0..<10).randomElement().flatMap { $0 == 0 ? true : false }!

[ConnectionPool] HTTP1StateMachine #416

[ConnectionPool] HTTP1StateMachine #416

Conversation

fabianfett commented Sep 6, 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

fabianfett Sep 8, 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

glbrntt left a 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

Lukasa left a comment

Choose a reason for hiding this comment

fabianfett commented Sep 6, 2021 •

edited

Loading

fabianfett Sep 8, 2021 •

edited

Loading