Skip to content

Fix sendability warnings in URLSessionHTTPClient #7441

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 3 commits into from
Apr 9, 2024
Merged

Fix sendability warnings in URLSessionHTTPClient #7441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
524bdf6
Fix sendability warnings in `URLSessionHTTPClient`
MaxDesiatov Apr 8, 2024
2407028
Fix build error
MaxDesiatov Apr 8, 2024
b34f5dd
Fix warning
MaxDesiatov Apr 8, 2024
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
7 changes: 3 additions & 4 deletions Sources/Basics/AuthorizationProvider.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ import struct Foundation.URL
import Security
#endif

public protocol AuthorizationProvider {
@Sendable
public protocol AuthorizationProvider: Sendable {
func authentication(for url: URL) -> (user: String, password: String)?
}

Expand Down Expand Up @@ -80,7 +79,7 @@ extension AuthorizationProvider {

// MARK: - netrc

public class NetrcAuthorizationProvider: AuthorizationProvider, AuthorizationWriter {
public final class NetrcAuthorizationProvider: AuthorizationProvider, AuthorizationWriter {
// marked internal for testing
internal let path: AbsolutePath
private let fileSystem: FileSystem
Expand Down Expand Up @@ -202,7 +201,7 @@ public class NetrcAuthorizationProvider: AuthorizationProvider, AuthorizationWri
// MARK: - Keychain

#if canImport(Security)
public class KeychainAuthorizationProvider: AuthorizationProvider, AuthorizationWriter {
public final class KeychainAuthorizationProvider: AuthorizationProvider, AuthorizationWriter {
private let observabilityScope: ObservabilityScope

private let cache = ThreadSafeKeyValueStore<String, (user: String, password: String)>()
Expand Down
18 changes: 9 additions & 9 deletions Sources/Basics/HTTPClient/LegacyHTTPClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@ public final class LegacyHTTPClient: Cancellable {
public typealias Request = LegacyHTTPClientRequest
public typealias Response = HTTPClientResponse
public typealias Handler = (Request, ProgressHandler?, @escaping (Result<Response, Error>) -> Void) -> Void
public typealias ProgressHandler = (_ bytesReceived: Int64, _ totalBytes: Int64?) throws -> Void
public typealias CompletionHandler = (Result<HTTPClientResponse, Error>) -> Void
public typealias ProgressHandler = @Sendable (_ bytesReceived: Int64, _ totalBytes: Int64?) throws -> Void
public typealias CompletionHandler = @Sendable (Result<HTTPClientResponse, Error>) -> Void

public var configuration: LegacyHTTPClientConfiguration
private let underlying: Handler
Expand Down Expand Up @@ -121,7 +121,7 @@ public final class LegacyHTTPClient: Cancellable {
requestNumber: 0,
observabilityScope: observabilityScope,
progress: progress.map { handler in
{ received, expected in
{ @Sendable received, expected in
// call back on the requested queue
callbackQueue.async {
do {
Expand Down Expand Up @@ -312,7 +312,7 @@ extension LegacyHTTPClient {
headers: HTTPClientHeaders = .init(),
options: Request.Options = .init(),
observabilityScope: ObservabilityScope? = .none,
completion: @escaping (Result<Response, Error>) -> Void
completion: @Sendable @escaping (Result<Response, Error>) -> Void
Copy link
Contributor

Choose a reason for hiding this comment

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

Stylistic nit: I think the preferred ordering for function types is @escaping @Sendable

) {
self.execute(
Request(method: .head, url: url, headers: headers, body: nil, options: options),
Expand All @@ -326,7 +326,7 @@ extension LegacyHTTPClient {
headers: HTTPClientHeaders = .init(),
options: Request.Options = .init(),
observabilityScope: ObservabilityScope? = .none,
completion: @escaping (Result<Response, Error>) -> Void
completion: @Sendable @escaping (Result<Response, Error>) -> Void
) {
self.execute(
Request(method: .get, url: url, headers: headers, body: nil, options: options),
Expand All @@ -341,7 +341,7 @@ extension LegacyHTTPClient {
headers: HTTPClientHeaders = .init(),
options: Request.Options = .init(),
observabilityScope: ObservabilityScope? = .none,
completion: @escaping (Result<Response, Error>) -> Void
completion: @Sendable @escaping (Result<Response, Error>) -> Void
) {
self.execute(
Request(method: .put, url: url, headers: headers, body: body, options: options),
Expand All @@ -356,7 +356,7 @@ extension LegacyHTTPClient {
headers: HTTPClientHeaders = .init(),
options: Request.Options = .init(),
observabilityScope: ObservabilityScope? = .none,
completion: @escaping (Result<Response, Error>) -> Void
completion: @Sendable @escaping (Result<Response, Error>) -> Void
) {
self.execute(
Request(method: .post, url: url, headers: headers, body: body, options: options),
Expand All @@ -370,7 +370,7 @@ extension LegacyHTTPClient {
headers: HTTPClientHeaders = .init(),
options: Request.Options = .init(),
observabilityScope: ObservabilityScope? = .none,
completion: @escaping (Result<Response, Error>) -> Void
completion: @Sendable @escaping (Result<Response, Error>) -> Void
) {
self.execute(
Request(method: .delete, url: url, headers: headers, body: nil, options: options),
Expand All @@ -383,7 +383,7 @@ extension LegacyHTTPClient {
// MARK: - LegacyHTTPClientConfiguration

public struct LegacyHTTPClientConfiguration {
public typealias AuthorizationProvider = (URL) -> String?
public typealias AuthorizationProvider = @Sendable (URL) -> String?

public var requestHeaders: HTTPClientHeaders?
public var requestTimeout: DispatchTimeInterval?
Expand Down
31 changes: 19 additions & 12 deletions Sources/Basics/HTTPClient/URLSessionHTTPClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import struct TSCUtility.Versioning
import FoundationNetworking
#endif

final class URLSessionHTTPClient {
final class URLSessionHTTPClient: Sendable {
private let dataTaskManager: DataTaskManager
private let downloadTaskManager: DownloadTaskManager

Expand All @@ -42,7 +42,7 @@ final class URLSessionHTTPClient {
urlRequest: urlRequest,
authorizationProvider: request.options.authorizationProvider,
progress: progress,
completion: continuation.resume(with:)
completion: { continuation.resume(with: $0) }
)
case .download(_, let destination):
task = self.downloadTaskManager.makeTask(
Expand All @@ -52,13 +52,14 @@ final class URLSessionHTTPClient {
fileSystem: localFileSystem,
destination: destination,
progress: progress,
completion: continuation.resume(with:)
completion: { continuation.resume(with: $0) }
)
}
task.resume()
}
}

@Sendable
public func execute(
_ request: LegacyHTTPClient.Request,
progress: LegacyHTTPClient.ProgressHandler?,
Expand Down Expand Up @@ -140,8 +141,8 @@ private class WeakDataTaskManager: NSObject, URLSessionDataDelegate {
}
}

private class DataTaskManager {
private var tasks = ThreadSafeKeyValueStore<Int, DataTask>()
private final class DataTaskManager: @unchecked Sendable {
private let tasks = ThreadSafeKeyValueStore<Int, DataTask>()
private let delegateQueue: OperationQueue
private var session: URLSession!

Expand Down Expand Up @@ -179,11 +180,13 @@ private class DataTaskManager {
didReceive response: URLResponse,
completionHandler: @escaping (URLSession.ResponseDisposition) -> Void
) {
guard let task = self.tasks[dataTask.taskIdentifier] else {
guard var task = self.tasks[dataTask.taskIdentifier] else {
return completionHandler(.cancel)
}
task.response = response as? HTTPURLResponse
task.expectedContentLength = response.expectedContentLength
self.tasks[dataTask.taskIdentifier] = task

do {
try task.progressHandler?(0, response.expectedContentLength)
completionHandler(.allow)
Expand All @@ -193,14 +196,15 @@ private class DataTaskManager {
}

public func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive data: Data) {
guard let task = self.tasks[dataTask.taskIdentifier] else {
guard var task = self.tasks[dataTask.taskIdentifier] else {
return
}
if task.buffer != nil {
task.buffer?.append(data)
} else {
task.buffer = data
}
self.tasks[dataTask.taskIdentifier] = task

do {
// safe since created in the line above
Expand Down Expand Up @@ -246,7 +250,7 @@ private class DataTaskManager {
completionHandler(request)
}

class DataTask {
struct DataTask: Sendable {
let task: URLSessionDataTask
let completionHandler: LegacyHTTPClient.CompletionHandler
/// A strong reference to keep the `DataTaskManager` alive so it can handle the callbacks from the
Expand Down Expand Up @@ -318,9 +322,11 @@ private class WeakDownloadTaskManager: NSObject, URLSessionDownloadDelegate {
}
}

private class DownloadTaskManager {
private var tasks = ThreadSafeKeyValueStore<Int, DownloadTask>()
private final class DownloadTaskManager: @unchecked Sendable {
private let tasks = ThreadSafeKeyValueStore<Int, DownloadTask>()
private let delegateQueue: OperationQueue

// FIXME: can't be `let` instead of `var`, as `URLSession` holds a reference to `self`.
private var session: URLSession!

init(configuration: URLSessionConfiguration) {
Expand Down Expand Up @@ -379,7 +385,7 @@ private class DownloadTaskManager {
downloadTask: URLSessionDownloadTask,
didFinishDownloadingTo location: URL
) {
guard let task = self.tasks[downloadTask.taskIdentifier] else {
guard var task = self.tasks[downloadTask.taskIdentifier] else {
return
}

Expand All @@ -392,6 +398,7 @@ private class DownloadTaskManager {
try task.fileSystem.move(from: path, to: task.destination)
} catch {
task.moveFileError = error
self.tasks[downloadTask.taskIdentifier] = task
}
}

Expand Down Expand Up @@ -419,7 +426,7 @@ private class DownloadTaskManager {
}
}

class DownloadTask {
struct DownloadTask: Sendable {
let task: URLSessionDownloadTask
let fileSystem: FileSystem
let destination: AbsolutePath
Expand Down
2 changes: 0 additions & 2 deletions Sources/Build/BuildOperation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@

import Basics

import Build
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated warning, but probably not worth a separate one-liner PR, so chucking that in here instead.


import LLBuildManifest
import PackageGraph
import PackageLoading
Expand Down
2 changes: 1 addition & 1 deletion Sources/PackageRegistry/RegistryClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1651,7 +1651,7 @@ public final class RegistryClient: Cancellable {
result.tryMap { response in
observabilityScope
.emit(
debug: "server response for \(request.url): \(response.statusCode) in \(start.distance(to: .now()).descriptionInSeconds)"
debug: "server response for \(url): \(response.statusCode) in \(start.distance(to: .now()).descriptionInSeconds)"
)
switch response.statusCode {
case 201:
Expand Down