Skip to content

make HTTPClient Sendable without @unchecked #852

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 1 commit into from
Aug 12, 2025
+60 −61
Merged

make HTTPClient Sendable without @unchecked #852

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
46 changes: 24 additions & 22 deletions Sources/AsyncHTTPClient/ConnectionPool/HTTPConnectionPool+Manager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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)
}

Expand All @@ -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:
Expand All @@ -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:
Expand Down Expand Up @@ -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
}
}
Expand Down
75 changes: 36 additions & 39 deletions Sources/AsyncHTTPClient/HTTPClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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() })
Expand Down Expand Up @@ -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.
"""
)
}
}
}
}
Expand Down Expand Up @@ -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)
Expand All @@ -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)
}
Expand All @@ -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
}
}

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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 {
Expand Down