HTTP1Connections state machine for ConnectionPool #413
Conversation
fabianfett
force-pushed
the
ff-pool-state-http1-connections
branch
from
3dcb4e0 to
85d48fa
Compare
fabianfett
force-pushed
the
ff-pool-state-http1-connections
branch
from
85d48fa to
66c9b5f
Compare
| /// Represents the state of a single HTTP/1.1 connection | ||
| private struct HTTP1ConnectionState { | ||
| enum State { | ||
| /// the connection is creating a connection. Valid transitions are to: .backingOff, .available and .failed |
There was a problem hiding this comment.
Is .failed meant to be .closed?
| enum State { | ||
| /// the connection is creating a connection. Valid transitions are to: .backingOff, .available and .failed | ||
| case starting | ||
| /// the connection is waiting to retry the establishing a connection. Transition to .closed. Must be replaced for a retry. |
There was a problem hiding this comment.
What do you mean by "Must be replaced for a retry" ?
| } | ||
|
|
||
| private var state: State | ||
| private(set) var connectionID: Connection.ID |
There was a problem hiding this comment.
When does this change, when we retry after backing off?
| } | ||
| } | ||
|
|
||
| mutating func cleanup(_ context: inout CleanupContext) -> Bool { |
There was a problem hiding this comment.
What does the Bool represent?
| } | ||
| } | ||
|
|
||
| mutating func cleanup(_ context: inout CleanupContext) -> Bool { |
There was a problem hiding this comment.
Also it doesn't look like this is actually mutating?
| guard let index = self.connections.firstIndex(where: { $0.connectionID == connectionID }) else { | ||
| // because of a race this connection (connection close runs against trigger of timeout) | ||
| // was already removed from the state machine. | ||
| return .none |
There was a problem hiding this comment.
nit: nil (let's be consistent within a single func 🙂)
| /// the connection is waiting to retry the establishing a connection. Transition to .closed. Must be replaced for a retry. | ||
| case backingOff | ||
| /// the connection is available for a new request. Valid transitions to: .leased and .closed | ||
| case available(Connection, since: NIODeadline) |
There was a problem hiding this comment.
"idle" and "available" are both used through this PR to refer to the same thing, can we stick to one?
|
|
||
| /// Remove or replace a connection. | ||
| /// | ||
| /// Call this function after the connection start backoff is done, or a live connection is closed. |
There was a problem hiding this comment.
So do we expect the state to be one of available, leased or backingOff?
| if index < self.overflowIndex { | ||
| use = .generalPurpose | ||
| // we need to subtract 1, since we dont want this connection to count | ||
| starting = self.startingGeneralPurposeConnections - 1 |
There was a problem hiding this comment.
Is this right? If we're a live connection that's been closed then we won't be in the starting state.
Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift
Show resolved
Hide resolved
fabianfett
force-pushed
the
ff-pool-state-http1-connections
branch
3 times, most recently
from
137c647 to
9463b80
Compare
fabianfett
force-pushed
the
ff-pool-state-http1-connections
branch
from
9463b80 to
b9ab9af
Compare
Add comments Add tests
fabianfett
force-pushed
the
ff-pool-state-http1-connections
branch
from
b9ab9af to
9d8037d
Compare
| /// Represents the state of a single HTTP/1.1 connection | ||
| private struct HTTP1ConnectionState { | ||
| enum State { | ||
| /// the connection is creating a connection. Valid transitions are to: .idle, .available and .closed |
There was a problem hiding this comment.
Nit: .available doesn't exist.
| enum State { | ||
| /// the connection is creating a connection. Valid transitions are to: .idle, .available and .closed | ||
| case starting | ||
| /// the connection is waiting to retry the establishing a connection. Transition to .closed. From .closed |
There was a problem hiding this comment.
What does "Transition to .closed." mean here?
| /// or fail until we finalize the shutdown. | ||
| /// | ||
| /// - Parameter context: A cleanup context to add the connection to based on its state. | ||
| /// - Returns: A bool weather the connection can be removed from the connections |
There was a problem hiding this comment.
Nit:
| /// - Returns: A bool weather the connection can be removed from the connections | |
| /// - Returns: A bool tracking whether the connection can be removed from the connections |
| /// - Parameter context: A cleanup context to add the connection to based on its state. | ||
| /// - Returns: A bool weather the connection can be removed from the connections | ||
| /// struct immediately. | ||
| func cleanup(_ context: inout CleanupContext) -> Bool { |
There was a problem hiding this comment.
I don't love "mystery booleans" in contexts like this: I'd normally prefer us to add an enum that makes it clear what the return values actually mean.
Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift
Show resolved
Hide resolved
|
|
||
| mutating func closeConnection(at index: Int) -> Connection { | ||
| var connectionState = self.connections.remove(at: index) | ||
| self.overflowIndex = self.connections.index(before: self.overflowIndex) |
There was a problem hiding this comment.
Is this unconditional decrement correct? We don't do this for removeConnection, we conditionalise it there.
| } | ||
|
|
||
| mutating func replaceConnection(at index: Int) -> (Connection.ID, EventLoop) { | ||
| assert(self.connections[index].isClosed) |
There was a problem hiding this comment.
Any reason this isn't a precondition?
| /// supplied index after this. | ||
| mutating func failConnection(_ connectionID: Connection.ID) -> (Int, FailedConnectionContext) { | ||
| guard let index = self.connections.firstIndex(where: { $0.connectionID == connectionID }) else { | ||
| preconditionFailure("We tried to create a new connection that we know nothing about?") |
There was a problem hiding this comment.
| preconditionFailure("We tried to create a new connection that we know nothing about?") | |
| preconditionFailure("We tried to fail a connection that we know nothing about?") |
| self.connections = self.connections.enumerated().compactMap { index, connectionState in | ||
| if connectionState.cleanup(&cleanupContext) { | ||
| if index < initialOverflowIndex { | ||
| self.overflowIndex -= 1 |
There was a problem hiding this comment.
You're inconsistent about using index(before:) in this spot. We should be consistent.
| // MARK: - Private functions - | ||
|
|
||
| private func generateIdleConnectionContextForConnection(at index: Int) -> IdleConnectionContext { | ||
| assert(self.connections[index].isIdle) |
There was a problem hiding this comment.
Any reason this isn't a precondition?
Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift
Outdated
Show resolved
Hide resolved
Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift
Show resolved
Hide resolved
Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift
Show resolved
Hide resolved
fabianfett
force-pushed
the
ff-pool-state-http1-connections
branch
from
fea4b49 to
e80b55d
Compare
…ctionPool+HTTP1Connections.swift Co-authored-by: George Barnett <[email protected]>
| return self.connections[self.overflowIndex..<self.connections.endIndex].reduce(into: 0) { count, connection in | ||
| guard connection.eventLoop === eventLoop else { return } | ||
| if connection.isConnecting || connection.isBackingOff { | ||
| count += 1 |
There was a problem hiding this comment.
This can also be unchecked for the same reasons as above.
If we have HTTP/1 connections, a data structure to store them is handy.
Changes
HTTPConnectionPool.HTTP1Connectionsstruct to store HTTP1 connectionsResult
Less code in the connection pool state machine. Shared code between HTTP/1 and HTTP/2 connection pool state machine.