Skip to content

Tolerate new request after connection error happened #688

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 17, 2023
Merged

Tolerate new request after connection error happened #688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91bce67
Tolerate new request after connection error happened
dnadoba May 15, 2023
8c1706e
fix tests
dnadoba May 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 25 additions & 25 deletions Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,14 +106,14 @@ struct HTTP1ConnectionStateMachine {
return .wait

case .modifying:
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}
}

mutating func channelInactive() -> Action {
switch self.state {
case .initialized:
preconditionFailure("A channel that isn't active, must not become inactive")
fatalError("A channel that isn't active, must not become inactive")

case .inRequest(var requestStateMachine, close: _):
return self.avoidingStateMachineCoW { state -> Action in
Expand All @@ -130,7 +130,7 @@ struct HTTP1ConnectionStateMachine {
return .wait

case .modifying:
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}
}

Expand All @@ -155,7 +155,7 @@ struct HTTP1ConnectionStateMachine {
return .fireChannelError(error, closeConnection: false)

case .modifying:
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}
}

Expand All @@ -173,7 +173,7 @@ struct HTTP1ConnectionStateMachine {
}

case .modifying:
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}
}

Expand All @@ -182,15 +182,15 @@ struct HTTP1ConnectionStateMachine {
metadata: RequestFramingMetadata
) -> Action {
switch self.state {
case .initialized, .closing, .inRequest:
case .initialized, .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)")
fatalError("Invalid state: \(self.state)")

case .closed:
case .closing, .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.
Comment on lines -185 to 196
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual bug fix.

Expand All @@ -208,13 +208,13 @@ struct HTTP1ConnectionStateMachine {
return self.state.modify(with: action)

case .modifying:
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}
}

mutating func requestStreamPartReceived(_ part: IOData, promise: EventLoopPromise<Void>?) -> Action {
guard case .inRequest(var requestStateMachine, let close) = self.state else {
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}

return self.avoidingStateMachineCoW { state -> Action in
Expand All @@ -226,7 +226,7 @@ struct HTTP1ConnectionStateMachine {

mutating func requestStreamFinished(promise: EventLoopPromise<Void>?) -> Action {
guard case .inRequest(var requestStateMachine, let close) = self.state else {
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}

return self.avoidingStateMachineCoW { state -> Action in
Expand All @@ -239,7 +239,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. Invalid state: \(self.state)")
fatalError("This event must only happen, if the connection is leased. During startup this is impossible. Invalid state: \(self.state)")

case .idle:
if closeConnection {
Expand All @@ -260,7 +260,7 @@ struct HTTP1ConnectionStateMachine {
return .wait

case .modifying:
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}
}

Expand All @@ -269,7 +269,7 @@ struct HTTP1ConnectionStateMachine {
mutating func read() -> Action {
switch self.state {
case .initialized:
preconditionFailure("Why should we read something, if we are not connected yet")
fatalError("Why should we read something, if we are not connected yet")
case .idle:
return .read
case .inRequest(var requestStateMachine, let close):
Expand All @@ -284,14 +284,14 @@ struct HTTP1ConnectionStateMachine {
return .read

case .modifying:
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}
}

mutating func channelRead(_ part: HTTPClientResponsePart) -> Action {
switch self.state {
case .initialized, .idle:
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")

case .inRequest(var requestStateMachine, var close):
return self.avoidingStateMachineCoW { state -> Action in
Expand All @@ -310,7 +310,7 @@ struct HTTP1ConnectionStateMachine {
return .wait

case .modifying:
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}
}

Expand All @@ -327,13 +327,13 @@ struct HTTP1ConnectionStateMachine {
}

case .modifying:
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}
}

mutating func demandMoreResponseBodyParts() -> Action {
guard case .inRequest(var requestStateMachine, let close) = self.state else {
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}

return self.avoidingStateMachineCoW { state -> Action in
Expand All @@ -345,7 +345,7 @@ struct HTTP1ConnectionStateMachine {

mutating func idleReadTimeoutTriggered() -> Action {
guard case .inRequest(var requestStateMachine, let close) = self.state else {
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}

return self.avoidingStateMachineCoW { state -> Action in
Expand Down Expand Up @@ -423,7 +423,7 @@ extension HTTP1ConnectionStateMachine.State {
return .forwardResponseBodyParts(parts)
case .succeedRequest(let finalAction, let finalParts):
guard case .inRequest(_, close: let close) = self else {
preconditionFailure("Invalid state: \(self)")
fatalError("Invalid state: \(self)")
}

let newFinalAction: HTTP1ConnectionStateMachine.Action.FinalSuccessfulStreamAction
Expand All @@ -443,9 +443,9 @@ extension HTTP1ConnectionStateMachine.State {
case .failRequest(let error, let finalAction):
switch self {
case .initialized:
preconditionFailure("Invalid state: \(self)")
fatalError("Invalid state: \(self)")
case .idle:
preconditionFailure("How can we fail a task, if we are idle")
fatalError("How can we fail a task, if we are idle")
case .inRequest(_, close: let close):
if case .close(let promise) = finalAction {
self = .closing
Expand All @@ -465,7 +465,7 @@ extension HTTP1ConnectionStateMachine.State {
return .failRequest(error, .none)

case .modifying:
preconditionFailure("Invalid state: \(self)")
fatalError("Invalid state: \(self)")
}

case .read:
Expand Down Expand Up @@ -497,7 +497,7 @@ extension HTTP1ConnectionStateMachine: CustomStringConvertible {
case .closed:
return ".closed"
case .modifying:
preconditionFailure("Invalid state: \(self.state)")
fatalError("Invalid state: \(self.state)")
}
}
}
16 changes: 16 additions & 0 deletions Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,19 @@ class HTTP1ConnectionStateMachineTests: XCTestCase {
XCTAssertEqual(state.requestCancelled(closeConnection: false), .failRequest(HTTPClientError.cancelled, .close(nil)))
}

func testNewRequestAfterErrorHappened() {
var state = HTTP1ConnectionStateMachine()
XCTAssertEqual(state.channelActive(isWritable: false), .fireChannelActive)
struct MyError: Error, Equatable {}
XCTAssertEqual(state.errorHappened(MyError()), .fireChannelError(MyError(), closeConnection: true))
let requestHead = HTTPRequestHead(version: .http1_1, method: .POST, uri: "/", headers: ["content-length": "4"])
let metadata = RequestFramingMetadata(connectionClose: false, body: .fixedSize(4))
let action = state.runNewRequest(head: requestHead, metadata: metadata)
guard case .failRequest = action else {
return XCTFail("unexpected action \(action)")
}
}

func testCancelRequestIsIgnoredWhenConnectionIsIdle() {
var state = HTTP1ConnectionStateMachine()
XCTAssertEqual(state.channelActive(isWritable: true), .fireChannelActive)
Expand Down Expand Up @@ -303,6 +316,8 @@ extension HTTP1ConnectionStateMachine.Action: Equatable {

case (.fireChannelInactive, .fireChannelInactive):
return true
case (.fireChannelError(_, let lhsCloseConnection), .fireChannelError(_, let rhsCloseConnection)):
return lhsCloseConnection == rhsCloseConnection

case (.sendRequestHead(let lhsHead, let lhsStartBody), .sendRequestHead(let rhsHead, let rhsStartBody)):
return lhsHead == rhsHead && lhsStartBody == rhsStartBody
Expand Down Expand Up @@ -377,6 +392,7 @@ extension HTTP1ConnectionStateMachine.Action.FinalFailedStreamAction: Equatable
return lhsPromise?.futureResult == rhsPromise?.futureResult
case (.none, .none):
return true

default:
return false
}
Expand Down