make HTTPClient Sendable without @unchecked (#852)

Previously, `HTTPClient` used `@unchecked Sendable`, now it's compiler
checked.

Author: weissi

Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Manager.swift

@@ -19,24 +19,26 @@ import NIOCore
 import NIOHTTP1
 
 extension HTTPConnectionPool {
-    final class Manager {
+    final class Manager: Sendable {
         private typealias Key = ConnectionPool.Key
 
-        private enum State {
+        private enum RunState: Sendable {
             case active
             case shuttingDown(promise: EventLoopPromise<Bool>?, unclean: Bool)
             case shutDown
         }
 
+        private struct State: Sendable {
+            var runState: RunState = .active
+            var pools: [Key: HTTPConnectionPool] = [:]
+        }
+
         private let eventLoopGroup: EventLoopGroup
         private let configuration: HTTPClient.Configuration
         private let connectionIDGenerator = Connection.ID.globalGenerator
         private let logger: Logger
 
-        private var state: State = .active
-        private var _pools: [Key: HTTPConnectionPool] = [:]
-        private let lock = NIOLock()
-
+        private let state: NIOLockedValueBox<State> = NIOLockedValueBox(State())
         private let sslContextCache = SSLContextCache()
 
         init(
@@ -51,10 +53,10 @@ extension HTTPConnectionPool {
 
         func executeRequest(_ request: HTTPSchedulableRequest) {
             let poolKey = request.poolKey
-            let poolResult = self.lock.withLock { () -> Result<HTTPConnectionPool, HTTPClientError> in
-                switch self.state {
+            let poolResult = self.state.withLockedValue { state -> Result<HTTPConnectionPool, HTTPClientError> in
+                switch state.runState {
                 case .active:
-                    if let pool = self._pools[poolKey] {
+                    if let pool = state.pools[poolKey] {
                         return .success(pool)
                     }
 
@@ -68,7 +70,7 @@ extension HTTPConnectionPool {
                         idGenerator: self.connectionIDGenerator,
                         backgroundActivityLogger: self.logger
                     )
-                    self._pools[poolKey] = pool
+                    state.pools[poolKey] = pool
                     return .success(pool)
 
                 case .shuttingDown, .shutDown:
@@ -95,17 +97,17 @@ extension HTTPConnectionPool {
                 case shutdown([Key: HTTPConnectionPool])
             }
 
-            let action = self.lock.withLock { () -> ShutdownAction in
-                switch self.state {
+            let action = self.state.withLockedValue { state -> ShutdownAction in
+                switch state.runState {
                 case .active:
                     // If there aren't any pools, we can mark the pool as shut down right away.
-                    if self._pools.isEmpty {
-                        self.state = .shutDown
+                    if state.pools.isEmpty {
+                        state.runState = .shutDown
                         return .done(promise)
                     } else {
                         // this promise will be succeeded once all connection pools are shutdown
-                        self.state = .shuttingDown(promise: promise, unclean: false)
-                        return .shutdown(self._pools)
+                        state.runState = .shuttingDown(promise: promise, unclean: false)
+                        return .shutdown(state.pools)
                     }
 
                 case .shuttingDown, .shutDown:
@@ -135,23 +137,23 @@ extension HTTPConnectionPool.Manager: HTTPConnectionPoolDelegate {
             case wait
         }
 
-        let closeAction = self.lock.withLock { () -> CloseAction in
-            switch self.state {
+        let closeAction = self.state.withLockedValue { state -> CloseAction in
+            switch state.runState {
             case .active, .shutDown:
                 preconditionFailure("Why are pools shutting down, if the manager did not give a signal")
 
             case .shuttingDown(let promise, let soFarUnclean):
-                guard self._pools.removeValue(forKey: pool.key) === pool else {
+                guard state.pools.removeValue(forKey: pool.key) === pool else {
                     preconditionFailure(
                         "Expected that the pool was created by this manager and is known for this reason."
                     )
                 }
 
-                if self._pools.isEmpty {
-                    self.state = .shutDown
+                if state.pools.isEmpty {
+                    state.runState = .shutDown
                     return .close(promise, unclean: soFarUnclean || unclean)
                 } else {
-                    self.state = .shuttingDown(promise: promise, unclean: soFarUnclean || unclean)
+                    state.runState = .shuttingDown(promise: promise, unclean: soFarUnclean || unclean)
                     return .wait
                 }
             }

Sources/AsyncHTTPClient/HTTPClient.swift

@@ -57,7 +57,7 @@ let globalRequestID = ManagedAtomic(0)
 ///         }
 ///     }
 /// ```
-public class HTTPClient {
+public final class HTTPClient: Sendable {
     /// The `EventLoopGroup` in use by this ``HTTPClient``.
     ///
     /// All HTTP transactions will occur on loops owned by this group.
@@ -66,11 +66,9 @@ public class HTTPClient {
     let poolManager: HTTPConnectionPool.Manager
 
     /// Shared thread pool used for file IO. It is lazily created on first access of ``Task/fileIOThreadPool``.
-    private var fileIOThreadPool: NIOThreadPool?
-    private let fileIOThreadPoolLock = NIOLock()
+    private let fileIOThreadPool: NIOLockedValueBox<NIOThreadPool?>
 
-    private var state: State
-    private let stateLock = NIOLock()
+    private let state: NIOLockedValueBox<State>
     private let canBeShutDown: Bool
 
     static let loggingDisabled = Logger(label: "AHC-do-not-log", factory: { _ in SwiftLogNoOpLogHandler() })
@@ -167,29 +165,32 @@ public class HTTPClient {
             configuration: self.configuration,
             backgroundActivityLogger: backgroundActivityLogger
         )
-        self.state = .upAndRunning
+        self.state = NIOLockedValueBox(.upAndRunning)
+        self.fileIOThreadPool = NIOLockedValueBox(nil)
     }
 
     deinit {
         debugOnly {
             // We want to crash only in debug mode.
-            switch self.state {
-            case .shutDown:
-                break
-            case .shuttingDown:
-                preconditionFailure(
-                    """
-                    This state should be totally unreachable. While the HTTPClient is shutting down a \
-                    reference cycle should exist, that prevents it from deinit.
-                    """
-                )
-            case .upAndRunning:
-                preconditionFailure(
-                    """
-                    Client not shut down before the deinit. Please call client.shutdown() when no \
-                    longer needed. Otherwise memory will leak.
-                    """
-                )
+            self.state.withLockedValue { state in
+                switch state {
+                case .shutDown:
+                    break
+                case .shuttingDown:
+                    preconditionFailure(
+                        """
+                        This state should be totally unreachable. While the HTTPClient is shutting down a \
+                        reference cycle should exist, that prevents it from deinit.
+                        """
+                    )
+                case .upAndRunning:
+                    preconditionFailure(
+                        """
+                        Client not shut down before the deinit. Please call client.shutdown() when no \
+                        longer needed. Otherwise memory will leak.
+                        """
+                    )
+                }
             }
         }
     }
@@ -302,11 +303,11 @@ public class HTTPClient {
             return
         }
         do {
-            try self.stateLock.withLock {
-                guard case .upAndRunning = self.state else {
+            try self.state.withLockedValue { state in
+                guard case .upAndRunning = state else {
                     throw HTTPClientError.alreadyShutdown
                 }
-                self.state = .shuttingDown(requiresCleanClose: requiresCleanClose, callback: callback)
+                state = .shuttingDown(requiresCleanClose: requiresCleanClose, callback: callback)
             }
         } catch {
             callback(error)
@@ -320,17 +321,16 @@ public class HTTPClient {
             case .failure:
                 preconditionFailure("Shutting down the connection pool must not fail, ever.")
             case .success(let unclean):
-                let (callback, uncleanError) = self.stateLock.withLock { () -> (ShutdownCallback, Error?) in
-                    guard case .shuttingDown(let requiresClean, callback: let callback) = self.state else {
+                let (callback, uncleanError) = self.state.withLockedValue {
+                    (state: inout HTTPClient.State) -> (ShutdownCallback, Error?) in
+                    guard case .shuttingDown(let requiresClean, callback: let callback) = state else {
                         preconditionFailure("Why did the pool manager shut down, if it was not instructed to")
                     }
 
                     let error: Error? = (requiresClean && unclean) ? HTTPClientError.uncleanShutdown : nil
+                    state = .shutDown
                     return (callback, error)
                 }
-                self.stateLock.withLock {
-                    self.state = .shutDown
-                }
                 queue.async {
                     callback(uncleanError)
                 }
@@ -340,11 +340,11 @@ public class HTTPClient {
 
     @Sendable
     private func makeOrGetFileIOThreadPool() -> NIOThreadPool {
-        self.fileIOThreadPoolLock.withLock {
-            guard let fileIOThreadPool = self.fileIOThreadPool else {
+        self.fileIOThreadPool.withLockedValue { pool in
+            guard let pool else {
                 return NIOThreadPool.singleton
             }
-            return fileIOThreadPool
+            return pool
         }
     }
 
@@ -734,8 +734,8 @@ public class HTTPClient {
             ]
         )
 
-        let failedTask: Task<Delegate.Response>? = self.stateLock.withLock {
-            switch self.state {
+        let failedTask: Task<Delegate.Response>? = self.state.withLockedValue { state in
+            switch state {
             case .upAndRunning:
                 return nil
             case .shuttingDown, .shutDown:
@@ -1113,9 +1113,6 @@ extension HTTPClient.Configuration: Sendable {}
 extension HTTPClient.EventLoopGroupProvider: Sendable {}
 extension HTTPClient.EventLoopPreference: Sendable {}
 
-// HTTPClient is thread-safe because its shared mutable state is protected through a lock
-extension HTTPClient: @unchecked Sendable {}
-
 extension HTTPClient.Configuration {
     /// Timeout configuration.
     public struct Timeout: Sendable {