Code review part 1

Author: fabianfett

Diff for: Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift

@@ -376,7 +376,7 @@ extension HTTPConnectionPool {
         }
 
         mutating func parkConnection(at index: Int) -> (Connection.ID, EventLoop) {
-            assert(self.connections[index].isIdle)
+            precondition(self.connections[index].isIdle)
             return (self.connections[index].connectionID, self.connections[index].eventLoop)
         }
 

Diff for: Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1StateMachine.swift

@@ -63,7 +63,7 @@ extension HTTPConnectionPool {
             }
         }
 
-        mutating func executeRequestOnPreferredEventLoop(_ request: Request, eventLoop: EventLoop) -> Action {
+        private mutating func executeRequestOnPreferredEventLoop(_ request: Request, eventLoop: EventLoop) -> Action {
             if let connection = self.connections.leaseConnection(onPreferred: eventLoop) {
                 return .init(
                     request: .executeRequest(request, connection, cancelTimeout: nil),
@@ -76,7 +76,7 @@ extension HTTPConnectionPool {
             let requestAction: StateMachine.RequestAction = .scheduleRequestTimeout(
                 request.connectionDeadline,
                 for: requestID,
-                on: request.preferredEventLoop
+                on: eventLoop
             )
 
             if !self.connections.canGrow {
@@ -101,7 +101,7 @@ extension HTTPConnectionPool {
             )
         }
 
-        mutating func executeRequestOnRequiredEventLoop(_ request: Request, eventLoop: EventLoop) -> Action {
+        private mutating func executeRequestOnRequiredEventLoop(_ request: Request, eventLoop: EventLoop) -> Action {
             if let connection = self.connections.leaseConnection(onRequired: eventLoop) {
                 return .init(
                     request: .executeRequest(request, connection, cancelTimeout: nil),
@@ -114,7 +114,7 @@ extension HTTPConnectionPool {
             let requestAction: StateMachine.RequestAction = .scheduleRequestTimeout(
                 request.connectionDeadline,
                 for: requestID,
-                on: request.preferredEventLoop
+                on: eventLoop
             )
 
             let starting = self.connections.startingEventLoopConnections(on: eventLoop)
@@ -156,12 +156,10 @@ extension HTTPConnectionPool {
                 // decision about the retry will be made in `connectionCreationBackoffDone(_:)`
                 let eventLoop = self.connections.backoffNextConnectionAttempt(connectionID)
 
-                let backoff = TimeAmount.milliseconds(100) * Int(pow(1.25, Double(self.failedConsecutiveConnectionAttempts)))
-                let jitterRange = backoff.nanoseconds / 100 * 5
-                let jitteredBackoff = backoff + .nanoseconds((-jitterRange...jitterRange).randomElement()!)
+                let backoff = self.calculateBackoff(for: self.failedConsecutiveConnectionAttempts)
                 return .init(
                     request: .none,
-                    connection: .scheduleBackoffTimer(connectionID, backoff: jitteredBackoff, on: eventLoop)
+                    connection: .scheduleBackoffTimer(connectionID, backoff: backoff, on: eventLoop)
                 )
 
             case .shuttingDown:
@@ -186,7 +184,7 @@ extension HTTPConnectionPool {
                 return .none
             }
 
-            assert(self.state == .running, "If we are shutting down, we must not have any idle connections")
+            precondition(self.state == .running, "If we are shutting down, we must not have any idle connections")
 
             return .init(
                 request: .none,
@@ -382,7 +380,7 @@ extension HTTPConnectionPool {
                 return .none
 
             case .shutDown:
-                preconditionFailure("It the pool is already shutdown, all connections must have been torn down.")
+                preconditionFailure("If the pool is already shutdown, all connections must have been torn down.")
             }
         }
 
@@ -419,6 +417,30 @@ extension HTTPConnectionPool {
             self.connections.removeConnection(at: index)
             return .none
         }
+        
+        private mutating func calculateBackoff(for attempt: Int) -> TimeAmount {
+            // Our backoff formula is: 100ms * 1.25^attempt that is capped of at 1minute
+            // This means for:
+            //   -  1 failed attempt :  100ms
+            //   -  5 failed attempts: ~300ms
+            //   - 10 failed attempts: ~930ms
+            //   - 15 failed attempts: ~2.84s
+            //   - 20 failed attempts: ~8.67s
+            //   - 25 failed attempts: ~26s
+            //   - 29 failed attempts: ~60s (max out)
+            
+            let start = Double(TimeAmount.milliseconds(100).nanoseconds)
+            let backoffNanoseconds = Int64(start * pow(1.25, Double(self.failedConsecutiveConnectionAttempts)))
+            
+            let backoff: TimeAmount = .nanoseconds(backoffNanoseconds)
+            
+            // Calculate a 3% jitter range
+            let jitterRange = (backoff.nanoseconds / 100) * 3
+            // Pick a random element from the range +/- jitter range.
+            let jitter: TimeAmount = .nanoseconds((-jitterRange...jitterRange).randomElement()!)
+            let jitteredBackoff = backoff + jitter
+            return jitteredBackoff
+        }
     }
 }
 

Diff for: Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift

@@ -62,8 +62,6 @@ extension HTTPConnectionPool {
 
         enum HTTPTypeStateMachine {
             case http1(HTTP1StateMachine)
-
-            case _modifying
         }
 
         var state: HTTPTypeStateMachine
@@ -85,59 +83,31 @@ extension HTTPConnectionPool {
         mutating func executeRequest(_ request: Request) -> Action {
             switch self.state {
             case .http1(var http1StateMachine):
-                return self.state.modify { state -> Action in
-                    let action = http1StateMachine.executeRequest(request)
-                    state = .http1(http1StateMachine)
-                    return action
-                }
-
-            case ._modifying:
-                preconditionFailure("Invalid state: \(self.state)")
+                return http1StateMachine.executeRequest(request)
             }
         }
 
         mutating func newHTTP1ConnectionCreated(_ connection: Connection) -> Action {
             switch self.state {
             case .http1(var httpStateMachine):
-                return self.state.modify { state -> Action in
-                    let action = httpStateMachine.newHTTP1ConnectionEstablished(connection)
-                    state = .http1(httpStateMachine)
-                    return action
-                }
-
-            case ._modifying:
-                preconditionFailure("Invalid state: \(self.state)")
+                return httpStateMachine.newHTTP1ConnectionEstablished(connection)
             }
         }
 
         mutating func failedToCreateNewConnection(_ error: Error, connectionID: Connection.ID) -> Action {
             switch self.state {
             case .http1(var http1StateMachine):
-                return self.state.modify { state -> Action in
-                    let action = http1StateMachine.failedToCreateNewConnection(
-                        error,
-                        connectionID: connectionID
-                    )
-                    state = .http1(http1StateMachine)
-                    return action
-                }
-
-            case ._modifying:
-                preconditionFailure("Invalid state: \(self.state)")
+                return http1StateMachine.failedToCreateNewConnection(
+                    error,
+                    connectionID: connectionID
+                )
             }
         }
 
         mutating func connectionCreationBackoffDone(_ connectionID: Connection.ID) -> Action {
             switch self.state {
             case .http1(var http1StateMachine):
-                return self.state.modify { state -> Action in
-                    let action = http1StateMachine.connectionCreationBackoffDone(connectionID)
-                    state = .http1(http1StateMachine)
-                    return action
-                }
-
-            case ._modifying:
-                preconditionFailure("Invalid state: \(self.state)")
+                return http1StateMachine.connectionCreationBackoffDone(connectionID)
             }
         }
 
@@ -149,14 +119,7 @@ extension HTTPConnectionPool {
         mutating func timeoutRequest(_ requestID: Request.ID) -> Action {
             switch self.state {
             case .http1(var http1StateMachine):
-                return self.state.modify { state -> Action in
-                    let action = http1StateMachine.timeoutRequest(requestID)
-                    state = .http1(http1StateMachine)
-                    return action
-                }
-
-            case ._modifying:
-                preconditionFailure("Invalid state: \(self.state)")
+                return http1StateMachine.timeoutRequest(requestID)
             }
         }
 
@@ -168,55 +131,29 @@ extension HTTPConnectionPool {
         mutating func cancelRequest(_ requestID: Request.ID) -> Action {
             switch self.state {
             case .http1(var http1StateMachine):
-                return self.state.modify { state -> Action in
-                    let action = http1StateMachine.cancelRequest(requestID)
-                    state = .http1(http1StateMachine)
-                    return action
-                }
-
-            case ._modifying:
-                preconditionFailure("Invalid state: \(self.state)")
+                return http1StateMachine.cancelRequest(requestID)
             }
         }
 
         mutating func connectionIdleTimeout(_ connectionID: Connection.ID) -> Action {
             switch self.state {
             case .http1(var http1StateMachine):
-                return self.state.modify { state -> Action in
-                    let action = http1StateMachine.connectionIdleTimeout(connectionID)
-                    state = .http1(http1StateMachine)
-                    return action
-                }
-
-            case ._modifying:
-                preconditionFailure("Invalid state: \(self.state)")
+                return http1StateMachine.connectionIdleTimeout(connectionID)
             }
         }
 
         /// A connection has been closed
         mutating func connectionClosed(_ connectionID: Connection.ID) -> Action {
             switch self.state {
             case .http1(var http1StateMachine):
-                return self.state.modify { state -> Action in
-                    let action = http1StateMachine.connectionClosed(connectionID)
-                    state = .http1(http1StateMachine)
-                    return action
-                }
-
-            case ._modifying:
-                preconditionFailure("Invalid state: \(self.state)")
+                return http1StateMachine.connectionClosed(connectionID)
             }
         }
 
         mutating func http1ConnectionReleased(_ connectionID: Connection.ID) -> Action {
-            guard case .http1(var http1StateMachine) = self.state else {
-                preconditionFailure("Invalid state: \(self.state)")
-            }
-
-            return self.state.modify { state -> Action in
-                let action = http1StateMachine.http1ConnectionReleased(connectionID)
-                state = .http1(http1StateMachine)
-                return action
+            switch self.state {
+            case .http1(var http1StateMachine):
+                return http1StateMachine.http1ConnectionReleased(connectionID)
             }
         }
 
@@ -229,14 +166,7 @@ extension HTTPConnectionPool {
 
             switch self.state {
             case .http1(var http1StateMachine):
-                return self.state.modify { state -> Action in
-                    let action = http1StateMachine.shutdown()
-                    state = .http1(http1StateMachine)
-                    return action
-                }
-
-            case ._modifying:
-                preconditionFailure("Invalid state: \(self.state)")
+                return http1StateMachine.shutdown()
             }
         }
     }
@@ -262,26 +192,11 @@ extension HTTPConnectionPool {
     }
 }
 
-extension HTTPConnectionPool.StateMachine.HTTPTypeStateMachine {
-    mutating func modify<T>(_ closure: (inout Self) throws -> (T)) rethrows -> T {
-        self = ._modifying
-        defer {
-            if case ._modifying = self {
-                preconditionFailure("Invalid state. Use closure to modify state")
-            }
-        }
-        return try closure(&self)
-    }
-}
-
 extension HTTPConnectionPool.StateMachine: CustomStringConvertible {
     var description: String {
         switch self.state {
         case .http1(let http1):
             return ".http1(\(http1))"
-
-        case ._modifying:
-            preconditionFailure("Invalid state: \(self.state)")
         }
     }
 }