Add MockTools for testing HTTP1ConnectionPool.StateMachine #417
Conversation
compiles again Compiles Better names.
fabianfett
force-pushed
the
ff-add-mockutils
branch
from
eae1d6e to
a75a7cd
Compare
| } | ||
| } | ||
|
|
||
| var isLeased: Bool { |
There was a problem hiding this comment.
Is leased the best adjective here? This sort of seems to mean "is in use".
| case .http1(leased: false, let lastIdle): | ||
| self.state = .http1(leased: true, lastIdle: lastIdle) | ||
| case .http2(let streams, let used) where used >= streams: | ||
| throw Errors.connectionNotIdle |
There was a problem hiding this comment.
Should we have a more granular error for this?
| let eventLoop: EventLoop | ||
|
|
||
| private(set) var state: State = .starting | ||
| private(set) var isParked: Bool = false |
There was a problem hiding this comment.
It's a bit odd that this isn't part of the state. Reading the code suggests that only idle connections can be parked, so it seems like we should be computing that from the state to avoid the risk of this information falling out of step.
| var newestParkedConnection: Connection? { | ||
| self.connections.values | ||
| .filter { $0.isParked } | ||
| .max(by: { $0.lastIdle! < $1.lastIdle! }) |
There was a problem hiding this comment.
My comment about parked falling out to step seems relevant here: you can park an HTTP/2 connection, so this force-unwrap can fail. The same is true for the few below this comment. This doesn't seem like it's what we intended.
| struct MockConnectionPool { | ||
| typealias Connection = HTTPConnectionPool.Connection | ||
|
|
||
| enum Errors: Error { |
|
|
||
| enum State { | ||
| case starting | ||
| case http1(leased: Bool, lastIdle: NIODeadline) |
| enum State { | ||
| case starting | ||
| case http1(leased: Bool, lastIdle: NIODeadline) | ||
| case http2(streams: Int, used: Int) |
There was a problem hiding this comment.
is streams max concurrent streams?
| private struct MockConnectionState { | ||
| typealias ID = HTTPConnectionPool.Connection.ID | ||
|
|
||
| enum State { |
There was a problem hiding this comment.
We could make this Hashable, but TBH really his should be private. So the benefit of it being Hashable is not very large. :)
There was a problem hiding this comment.
Agreed, it isn't large. I was thinking along the lines of killing some of the switches in the various is<State> computed properties.
| throw Errors.connectionNotIdle | ||
| } | ||
|
|
||
| guard !self.isParked else { |
There was a problem hiding this comment.
nit: I find guard not x else { return ... } much less clear than if x { return ... }
| case .http1(let leased, _): | ||
| if leased { | ||
| throw Errors.connectionNotIdle | ||
| } | ||
| case .http2(_, let used): | ||
| if used > 0 { | ||
| throw Errors.connectionNotIdle | ||
| } |
There was a problem hiding this comment.
Do we need to transition to closed here on the happy path?
| func newestParkedConnection(for eventLoop: EventLoop) -> Connection? { | ||
| self.connections.values | ||
| .filter { $0.eventLoop === eventLoop && $0.isParked } | ||
| .sorted(by: { $0.lastIdle! > $1.lastIdle! }) |
There was a problem hiding this comment.
This is redundant with the max(by:) below
| .sorted(by: { $0.lastIdle! > $1.lastIdle! }) |
|
|
||
| mutating func createConnection(_ connectionID: Connection.ID, on eventLoop: EventLoop) throws { | ||
| guard self.connections[connectionID] == nil else { | ||
| throw Errors.connectionIDAlreadyUsed |
There was a problem hiding this comment.
do we really need to distinguish this from Errors.connectionExists?
fabianfett
force-pushed
the
ff-add-mockutils
branch
from
32bf0fa to
7bc934d
Compare
Co-authored-by: Cory Benfield <[email protected]>
fabianfett
force-pushed
the
ff-add-mockutils
branch
from
7bc934d to
054fca8
Compare
| if let required = request.requiredEventLoop, required !== self.eventLoop { | ||
| throw Errors.connectionDoesNotFulfillEventLoopRequirement | ||
| } | ||
| if used + 1 > maxStreams { |
There was a problem hiding this comment.
Seems odd to do the addition twice.
| if used + 1 > maxStreams { | |
| if used >= maxStreams { |
| self.connections.values.filter { $0.isParked }.count | ||
| } | ||
|
|
||
| var leased: Int { |
| } | ||
|
|
||
| extension MockConnectionPool { | ||
| mutating func randomStartingConnection() -> Connection.ID? { |
There was a problem hiding this comment.
Why are the random<State>Connection functions mutating?
| if let required = request.requiredEventLoop, required !== self.eventLoop { | ||
| throw Errors.connectionDoesNotFulfillEventLoopRequirement | ||
| } | ||
| if maxStreams < 1 { |
There was a problem hiding this comment.
I don't think this state is real: a HTTP/2 connection is not allowed to have max streams less than 1.
| if used == 1 { | ||
| self.state = .http2(.idle(maxConcurrentStreams: maxStreams, parked: false, lastIdle: .now())) | ||
| } else { | ||
| self.state = .http2(.inUse(maxConcurrentStreams: maxStreams, used: used)) |
There was a problem hiding this comment.
Shouldn't used be decremented here?
fabianfett
force-pushed
the
ff-add-mockutils
branch
from
a502f8d to
abdabf1
Compare
No description provided.