Skip to content

Fix a few simple sendability issues #832

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 2 commits into from
Apr 28, 2025
Merged

Fix a few simple sendability issues #832

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
efb6e3f
Fix a few simple sendability issues
glbrntt Apr 28, 2025
70de79c
Avoid waiting on a dispatch group
glbrntt Apr 28, 2025
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
2 changes: 1 addition & 1 deletion Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/HTTP1ProxyConnectHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?")
Expand Down
2 changes: 1 addition & 1 deletion Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/SOCKSEventsHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Sources/AsyncHTTPClient/ConnectionPool/ChannelHandler/TLSEventsHandler.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ final class TLSEventsHandler: ChannelInboundHandler, RemovableChannelHandler {

var scheduled: Scheduled<Void>?
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
Expand Down
8 changes: 6 additions & 2 deletions Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+StateMachine.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ extension HTTPConnectionPool {
self.connection = connection
}

static let none = Action(request: .none, connection: .none)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially silly question but was this unsafe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not "unsafe" per-se as neither .none case contains anything. However, Action isn't Sendable (and shouldn't be) so the compiler appropriately produces a warning here.

static var none: Action {
Action(request: .none, connection: .none)
}
}

enum ConnectionAction {
Expand Down Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/AsyncHTTPClient/HTTPClient+HTTPCookie.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)!
Expand Down
12 changes: 6 additions & 6 deletions Sources/AsyncHTTPClient/HTTPClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,17 +223,18 @@ public class HTTPClient {
)
}
let errorStorage: NIOLockedValueBox<Error?> = NIOLockedValueBox(nil)
let continuation = DispatchWorkItem {}
let dispatchGroup = DispatchGroup()
dispatchGroup.enter()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this should probably use the condition variable workaround we used in NIO to avoid triggering the main thread checker.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, good idea

self.shutdown(requiresCleanClose: requiresCleanClose, queue: DispatchQueue(label: "async-http-client.shutdown"))
{ error in
if let error = error {
errorStorage.withLockedValue { errorStorage in
errorStorage = error
}
}
continuation.perform()
dispatchGroup.leave()
}
continuation.wait()
dispatchGroup.wait()
try errorStorage.withLockedValue { errorStorage in
if let error = errorStorage {
throw error
Expand Down Expand Up @@ -756,14 +757,13 @@ public class HTTPClient {
delegate: delegate
)

var deadlineSchedule: Scheduled<Void>?
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()
}
}

Expand Down
2 changes: 1 addition & 1 deletion Sources/AsyncHTTPClient/NIOTransportServices/TLSConfiguration.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
///
Expand Down
Loading