From eb7b1fb5ce82bb08af2ab17d3dbbcafbc6c88da7 Mon Sep 17 00:00:00 2001 From: Fabian Fett Date: Mon, 24 Jan 2022 13:14:22 +0100 Subject: [PATCH 1/2] Print invalid state, if hitting precondition (#545) (#547) --- .../HTTP1.1/HTTP1ConnectionStateMachine.swift | 16 +++++++-------- .../RequestBag+StateMachine.swift | 20 +++++++++---------- .../HTTP2ConnectionTests.swift | 6 +++--- 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ConnectionStateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ConnectionStateMachine.swift index 614ba712a..743626607 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ConnectionStateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ConnectionStateMachine.swift @@ -159,7 +159,7 @@ struct HTTP1ConnectionStateMachine { metadata: RequestFramingMetadata ) -> Action { guard case .idle = self.state else { - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } var requestStateMachine = HTTPRequestStateMachine(isChannelWritable: self.isChannelWritable) @@ -173,7 +173,7 @@ struct HTTP1ConnectionStateMachine { mutating func requestStreamPartReceived(_ part: IOData) -> Action { guard case .inRequest(var requestStateMachine, let close) = self.state else { - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } return self.avoidingStateMachineCoW { state -> Action in @@ -185,7 +185,7 @@ struct HTTP1ConnectionStateMachine { mutating func requestStreamFinished() -> Action { guard case .inRequest(var requestStateMachine, let close) = self.state else { - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } return self.avoidingStateMachineCoW { state -> Action in @@ -198,7 +198,7 @@ struct HTTP1ConnectionStateMachine { mutating func requestCancelled(closeConnection: Bool) -> Action { switch self.state { case .initialized: - preconditionFailure("This event must only happen, if the connection is leased. During startup this is impossible") + preconditionFailure("This event must only happen, if the connection is leased. During startup this is impossible. Invalid state: \(self.state)") case .idle: if closeConnection { @@ -250,7 +250,7 @@ struct HTTP1ConnectionStateMachine { mutating func channelRead(_ part: HTTPClientResponsePart) -> Action { switch self.state { case .initialized, .idle: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") case .inRequest(var requestStateMachine, var close): return self.avoidingStateMachineCoW { state -> Action in @@ -369,7 +369,7 @@ extension HTTP1ConnectionStateMachine.State { return .forwardResponseBodyParts(parts) case .succeedRequest(let finalAction, let finalParts): guard case .inRequest(_, close: let close) = self else { - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self)") } let newFinalAction: HTTP1ConnectionStateMachine.Action.FinalStreamAction @@ -388,7 +388,7 @@ extension HTTP1ConnectionStateMachine.State { case .failRequest(let error, let finalAction): switch self { case .initialized: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self)") case .idle: preconditionFailure("How can we fail a task, if we are idle") case .inRequest(_, close: let close): @@ -433,7 +433,7 @@ extension HTTP1ConnectionStateMachine: CustomStringConvertible { case .closed: return ".closed" case .modifying: - preconditionFailure(".modifying") + preconditionFailure("Invalid state: \(self.state)") } } } diff --git a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift index c20d5e211..f536f119f 100644 --- a/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift +++ b/Sources/AsyncHTTPClient/RequestBag+StateMachine.swift @@ -127,10 +127,10 @@ extension RequestBag.StateMachine { return .none case .finished: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") case .modifying: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } } @@ -158,7 +158,7 @@ extension RequestBag.StateMachine { // the request is already finished nothing further to do break case .modifying: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } } @@ -205,7 +205,7 @@ extension RequestBag.StateMachine { case .finished(error: .none): return .failFuture(HTTPClientError.requestStreamCancelled) case .modifying: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } } @@ -251,7 +251,7 @@ extension RequestBag.StateMachine { case .finished(error: _): return .none case .modifying: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } } @@ -282,7 +282,7 @@ extension RequestBag.StateMachine { case .finished(error: .none): preconditionFailure("How can the request be finished without error, before receiving response head?") case .modifying: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } } @@ -319,7 +319,7 @@ extension RequestBag.StateMachine { case .finished(error: .none): preconditionFailure("How can the request be finished without error, before receiving response head?") case .modifying: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } } @@ -371,7 +371,7 @@ extension RequestBag.StateMachine { case .finished(error: .none): preconditionFailure("How can the request be finished without error, before receiving response head?") case .modifying: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } } @@ -395,7 +395,7 @@ extension RequestBag.StateMachine { private mutating func failWithConsumptionError(_ error: Error) -> ConsumeAction { switch self.state { case .initialized, .queued: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") case .executing(_, _, .initialized): preconditionFailure("Invalid state: Must have received response head, before this method is called for the first time") @@ -518,7 +518,7 @@ extension RequestBag.StateMachine { // this might happen, if the stream consumer has failed... let's just drop the data return .none case .modifying: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } } } diff --git a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift index 76b931aae..fab866867 100644 --- a/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP2ConnectionTests.swift @@ -352,7 +352,7 @@ extension TestConnectionCreator: HTTPConnectionRequester { return .fail(promise, Error.wantedHTTP2ConnectionButGotHTTP1) case .idle: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } } wrapper.complete() @@ -369,7 +369,7 @@ extension TestConnectionCreator: HTTPConnectionRequester { return .succeed(promise, connection) case .idle: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } } wrapper.complete() @@ -400,7 +400,7 @@ extension TestConnectionCreator: HTTPConnectionRequester { return .type2(promise) case .idle: - preconditionFailure("Invalid state") + preconditionFailure("Invalid state: \(self.state)") } } wrapper.fail(error) From cbf533073d9a5edfb16c266d14151f69137ba7c9 Mon Sep 17 00:00:00 2001 From: Fabian Fett Date: Mon, 24 Jan 2022 13:32:28 +0100 Subject: [PATCH 2/2] Fix race between connection close and scheduling new request (#546) (#548) --- .../HTTP1.1/HTTP1ConnectionStateMachine.swift | 34 ++++++++++++++----- ...P1ConnectionStateMachineTests+XCTest.swift | 1 + .../HTTP1ConnectionStateMachineTests.swift | 14 ++++++++ 3 files changed, 41 insertions(+), 8 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ConnectionStateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ConnectionStateMachine.swift index 743626607..19825aec7 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ConnectionStateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ConnectionStateMachine.swift @@ -158,17 +158,35 @@ struct HTTP1ConnectionStateMachine { head: HTTPRequestHead, metadata: RequestFramingMetadata ) -> Action { - guard case .idle = self.state else { + switch self.state { + case .initialized, .closing, .inRequest: + // These states are unreachable as the connection pool state machine has put the + // connection into these states. In other words the connection pool state machine must + // be aware about these states before the connection itself. For this reason the + // connection pool state machine must not send a new request to the connection, if the + // connection is `.initialized`, `.closing` or `.inRequest` preconditionFailure("Invalid state: \(self.state)") - } - var requestStateMachine = HTTPRequestStateMachine(isChannelWritable: self.isChannelWritable) - let action = requestStateMachine.startRequest(head: head, metadata: metadata) + case .closed: + // The remote may have closed the connection and the connection pool state machine + // was not updated yet because of a race condition. New request vs. marking connection + // as closed. + // + // TODO: AHC should support a fast rescheduling mechanism here. + return .failRequest(HTTPClientError.remoteConnectionClosed, .none) + + case .idle: + var requestStateMachine = HTTPRequestStateMachine(isChannelWritable: self.isChannelWritable) + let action = requestStateMachine.startRequest(head: head, metadata: metadata) - // by default we assume a persistent connection. however in `requestVerified`, we read the - // "connection" header. - self.state = .inRequest(requestStateMachine, close: metadata.connectionClose) - return self.state.modify(with: action) + // by default we assume a persistent connection. however in `requestVerified`, we read the + // "connection" header. + self.state = .inRequest(requestStateMachine, close: metadata.connectionClose) + return self.state.modify(with: action) + + case .modifying: + preconditionFailure("Invalid state: \(self.state)") + } } mutating func requestStreamPartReceived(_ part: IOData) -> Action { diff --git a/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests+XCTest.swift b/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests+XCTest.swift index 75bc6c017..76a37936b 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests+XCTest.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests+XCTest.swift @@ -42,6 +42,7 @@ extension HTTP1ConnectionStateMachineTests { ("testConnectionIsClosedIfErrorHappensWhileInRequest", testConnectionIsClosedIfErrorHappensWhileInRequest), ("testConnectionIsClosedAfterSwitchingProtocols", testConnectionIsClosedAfterSwitchingProtocols), ("testWeDontCrashAfterEarlyHintsAndConnectionClose", testWeDontCrashAfterEarlyHintsAndConnectionClose), + ("testWeDontCrashInRaceBetweenSchedulingNewRequestAndConnectionClose", testWeDontCrashInRaceBetweenSchedulingNewRequestAndConnectionClose), ] } } diff --git a/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift b/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift index 1ceb9f2bf..c8ad3d510 100644 --- a/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift +++ b/Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift @@ -267,6 +267,20 @@ class HTTP1ConnectionStateMachineTests: XCTestCase { XCTAssertEqual(state.channelRead(.head(responseHead)), .wait) XCTAssertEqual(state.channelInactive(), .failRequest(HTTPClientError.remoteConnectionClosed, .none)) } + + func testWeDontCrashInRaceBetweenSchedulingNewRequestAndConnectionClose() { + var state = HTTP1ConnectionStateMachine() + XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive) + XCTAssertEqual(state.channelInactive(), .fireChannelInactive) + + let requestHead = HTTPRequestHead(version: .http1_1, method: .GET, uri: "/") + let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(0)) + let newRequestAction = state.runNewRequest(head: requestHead, metadata: metadata) + guard case .failRequest(let error, .none) = newRequestAction else { + return XCTFail("Unexpected test case") + } + XCTAssertEqual(error as? HTTPClientError, .remoteConnectionClosed) + } } extension HTTP1ConnectionStateMachine.Action: Equatable {