Fix race between connection close and scheduling new request (#546) (#548)

Author: fabianfett

Sources/AsyncHTTPClient/ConnectionPool/HTTP1.1/HTTP1ConnectionStateMachine.swift

@@ -158,17 +158,35 @@ struct HTTP1ConnectionStateMachine{
  head:HTTPRequestHead,
  metadata:RequestFramingMetadata
 )->Action{
-guard case .idle =self.state else{
+switchself.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)")
-}
 
-varrequestStateMachine=HTTPRequestStateMachine(isChannelWritable:self.isChannelWritable)
-letaction= 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:
+varrequestStateMachine=HTTPRequestStateMachine(isChannelWritable:self.isChannelWritable)
+letaction= 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)
-returnself.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)
+returnself.state.modify(with: action)
+
+case.modifying:
+preconditionFailure("Invalid state: \(self.state)")
+}
 }
 
 mutatingfunc requestStreamPartReceived(_ part:IOData)->Action{

Tests/AsyncHTTPClientTests/HTTP1ConnectionStateMachineTests+XCTest.swift

@@ -42,6 +42,7 @@ extension HTTP1ConnectionStateMachineTests{
 ("testConnectionIsClosedIfErrorHappensWhileInRequest", testConnectionIsClosedIfErrorHappensWhileInRequest),
 ("testConnectionIsClosedAfterSwitchingProtocols", testConnectionIsClosedAfterSwitchingProtocols),
 ("testWeDontCrashAfterEarlyHintsAndConnectionClose", testWeDontCrashAfterEarlyHintsAndConnectionClose),
+("testWeDontCrashInRaceBetweenSchedulingNewRequestAndConnectionClose", testWeDontCrashInRaceBetweenSchedulingNewRequestAndConnectionClose),
 ]
 }
 }

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(){
+varstate=HTTP1ConnectionStateMachine()
+XCTAssertEqual(state.channelActive(isWritable:true),.fireChannelActive)
+XCTAssertEqual(state.channelInactive(),.fireChannelInactive)
+
+letrequestHead=HTTPRequestHead(version:.http1_1, method:.GET, uri:"/")
+letmetadata=RequestFramingMetadata(connectionClose:false, body:.fixedSize(0))
+letnewRequestAction= state.runNewRequest(head: requestHead, metadata: metadata)
+guard case .failRequest(let error,.none)= newRequestAction else{
+returnXCTFail("Unexpected test case")
+}
+XCTAssertEqual(error as?HTTPClientError,.remoteConnectionClosed)
+}
 }
 
 extensionHTTP1ConnectionStateMachine.Action:Equatable{