Fix a race between shutdown and backoff timer (#419)

Author: fabianfett

Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

@@ -168,16 +168,25 @@ extension HTTPConnectionPool {
         }
 
         mutating func connectionCreationBackoffDone(_ connectionID: Connection.ID) -> Action {
-            assert(self.connections.stats.backingOff >= 1, "At least this connection is currently in backoff")
-            // The naming of `failConnection` is a little confusing here. All it does is moving the
-            // connection state from `.backingOff` to `.closed` here. It also returns the
-            // connection's index.
-            guard let (index, context) = self.connections.failConnection(connectionID) else {
-                preconditionFailure("Backing off a connection that is unknown to us?")
+            switch self.state {
+            case .running:
+                // The naming of `failConnection` is a little confusing here. All it does is moving the
+                // connection state from `.backingOff` to `.closed` here. It also returns the
+                // connection's index.
+                guard let (index, context) = self.connections.failConnection(connectionID) else {
+                    preconditionFailure("Backing off a connection that is unknown to us?")
+                }
+                // In `nextActionForFailedConnection` a decision will be made whether the failed
+                // connection should be replaced or removed.
+                return self.nextActionForFailedConnection(at: index, context: context)
+
+            case .shuttingDown, .shutDown:
+                // There might be a race between shutdown and a backoff timer firing. On thread A
+                // we might call shutdown which removes the backoff timer. On thread B the backoff
+                // timer might fire at the same time and be blocked by the state lock. In this case
+                // we would look for the backoff timer that was removed just before by the shutdown.
+                return .none
             }
-            // In `nextActionForFailedConnection` a decision will be made whether the failed
-            // connection should be replaced or removed.
-            return self.nextActionForFailedConnection(at: index, context: context)
         }
 
         mutating func connectionIdleTimeout(_ connectionID: Connection.ID) -> Action {

Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests+XCTest.swift

@@ -37,6 +37,7 @@ extension HTTPConnectionPool_HTTP1StateMachineTests {
             ("testParkedConnectionTimesOut", testParkedConnectionTimesOut),
             ("testConnectionPoolFullOfParkedConnectionsIsShutdownImmediately", testConnectionPoolFullOfParkedConnectionsIsShutdownImmediately),
             ("testParkedConnectionTimesOutButIsAlsoClosedByRemote", testParkedConnectionTimesOutButIsAlsoClosedByRemote),
+            ("testConnectionBackoffVsShutdownRace", testConnectionBackoffVsShutdownRace),
         ]
     }
 }

Tests/AsyncHTTPClientTests/HTTPConnectionPool+HTTP1StateTests.swift

@@ -587,4 +587,43 @@ class HTTPConnectionPool_HTTP1StateMachineTests: XCTestCase {
         // triggered by timer
         XCTAssertEqual(state.connectionIdleTimeout(connection.id), .none)
     }
+
+    func testConnectionBackoffVsShutdownRace() {
+        let elg = EmbeddedEventLoopGroup(loops: 2)
+        defer { XCTAssertNoThrow(try elg.syncShutdownGracefully()) }
+
+        var state = HTTPConnectionPool.StateMachine(
+            eventLoopGroup: elg,
+            idGenerator: .init(),
+            maximumConcurrentHTTP1Connections: 6
+        )
+
+        let mockRequest = MockHTTPRequest(eventLoop: elg.next(), requiresEventLoopForChannel: false)
+        let request = HTTPConnectionPool.Request(mockRequest)
+
+        let executeAction = state.executeRequest(request)
+        guard case .createConnection(let connectionID, on: let connEL) = executeAction.connection else {
+            return XCTFail("Expected to create a connection")
+        }
+
+        XCTAssertEqual(executeAction.request, .scheduleRequestTimeout(for: request, on: mockRequest.eventLoop))
+
+        let failAction = state.failedToCreateNewConnection(HTTPClientError.cancelled, connectionID: connectionID)
+        guard case .scheduleBackoffTimer(connectionID, backoff: _, on: let timerEL) = failAction.connection else {
+            return XCTFail("Expected to create a backoff timer")
+        }
+        XCTAssert(timerEL === connEL)
+        XCTAssertEqual(failAction.request, .none)
+
+        let shutdownAction = state.shutdown()
+        guard case .cleanupConnections(let context, isShutdown: .yes(unclean: true)) = shutdownAction.connection else {
+            return XCTFail("Expected to cleanup")
+        }
+        XCTAssertEqual(context.close.count, 0)
+        XCTAssertEqual(context.cancel.count, 0)
+        XCTAssertEqual(context.connectBackoff, [connectionID])
+        XCTAssertEqual(shutdownAction.request, .failRequestsAndCancelTimeouts([request], HTTPClientError.cancelled))
+
+        XCTAssertEqual(state.connectionCreationBackoffDone(connectionID), .none)
+    }
 }