From efb6e3f808792c1f6e5edd88f0e4e62603ed1713 Mon Sep 17 00:00:00 2001 From: George Barnett Date: Mon, 28 Apr 2025 10:38:41 +0100 Subject: [PATCH 1/2] Fix a few simple sendability issues Motivation: We're about to go on a sendability journey. Let's pick some low hanging fruit to get started. Modifications: - Add a few assume-isolated calls - Stop using static var - Use a dispatch group instead of a work item to wait for work to be done. Result: Fewer warnings --- .../ChannelHandler/HTTP1ProxyConnectHandler.swift | 2 +- .../ChannelHandler/SOCKSEventsHandler.swift | 2 +- .../ChannelHandler/TLSEventsHandler.swift | 2 +- .../HTTPConnectionPool+StateMachine.swift | 8 ++++++-- Sources/AsyncHTTPClient/HTTPClient+HTTPCookie.swift | 2 +- Sources/AsyncHTTPClient/HTTPClient.swift | 12 ++++++------ .../NIOTransportServices/TLSConfiguration.swift | 2 +- 7 files changed, 17 insertions(+), 13 deletions(-) diff --git a/Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/HTTP1ProxyConnectHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/HTTP1ProxyConnectHandler.swift index db7b7b7ef..1636fe379 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/HTTP1ProxyConnectHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/HTTP1ProxyConnectHandler.swift @@ -137,7 +137,7 @@ final class HTTP1ProxyConnectHandler: ChannelDuplexHandler, RemovableChannelHand return } - let timeout = context.eventLoop.scheduleTask(deadline: self.deadline) { + let timeout = context.eventLoop.assumeIsolated().scheduleTask(deadline: self.deadline) { switch self.state { case .initialized: preconditionFailure("How can we have a scheduled timeout, if the connection is not even up?") diff --git a/Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/SOCKSEventsHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/SOCKSEventsHandler.swift index a98f97d4d..7458627fd 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/SOCKSEventsHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/SOCKSEventsHandler.swift @@ -99,7 +99,7 @@ final class SOCKSEventsHandler: ChannelInboundHandler, RemovableChannelHandler { return } - let scheduled = context.eventLoop.scheduleTask(deadline: self.deadline) { + let scheduled = context.eventLoop.assumeIsolated().scheduleTask(deadline: self.deadline) { switch self.state { case .initialized, .channelActive: // close the connection, if the handshake timed out diff --git a/Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/TLSEventsHandler.swift b/Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/TLSEventsHandler.swift index bebd0bcc7..d210b2747 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/TLSEventsHandler.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/TLSEventsHandler.swift @@ -104,7 +104,7 @@ final class TLSEventsHandler: ChannelInboundHandler, RemovableChannelHandler { var scheduled: Scheduled? if let deadline = deadline { - scheduled = context.eventLoop.scheduleTask(deadline: deadline) { + scheduled = context.eventLoop.assumeIsolated().scheduleTask(deadline: deadline) { switch self.state { case .initialized, .channelActive: // close the connection, if the handshake timed out diff --git a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift index 6dfd4223e..0cc02cf0f 100644 --- a/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift +++ b/Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift @@ -26,7 +26,9 @@ extension HTTPConnectionPool { self.connection = connection } - static let none = Action(request: .none, connection: .none) + static var none: Action { + Action(request: .none, connection: .none) + } } enum ConnectionAction { @@ -397,7 +399,9 @@ extension HTTPConnectionPool.StateMachine { } struct EstablishedAction { - static let none: Self = .init(request: .none, connection: .none) + static var none: Self { + Self(request: .none, connection: .none) + } let request: HTTPConnectionPool.StateMachine.RequestAction let connection: EstablishedConnectionAction } diff --git a/Sources/AsyncHTTPClient/HTTPClient+HTTPCookie.swift b/Sources/AsyncHTTPClient/HTTPClient+HTTPCookie.swift index ea272a137..759f6728a 100644 --- a/Sources/AsyncHTTPClient/HTTPClient+HTTPCookie.swift +++ b/Sources/AsyncHTTPClient/HTTPClient+HTTPCookie.swift @@ -216,7 +216,7 @@ extension String.UTF8View.SubSequence { } } -private let posixLocale: UnsafeMutableRawPointer = { +nonisolated(unsafe) private let posixLocale: UnsafeMutableRawPointer = { // All POSIX systems must provide a "POSIX" locale, and its date/time formats are US English. // https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap07.html#tag_07_03_05 let _posixLocale = newlocale(LC_TIME_MASK | LC_NUMERIC_MASK, "POSIX", nil)! diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index ff222bd6f..e24965f20 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -223,7 +223,8 @@ public class HTTPClient { ) } let errorStorage: NIOLockedValueBox = NIOLockedValueBox(nil) - let continuation = DispatchWorkItem {} + let dispatchGroup = DispatchGroup() + dispatchGroup.enter() self.shutdown(requiresCleanClose: requiresCleanClose, queue: DispatchQueue(label: "async-http-client.shutdown")) { error in if let error = error { @@ -231,9 +232,9 @@ public class HTTPClient { errorStorage = error } } - continuation.perform() + dispatchGroup.leave() } - continuation.wait() + dispatchGroup.wait() try errorStorage.withLockedValue { errorStorage in if let error = errorStorage { throw error @@ -756,14 +757,13 @@ public class HTTPClient { delegate: delegate ) - var deadlineSchedule: Scheduled? if let deadline = deadline { - deadlineSchedule = taskEL.scheduleTask(deadline: deadline) { + let deadlineSchedule = taskEL.scheduleTask(deadline: deadline) { requestBag.deadlineExceeded() } task.promise.futureResult.whenComplete { _ in - deadlineSchedule?.cancel() + deadlineSchedule.cancel() } } diff --git a/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift b/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift index ef505e3b7..e8278e095 100644 --- a/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift +++ b/Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift @@ -60,7 +60,7 @@ extension TLSVersion { @available(macOS 10.14, iOS 12.0, tvOS 12.0, watchOS 5.0, *) extension TLSConfiguration { /// Dispatch queue used by Network framework TLS to control certificate verification - static var tlsDispatchQueue = DispatchQueue(label: "TLSDispatch") + static let tlsDispatchQueue = DispatchQueue(label: "TLSDispatch") /// create NWProtocolTLS.Options for use with NIOTransportServices from the NIOSSL TLSConfiguration /// From 70de79c8ea33eea8de8871307525cf7ba8e4915c Mon Sep 17 00:00:00 2001 From: George Barnett Date: Mon, 28 Apr 2025 13:50:07 +0100 Subject: [PATCH 2/2] Avoid waiting on a dispatch group --- Sources/AsyncHTTPClient/HTTPClient.swift | 54 ++++++++++++++++++------ 1 file changed, 40 insertions(+), 14 deletions(-) diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift index e24965f20..e523a8f7d 100644 --- a/Sources/AsyncHTTPClient/HTTPClient.swift +++ b/Sources/AsyncHTTPClient/HTTPClient.swift @@ -222,24 +222,50 @@ public class HTTPClient { """ ) } - let errorStorage: NIOLockedValueBox = NIOLockedValueBox(nil) - let dispatchGroup = DispatchGroup() - dispatchGroup.enter() - self.shutdown(requiresCleanClose: requiresCleanClose, queue: DispatchQueue(label: "async-http-client.shutdown")) - { error in - if let error = error { - errorStorage.withLockedValue { errorStorage in - errorStorage = error + + final class ShutdownError: @unchecked Sendable { + // @unchecked because error is protected by lock. + + // Stores whether the shutdown has happened or not. + private let lock: ConditionLock + private var error: Error? + + init() { + self.error = nil + self.lock = ConditionLock(value: false) + } + + func didShutdown(_ error: (any Error)?) { + self.lock.lock(whenValue: false) + defer { + self.lock.unlock(withValue: true) } + self.error = error } - dispatchGroup.leave() - } - dispatchGroup.wait() - try errorStorage.withLockedValue { errorStorage in - if let error = errorStorage { - throw error + + func blockUntilShutdown() -> (any Error)? { + self.lock.lock(whenValue: true) + defer { + self.lock.unlock(withValue: true) + } + return self.error } } + + let shutdownError = ShutdownError() + + self.shutdown( + requiresCleanClose: requiresCleanClose, + queue: DispatchQueue(label: "async-http-client.shutdown") + ) { error in + shutdownError.didShutdown(error) + } + + let error = shutdownError.blockUntilShutdown() + + if let error = error { + throw error + } } /// Shuts down the client and event loop gracefully.