From a75a7cd5e5040f9cfdfe017d559d0d1ca8a532ea Mon Sep 17 00:00:00 2001 From: Fabian Fett Date: Tue, 7 Sep 2021 11:53:45 +0200 Subject: [PATCH 1/4] Add MockTools for testing HTTP1ConnectionPool.StateMachine compiles again Compiles Better names. --- .../Mocks/MockConnectionPool.swift | 486 ++++++++++++++++++ .../Mocks/MockRequestQueuer.swift | 92 ++++ 2 files changed, 578 insertions(+) create mode 100644 Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift create mode 100644 Tests/AsyncHTTPClientTests/Mocks/MockRequestQueuer.swift diff --git a/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift b/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift new file mode 100644 index 000000000..0b8ad91b2 --- /dev/null +++ b/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift @@ -0,0 +1,486 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +@testable import AsyncHTTPClient +import Logging +import NIO +import NIOHTTP1 + +/// A mock connection pool (not creating any actual connections) that is used to validate +/// connection actions returned by the `HTTPConnectionPool.StateMachine`. +struct MockConnectionPool { + typealias Connection = HTTPConnectionPool.Connection + + enum Errors: Error { + case connectionIDAlreadyUsed + case connectionNotFound + case connectionExists + case connectionNotIdle + case connectionAlreadyParked + case connectionNotParked + case connectionIsParked + case connectionIsClosed + case connectionIsNotStarting + case connectionIsNotExecuting + case connectionDoesNotFulfillEventLoopRequirement + case connectionBackoffTimerExists + case connectionBackoffTimerNotFound + } + + private struct MockConnectionState { + typealias ID = HTTPConnectionPool.Connection.ID + + enum State { + case starting + case http1(leased: Bool, lastIdle: NIODeadline) + case http2(streams: Int, used: Int) + case closed + } + + let id: ID + let eventLoop: EventLoop + + private(set) var state: State = .starting + private(set) var isParked: Bool = false + + init(id: ID, eventLoop: EventLoop) { + self.id = id + self.eventLoop = eventLoop + } + + var isStarting: Bool { + switch self.state { + case .starting: + return true + default: + return false + } + } + + var isIdle: Bool { + switch self.state { + case .starting, .closed: + return false + case .http1(let leased, _): + return !leased + case .http2(_, let used): + return used == 0 + } + } + + var isLeased: Bool { + switch self.state { + case .starting, .closed: + return false + case .http1(let leased, _): + return leased + case .http2(_, let used): + return used > 0 + } + } + + var lastIdle: NIODeadline? { + switch self.state { + case .starting, .closed: + return nil + case .http1(_, let lastReturn): + return lastReturn + case .http2: + return nil + } + } + + mutating func http1Started() throws { + guard case .starting = self.state else { + throw Errors.connectionIsNotStarting + } + + self.state = .http1(leased: false, lastIdle: .now()) + } + + mutating func park() throws { + guard self.isIdle else { + throw Errors.connectionNotIdle + } + + guard !self.isParked else { + throw Errors.connectionAlreadyParked + } + + self.isParked = true + } + + mutating func activate() throws { + guard self.isIdle else { + throw Errors.connectionNotIdle + } + + guard self.isParked else { + throw Errors.connectionNotParked + } + + self.isParked = false + } + + mutating func execute(_ request: HTTPSchedulableRequest) throws { + guard !self.isParked else { + throw Errors.connectionIsParked + } + + if let required = request.requiredEventLoop, required !== self.eventLoop { + throw Errors.connectionDoesNotFulfillEventLoopRequirement + } + + switch self.state { + case .starting: + preconditionFailure("Should be unreachable") + case .http1(leased: true, _): + throw Errors.connectionNotIdle + 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 + case .http2(let streams, var used): + used += 1 + self.state = .http2(streams: streams, used: used) + case .closed: + throw Errors.connectionIsClosed + } + } + + mutating func finishRequest() throws { + guard !self.isParked else { + throw Errors.connectionIsParked + } + + switch self.state { + case .starting: + throw Errors.connectionIsNotExecuting + case .http1(leased: true, _): + self.state = .http1(leased: false, lastIdle: .now()) + case .http1(leased: false, _): + throw Errors.connectionIsNotExecuting + case .http2(_, let used) where used <= 0: + throw Errors.connectionIsNotExecuting + case .http2(let streams, var used): + used -= 1 + self.state = .http2(streams: streams, used: used) + case .closed: + throw Errors.connectionIsClosed + } + } + + mutating func close() throws { + switch self.state { + case .starting: + throw Errors.connectionNotIdle + case .http1(let leased, _): + if leased { + throw Errors.connectionNotIdle + } + case .http2(_, let used): + if used > 0 { + throw Errors.connectionNotIdle + } + case .closed: + throw Errors.connectionIsClosed + } + } + } + + private var connections = [Connection.ID: MockConnectionState]() + private var backoff = Set() + + init() {} + + var parked: Int { + self.connections.values.filter { $0.isParked }.count + } + + var leased: Int { + self.connections.values.filter { $0.isLeased }.count + } + + var starting: Int { + self.connections.values.filter { $0.isStarting }.count + } + + var count: Int { + self.connections.count + } + + var isEmpty: Bool { + self.connections.isEmpty + } + + var newestParkedConnection: Connection? { + self.connections.values + .filter { $0.isParked } + .max(by: { $0.lastIdle! < $1.lastIdle! }) + .flatMap { .__testOnly_connection(id: $0.id, eventLoop: $0.eventLoop) } + } + + var oldestParkedConnection: Connection? { + self.connections.values + .filter { $0.isParked } + .min(by: { $0.lastIdle! < $1.lastIdle! }) + .flatMap { .__testOnly_connection(id: $0.id, eventLoop: $0.eventLoop) } + } + + func newestParkedConnection(for eventLoop: EventLoop) -> Connection? { + self.connections.values + .filter { $0.eventLoop === eventLoop && $0.isParked } + .sorted(by: { $0.lastIdle! > $1.lastIdle! }) + .max(by: { $0.lastIdle! < $1.lastIdle! }) + .flatMap { .__testOnly_connection(id: $0.id, eventLoop: $0.eventLoop) } + } + + func connections(on eventLoop: EventLoop) -> Int { + self.connections.values.filter { $0.eventLoop === eventLoop }.count + } + + // MARK: Connection creation + + mutating func createConnection(_ connectionID: Connection.ID, on eventLoop: EventLoop) throws { + guard self.connections[connectionID] == nil else { + throw Errors.connectionIDAlreadyUsed + } + self.connections[connectionID] = .init(id: connectionID, eventLoop: eventLoop) + } + + mutating func succeedConnectionCreationHTTP1(_ connectionID: Connection.ID) throws -> Connection { + guard var connection = self.connections[connectionID] else { + throw Errors.connectionNotFound + } + + try connection.http1Started() + self.connections[connection.id] = connection + return .__testOnly_connection(id: connection.id, eventLoop: connection.eventLoop) + } + + mutating func failConnectionCreation(_ connectionID: Connection.ID) throws { + guard let connection = self.connections[connectionID] else { + throw Errors.connectionNotFound + } + + guard connection.isStarting else { + throw Errors.connectionIsNotStarting + } + + self.connections[connection.id] = nil + } + + mutating func startConnectionBackoffTimer(_ connectionID: Connection.ID) throws { + guard self.connections[connectionID] == nil else { + throw Errors.connectionExists + } + + guard !self.backoff.contains(connectionID) else { + throw Errors.connectionBackoffTimerExists + } + + self.backoff.insert(connectionID) + } + + mutating func connectionBackoffTimerDone(_ connectionID: Connection.ID) throws { + guard self.backoff.remove(connectionID) != nil else { + throw Errors.connectionBackoffTimerNotFound + } + } + + mutating func cancelConnectionBackoffTimer(_ connectionID: Connection.ID) throws { + guard self.backoff.remove(connectionID) != nil else { + throw Errors.connectionBackoffTimerNotFound + } + } + + // MARK: Connection destruction + + /// Closing a connection signals intend. For this reason, it is verified, that the connection is not running any + /// requests when closing. + mutating func closeConnection(_ connection: Connection) throws { + guard var mockConnection = self.connections.removeValue(forKey: connection.id) else { + throw Errors.connectionNotFound + } + + try mockConnection.close() + } + + /// Aborting a connection does not verify if the connection does anything right now + mutating func abortConnection(_ connectionID: Connection.ID) throws { + guard self.connections.removeValue(forKey: connectionID) != nil else { + throw Errors.connectionNotFound + } + } + + // MARK: Connection usage + + mutating func parkConnection(_ connectionID: Connection.ID) throws { + guard var connection = self.connections[connectionID] else { + throw Errors.connectionNotFound + } + + try connection.park() + self.connections[connectionID] = connection + } + + mutating func activateConnection(_ connectionID: Connection.ID) throws { + guard var connection = self.connections[connectionID] else { + throw Errors.connectionNotFound + } + + try connection.activate() + self.connections[connectionID] = connection + } + + mutating func execute(_ request: HTTPSchedulableRequest, on connection: Connection) throws { + guard var connection = self.connections[connection.id] else { + throw Errors.connectionNotFound + } + + try connection.execute(request) + self.connections[connection.id] = connection + } + + mutating func finishExecution(_ connectionID: Connection.ID) throws { + guard var connection = self.connections[connectionID] else { + throw Errors.connectionNotFound + } + + try connection.finishRequest() + self.connections[connectionID] = connection + } + + mutating func randomStartingConnection() -> Connection.ID? { + self.connections.values + .filter { $0.isStarting } + .randomElement() + .map(\.id) + } + + mutating func randomActiveConnection() -> Connection.ID? { + self.connections.values + .filter { $0.isLeased || $0.isParked } + .randomElement() + .map(\.id) + } + + mutating func randomParkedConnection() -> Connection? { + self.connections.values + .filter { $0.isParked } + .randomElement() + .flatMap { .__testOnly_connection(id: $0.id, eventLoop: $0.eventLoop) } + } + + mutating func randomLeasedConnection() -> Connection? { + self.connections.values + .filter { $0.isLeased } + .randomElement() + .flatMap { .__testOnly_connection(id: $0.id, eventLoop: $0.eventLoop) } + } + + func randomBackingOffConnection() -> Connection.ID? { + self.backoff.randomElement() + } + + mutating func closeRandomActiveConnection() -> Connection.ID? { + guard let connectionID = self.randomActiveConnection() else { + return nil + } + + self.connections.removeValue(forKey: connectionID) + return connectionID + } +} + +/// A request that can be used when testing the `HTTPConnectionPool.StateMachine` +/// with the `MockConnectionPool`. +class MockHTTPRequest: HTTPSchedulableRequest { + let logger: Logger + let connectionDeadline: NIODeadline + let idleReadTimeout: TimeAmount? + + let preferredEventLoop: EventLoop + let requiredEventLoop: EventLoop? + + init(eventLoop: EventLoop, + logger: Logger = Logger(label: "mock"), + connectionTimeout: TimeAmount = .seconds(60), + idleReadTimeout: TimeAmount? = nil, + requiresEventLoopForChannel: Bool = false) { + self.logger = logger + + self.connectionDeadline = .now() + connectionTimeout + self.idleReadTimeout = idleReadTimeout + + self.preferredEventLoop = eventLoop + if requiresEventLoopForChannel { + self.requiredEventLoop = eventLoop + } else { + self.requiredEventLoop = nil + } + } + + var eventLoop: EventLoop { + return self.preferredEventLoop + } + + // MARK: HTTPSchedulableRequest + + func requestWasQueued(_: HTTPRequestScheduler) { + preconditionFailure("Unimplemented") + } + + func fail(_: Error) { + preconditionFailure("Unimplemented") + } + + // MARK: HTTPExecutableRequest + + var requestHead: HTTPRequestHead { + preconditionFailure("Unimplemented") + } + + var requestFramingMetadata: RequestFramingMetadata { + preconditionFailure("Unimplemented") + } + + func willExecuteRequest(_: HTTPRequestExecutor) { + preconditionFailure("Unimplemented") + } + + func requestHeadSent() { + preconditionFailure("Unimplemented") + } + + func resumeRequestBodyStream() { + preconditionFailure("Unimplemented") + } + + func pauseRequestBodyStream() { + preconditionFailure("Unimplemented") + } + + func receiveResponseHead(_: HTTPResponseHead) { + preconditionFailure("Unimplemented") + } + + func receiveResponseBodyParts(_: CircularBuffer) { + preconditionFailure("Unimplemented") + } + + func succeedRequest(_: CircularBuffer?) { + preconditionFailure("Unimplemented") + } +} diff --git a/Tests/AsyncHTTPClientTests/Mocks/MockRequestQueuer.swift b/Tests/AsyncHTTPClientTests/Mocks/MockRequestQueuer.swift new file mode 100644 index 000000000..eb4cd1997 --- /dev/null +++ b/Tests/AsyncHTTPClientTests/Mocks/MockRequestQueuer.swift @@ -0,0 +1,92 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the AsyncHTTPClient open source project +// +// Copyright (c) 2021 Apple Inc. and the AsyncHTTPClient project authors +// Licensed under Apache License v2.0 +// +// See LICENSE.txt for license information +// See CONTRIBUTORS.txt for the list of AsyncHTTPClient project authors +// +// SPDX-License-Identifier: Apache-2.0 +// +//===----------------------------------------------------------------------===// + +@testable import AsyncHTTPClient +import Logging +import NIO +import NIOHTTP1 + +/// A mock request queue (not creating any timers) that is used to validate +/// request actions returned by the `HTTPConnectionPool.StateMachine`. +struct MockRequestQueuer { + enum Errors: Error { + case requestIDNotFound + case requestIDAlreadyUsed + case requestIDDoesNotMatchTask + } + + typealias RequestID = HTTPConnectionPool.Request.ID + + private struct QueuedRequest { + let id: RequestID + let request: HTTPSchedulableRequest + } + + init() { + self.waiters = [:] + } + + private var waiters: [RequestID: QueuedRequest] + + var count: Int { + self.waiters.count + } + + var isEmpty: Bool { + self.waiters.isEmpty + } + + mutating func queue(_ request: HTTPSchedulableRequest, id: RequestID) throws { + guard self.waiters[id] == nil else { + throw Errors.requestIDAlreadyUsed + } + + self.waiters[id] = QueuedRequest(id: id, request: request) + } + + mutating func fail(_ id: RequestID, request: HTTPSchedulableRequest) throws { + guard let waiter = self.waiters.removeValue(forKey: id) else { + throw Errors.requestIDNotFound + } + guard waiter.request === request else { + throw Errors.requestIDDoesNotMatchTask + } + } + + mutating func get(_ id: RequestID, request: HTTPSchedulableRequest) throws -> HTTPSchedulableRequest { + guard let waiter = self.waiters.removeValue(forKey: id) else { + throw Errors.requestIDNotFound + } + guard waiter.request === request else { + throw Errors.requestIDDoesNotMatchTask + } + return waiter.request + } + + @discardableResult + mutating func cancel(_ id: RequestID) throws -> HTTPSchedulableRequest { + guard let waiter = self.waiters.removeValue(forKey: id) else { + throw Errors.requestIDNotFound + } + return waiter.request + } + + mutating func timeoutRandomRequest() -> RequestID? { + guard let waiterID = self.waiters.randomElement().map(\.0) else { + return nil + } + self.waiters.removeValue(forKey: waiterID) + return waiterID + } +} From 054fca844201c5a1b7749273c33ce19f0afed711 Mon Sep 17 00:00:00 2001 From: Fabian Fett Date: Tue, 7 Sep 2021 13:57:02 +0200 Subject: [PATCH 2/4] Update Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift Co-authored-by: Cory Benfield --- .../HTTPConnectionPool+HTTP1Connections.swift | 4 +- .../Mocks/MockConnectionPool.swift | 219 +++++++++++------- 2 files changed, 136 insertions(+), 87 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift index 3d0e08e51..c29b32a7a 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift @@ -80,8 +80,8 @@ extension HTTPConnectionPool { var idleSince: NIODeadline? { switch self.state { - case .idle(_, since: let lastReturn): - return lastReturn + case .idle(_, since: let idleSince): + return idleSince case .backingOff, .starting, .leased, .closed: return nil } diff --git a/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift b/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift index 0b8ad91b2..609ee618f 100644 --- a/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift +++ b/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift @@ -22,37 +22,46 @@ import NIOHTTP1 struct MockConnectionPool { typealias Connection = HTTPConnectionPool.Connection - enum Errors: Error { + enum Errors: Error, Hashable { case connectionIDAlreadyUsed case connectionNotFound case connectionExists case connectionNotIdle - case connectionAlreadyParked case connectionNotParked case connectionIsParked case connectionIsClosed case connectionIsNotStarting case connectionIsNotExecuting case connectionDoesNotFulfillEventLoopRequirement + case connectionDoesNotHaveHTTP2StreamAvailable case connectionBackoffTimerExists case connectionBackoffTimerNotFound } - private struct MockConnectionState { + fileprivate struct MockConnectionState { typealias ID = HTTPConnectionPool.Connection.ID - enum State { + private enum State { + enum HTTP1State { + case inUse + case idle(parked: Bool, idleSince: NIODeadline) + } + + enum HTTP2State { + case inUse(maxConcurrentStreams: Int, used: Int) + case idle(maxConcurrentStreams: Int, parked: Bool, lastIdle: NIODeadline) + } + case starting - case http1(leased: Bool, lastIdle: NIODeadline) - case http2(streams: Int, used: Int) + case http1(HTTP1State) + case http2(HTTP2State) case closed } let id: ID let eventLoop: EventLoop - private(set) var state: State = .starting - private(set) var isParked: Bool = false + private var state: State = .starting init(id: ID, eventLoop: EventLoop) { self.id = id @@ -68,36 +77,60 @@ struct MockConnectionPool { } } + /// Is the connection idle (meaning there are no requests executing on it) var isIdle: Bool { switch self.state { - case .starting, .closed: + case .starting, .closed, .http1(.inUse), .http2(.inUse): return false - case .http1(let leased, _): - return !leased - case .http2(_, let used): - return used == 0 + + case .http1(.idle), .http2(.idle): + return true } } - var isLeased: Bool { + /// Is the connection available (can another request be executed on it) + var isAvailable: Bool { switch self.state { - case .starting, .closed: + case .starting, .closed, .http1(.inUse): return false - case .http1(let leased, _): - return leased - case .http2(_, let used): - return used > 0 + + case .http2(.inUse(let maxStreams, let used)): + return used < maxStreams + + case .http1(.idle), .http2(.idle): + return true } } - var lastIdle: NIODeadline? { + /// Is the connection idle and did we create a timeout timer for it? + var isParked: Bool { switch self.state { - case .starting, .closed: - return nil - case .http1(_, let lastReturn): - return lastReturn - case .http2: + case .starting, .closed, .http1(.inUse), .http2(.inUse): + return false + + case .http1(.idle(let parked, _)), .http2(.idle(_, let parked, _)): + return parked + } + } + + /// Is the connection in use (are there requests executing on it) + var isUsed: Bool { + switch self.state { + case .starting, .closed, .http1(.idle), .http2(.idle): + return false + + case .http1(.inUse), .http2(.inUse): + return true + } + } + + var idleSince: NIODeadline? { + switch self.state { + case .starting, .closed, .http1(.inUse), .http2(.inUse): return nil + + case .http1(.idle(_, let lastIdle)), .http2(.idle(_, _, let lastIdle)): + return lastIdle } } @@ -106,76 +139,93 @@ struct MockConnectionPool { throw Errors.connectionIsNotStarting } - self.state = .http1(leased: false, lastIdle: .now()) + self.state = .http1(.idle(parked: false, idleSince: .now())) } mutating func park() throws { - guard self.isIdle else { + switch self.state { + case .starting, .closed, .http1(.inUse), .http2(.inUse): throw Errors.connectionNotIdle - } - guard !self.isParked else { - throw Errors.connectionAlreadyParked - } + case .http1(.idle(true, _)), .http2(.idle(_, true, _)): + throw Errors.connectionIsParked - self.isParked = true + case .http1(.idle(false, let lastIdle)): + self.state = .http1(.idle(parked: true, idleSince: lastIdle)) + + case .http2(.idle(let maxStreams, false, let lastIdle)): + self.state = .http2(.idle(maxConcurrentStreams: maxStreams, parked: true, lastIdle: lastIdle)) + } } mutating func activate() throws { - guard self.isIdle else { + switch self.state { + case .starting, .closed, .http1(.inUse), .http2(.inUse): throw Errors.connectionNotIdle - } - guard self.isParked else { + case .http1(.idle(false, _)), .http2(.idle(_, false, _)): throw Errors.connectionNotParked - } - self.isParked = false + case .http1(.idle(true, let lastIdle)): + self.state = .http1(.idle(parked: false, idleSince: lastIdle)) + + case .http2(.idle(let maxStreams, true, let lastIdle)): + self.state = .http2(.idle(maxConcurrentStreams: maxStreams, parked: false, lastIdle: lastIdle)) + } } mutating func execute(_ request: HTTPSchedulableRequest) throws { - guard !self.isParked else { + switch self.state { + case .starting, .http1(.inUse): + throw Errors.connectionNotIdle + + case .http1(.idle(true, _)), .http2(.idle(_, true, _)): throw Errors.connectionIsParked - } - if let required = request.requiredEventLoop, required !== self.eventLoop { - throw Errors.connectionDoesNotFulfillEventLoopRequirement - } + case .http1(.idle(false, _)): + if let required = request.requiredEventLoop, required !== self.eventLoop { + throw Errors.connectionDoesNotFulfillEventLoopRequirement + } + self.state = .http1(.inUse) + + case .http2(.idle(let maxStreams, false, _)): + if let required = request.requiredEventLoop, required !== self.eventLoop { + throw Errors.connectionDoesNotFulfillEventLoopRequirement + } + if maxStreams < 1 { + throw Errors.connectionDoesNotHaveHTTP2StreamAvailable + } + self.state = .http2(.inUse(maxConcurrentStreams: maxStreams, used: 1)) + + case .http2(.inUse(let maxStreams, let used)): + if let required = request.requiredEventLoop, required !== self.eventLoop { + throw Errors.connectionDoesNotFulfillEventLoopRequirement + } + if used + 1 > maxStreams { + throw Errors.connectionDoesNotHaveHTTP2StreamAvailable + } + self.state = .http2(.inUse(maxConcurrentStreams: maxStreams, used: used + 1)) - switch self.state { - case .starting: - preconditionFailure("Should be unreachable") - case .http1(leased: true, _): - throw Errors.connectionNotIdle - 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 - case .http2(let streams, var used): - used += 1 - self.state = .http2(streams: streams, used: used) case .closed: throw Errors.connectionIsClosed } } mutating func finishRequest() throws { - guard !self.isParked else { - throw Errors.connectionIsParked - } - switch self.state { - case .starting: - throw Errors.connectionIsNotExecuting - case .http1(leased: true, _): - self.state = .http1(leased: false, lastIdle: .now()) - case .http1(leased: false, _): + case .starting, .http1(.idle), .http2(.idle): throw Errors.connectionIsNotExecuting - case .http2(_, let used) where used <= 0: - throw Errors.connectionIsNotExecuting - case .http2(let streams, var used): - used -= 1 - self.state = .http2(streams: streams, used: used) + + case .http1(.inUse): + self.state = .http1(.idle(parked: false, idleSince: .now())) + + case .http2(.inUse(let maxStreams, let used)): + if used == 1 { + self.state = .http2(.idle(maxConcurrentStreams: maxStreams, parked: false, lastIdle: .now())) + } else { + self.state = .http2(.inUse(maxConcurrentStreams: maxStreams, used: used)) + } + case .closed: throw Errors.connectionIsClosed } @@ -185,14 +235,12 @@ struct MockConnectionPool { switch self.state { case .starting: throw Errors.connectionNotIdle - case .http1(let leased, _): - if leased { - throw Errors.connectionNotIdle - } - case .http2(_, let used): - if used > 0 { - throw Errors.connectionNotIdle - } + case .http1(.idle), .http2(.idle): + self.state = .closed + + case .http1(.inUse), .http2(.inUse): + throw Errors.connectionNotIdle + case .closed: throw Errors.connectionIsClosed } @@ -209,7 +257,7 @@ struct MockConnectionPool { } var leased: Int { - self.connections.values.filter { $0.isLeased }.count + self.connections.values.filter { $0.isUsed }.count } var starting: Int { @@ -227,22 +275,21 @@ struct MockConnectionPool { var newestParkedConnection: Connection? { self.connections.values .filter { $0.isParked } - .max(by: { $0.lastIdle! < $1.lastIdle! }) + .max(by: { $0.idleSince! < $1.idleSince! }) .flatMap { .__testOnly_connection(id: $0.id, eventLoop: $0.eventLoop) } } var oldestParkedConnection: Connection? { self.connections.values .filter { $0.isParked } - .min(by: { $0.lastIdle! < $1.lastIdle! }) + .min(by: { $0.idleSince! < $1.idleSince! }) .flatMap { .__testOnly_connection(id: $0.id, eventLoop: $0.eventLoop) } } func newestParkedConnection(for eventLoop: EventLoop) -> Connection? { self.connections.values .filter { $0.eventLoop === eventLoop && $0.isParked } - .sorted(by: { $0.lastIdle! > $1.lastIdle! }) - .max(by: { $0.lastIdle! < $1.lastIdle! }) + .max(by: { $0.idleSince! < $1.idleSince! }) .flatMap { .__testOnly_connection(id: $0.id, eventLoop: $0.eventLoop) } } @@ -254,7 +301,7 @@ struct MockConnectionPool { mutating func createConnection(_ connectionID: Connection.ID, on eventLoop: EventLoop) throws { guard self.connections[connectionID] == nil else { - throw Errors.connectionIDAlreadyUsed + throw Errors.connectionExists } self.connections[connectionID] = .init(id: connectionID, eventLoop: eventLoop) } @@ -307,7 +354,7 @@ struct MockConnectionPool { // MARK: Connection destruction - /// Closing a connection signals intend. For this reason, it is verified, that the connection is not running any + /// Closing a connection signals intent. For this reason, it is verified, that the connection is not running any /// requests when closing. mutating func closeConnection(_ connection: Connection) throws { guard var mockConnection = self.connections.removeValue(forKey: connection.id) else { @@ -361,7 +408,9 @@ struct MockConnectionPool { try connection.finishRequest() self.connections[connectionID] = connection } +} +extension MockConnectionPool { mutating func randomStartingConnection() -> Connection.ID? { self.connections.values .filter { $0.isStarting } @@ -371,7 +420,7 @@ struct MockConnectionPool { mutating func randomActiveConnection() -> Connection.ID? { self.connections.values - .filter { $0.isLeased || $0.isParked } + .filter { $0.isUsed || $0.isParked } .randomElement() .map(\.id) } @@ -385,7 +434,7 @@ struct MockConnectionPool { mutating func randomLeasedConnection() -> Connection? { self.connections.values - .filter { $0.isLeased } + .filter { $0.isUsed } .randomElement() .flatMap { .__testOnly_connection(id: $0.id, eventLoop: $0.eventLoop) } } From 322fd45f3b3f82f4fc31d5ce1c2e8826cb22969f Mon Sep 17 00:00:00 2001 From: Fabian Fett Date: Wed, 8 Sep 2021 10:33:29 +0200 Subject: [PATCH 3/4] Small fixes --- .../Mocks/MockConnectionPool.swift | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift b/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift index 609ee618f..f763528e2 100644 --- a/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift +++ b/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift @@ -201,7 +201,7 @@ struct MockConnectionPool { if let required = request.requiredEventLoop, required !== self.eventLoop { throw Errors.connectionDoesNotFulfillEventLoopRequirement } - if used + 1 > maxStreams { + if used >= maxStreams { throw Errors.connectionDoesNotHaveHTTP2StreamAvailable } self.state = .http2(.inUse(maxConcurrentStreams: maxStreams, used: used + 1)) @@ -256,7 +256,7 @@ struct MockConnectionPool { self.connections.values.filter { $0.isParked }.count } - var leased: Int { + var used: Int { self.connections.values.filter { $0.isUsed }.count } @@ -411,28 +411,28 @@ struct MockConnectionPool { } extension MockConnectionPool { - mutating func randomStartingConnection() -> Connection.ID? { + func randomStartingConnection() -> Connection.ID? { self.connections.values .filter { $0.isStarting } .randomElement() .map(\.id) } - mutating func randomActiveConnection() -> Connection.ID? { + func randomActiveConnection() -> Connection.ID? { self.connections.values .filter { $0.isUsed || $0.isParked } .randomElement() .map(\.id) } - mutating func randomParkedConnection() -> Connection? { + func randomParkedConnection() -> Connection? { self.connections.values .filter { $0.isParked } .randomElement() .flatMap { .__testOnly_connection(id: $0.id, eventLoop: $0.eventLoop) } } - mutating func randomLeasedConnection() -> Connection? { + func randomLeasedConnection() -> Connection? { self.connections.values .filter { $0.isUsed } .randomElement() From abdabf11ed792717c90809c3ab070fc4199d3cc4 Mon Sep 17 00:00:00 2001 From: Fabian Fett Date: Wed, 8 Sep 2021 11:05:58 +0200 Subject: [PATCH 4/4] Code review --- .../Mocks/MockConnectionPool.swift | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift b/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift index f763528e2..acee1fa53 100644 --- a/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift +++ b/Tests/AsyncHTTPClientTests/Mocks/MockConnectionPool.swift @@ -42,6 +42,25 @@ struct MockConnectionPool { typealias ID = HTTPConnectionPool.Connection.ID private enum State { + // A note about idle vs. parked connections + // + // In our state machine we differentiate the concept of a connection being idle vs. it + // being parked. An idle connection is a connection that we are not executing any + // request on. A parked connection is an idle connection that will remain in this state + // for a longer period of time. For parked connections we create idle timeout timers. + // + // Consider those two flows to better understand the difference: + // + // 1. A connection becomes `idle` and there is more work queued. This will lead to a new + // request being executed on the connection. It will switch back to be in use without + // having been parked in the meantime. + // + // 2. A connection becomes `idle` and there is no more work queued. We don't want to get + // rid of the connection right away. For this reason we create an idle timeout timer + // for the connection. After having created the timer we consider the connection + // being parked. If a new request arrives, before the connection timed out, we need + // to cancel the idle timeout timer. + enum HTTP1State { case inUse case idle(parked: Bool, idleSince: NIODeadline) @@ -102,7 +121,7 @@ struct MockConnectionPool { } } - /// Is the connection idle and did we create a timeout timer for it? + /// Is the connection idle and did we create an idle timeout timer for it? var isParked: Bool { switch self.state { case .starting, .closed, .http1(.inUse), .http2(.inUse): @@ -192,9 +211,6 @@ struct MockConnectionPool { if let required = request.requiredEventLoop, required !== self.eventLoop { throw Errors.connectionDoesNotFulfillEventLoopRequirement } - if maxStreams < 1 { - throw Errors.connectionDoesNotHaveHTTP2StreamAvailable - } self.state = .http2(.inUse(maxConcurrentStreams: maxStreams, used: 1)) case .http2(.inUse(let maxStreams, let used)): @@ -223,7 +239,7 @@ struct MockConnectionPool { if used == 1 { self.state = .http2(.idle(maxConcurrentStreams: maxStreams, parked: false, lastIdle: .now())) } else { - self.state = .http2(.inUse(maxConcurrentStreams: maxStreams, used: used)) + self.state = .http2(.inUse(maxConcurrentStreams: maxStreams, used: used - 1)) } case .closed: