From 08b1b43c7b27580f3f0535580e1d6728e687980e Mon Sep 17 00:00:00 2001 From: Max Desiatov Date: Wed, 10 Apr 2024 13:17:18 +0100 Subject: [PATCH 1/2] Enable strict concurrency checks on `Basics` module Also refactored `URLSessionHTTPClient` to remove `weak` references, which were incompatible with strict concurrency checks. --- Package.swift | 84 +++++--- Sources/Basics/Archiver/Archiver.swift | 20 +- Sources/Basics/Archiver/TarArchiver.swift | 6 +- .../Basics/Archiver/UniversalArchiver.swift | 6 +- Sources/Basics/Archiver/ZipArchiver.swift | 6 +- Sources/Basics/Cancellator.swift | 6 +- .../Concurrency/ConcurrencyHelpers.swift | 2 +- Sources/Basics/Concurrency/TokenBucket.swift | 2 +- .../FileSystem/FileSystem+Extensions.swift | 2 +- Sources/Basics/FileSystem/TemporaryFile.swift | 2 +- .../Basics/HTTPClient/LegacyHTTPClient.swift | 2 +- .../HTTPClient/URLSessionHTTPClient.swift | 199 +++++------------- Sources/PackageRegistry/RegistryClient.swift | 6 +- .../RegistryDownloadsManager.swift | 2 +- 14 files changed, 133 insertions(+), 212 deletions(-) diff --git a/Package.swift b/Package.swift index 5ca57fece39..31bdadc7950 100644 --- a/Package.swift +++ b/Package.swift @@ -12,26 +12,38 @@ // //===----------------------------------------------------------------------===// -import PackageDescription import class Foundation.ProcessInfo +import PackageDescription // When building the toolchain on the CI for ELF platforms, remove the CI's // stdlib absolute runpath and add ELF's $ORIGIN relative paths before installing. -let swiftpmLinkSettings : [LinkerSetting] -let packageLibraryLinkSettings : [LinkerSetting] +let swiftpmLinkSettings: [LinkerSetting] +let packageLibraryLinkSettings: [LinkerSetting] if let resourceDirPath = ProcessInfo.processInfo.environment["SWIFTCI_INSTALL_RPATH_OS"] { - swiftpmLinkSettings = [ .unsafeFlags(["-no-toolchain-stdlib-rpath", "-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../lib/swift/\(resourceDirPath)"]) ] - packageLibraryLinkSettings = [ .unsafeFlags(["-no-toolchain-stdlib-rpath", "-Xlinker", "-rpath", "-Xlinker", "$ORIGIN/../../\(resourceDirPath)"]) ] + swiftpmLinkSettings = [.unsafeFlags([ + "-no-toolchain-stdlib-rpath", + "-Xlinker", + "-rpath", + "-Xlinker", + "$ORIGIN/../lib/swift/\(resourceDirPath)", + ])] + packageLibraryLinkSettings = [.unsafeFlags([ + "-no-toolchain-stdlib-rpath", + "-Xlinker", + "-rpath", + "-Xlinker", + "$ORIGIN/../../\(resourceDirPath)", + ])] } else { - swiftpmLinkSettings = [] - packageLibraryLinkSettings = [] + swiftpmLinkSettings = [] + packageLibraryLinkSettings = [] } /** SwiftPMDataModel is the subset of SwiftPM product that includes just its data model. -This allows some clients (such as IDEs) that use SwiftPM's data model but not its build system -to not have to depend on SwiftDriver, SwiftLLBuild, etc. We should probably have better names here, -though that could break some clients. -*/ + This allows some clients (such as IDEs) that use SwiftPM's data model but not its build system + to not have to depend on SwiftDriver, SwiftLLBuild, etc. We should probably have better names here, + though that could break some clients. + */ let swiftPMDataModelProduct = ( name: "SwiftPMDataModel", targets: [ @@ -51,7 +63,7 @@ let swiftPMDataModelProduct = ( command line tools, while `libSwiftPMDataModel` includes only the data model. NOTE: This API is *unstable* and may change at any time. -*/ + */ let swiftPMProduct = ( name: "SwiftPM", targets: swiftPMDataModelProduct.targets + [ @@ -69,11 +81,10 @@ let systemSQLitePkgConfig: String? = "sqlite3" #endif /** An array of products which have two versions listed: one dynamically linked, the other with the -automatic linking type with `-auto` suffix appended to product's name. -*/ + automatic linking type with `-auto` suffix appended to product's name. + */ let autoProducts = [swiftPMProduct, swiftPMDataModelProduct] - let packageModelResourcesSettings: [SwiftSetting] let packageModelResources: [Resource] if ProcessInfo.processInfo.environment["SWIFTPM_USE_LIBRARIES_METADATA"] == nil { @@ -90,11 +101,11 @@ let package = Package( name: "SwiftPM", platforms: [ .macOS(.v13), - .iOS(.v16) + .iOS(.v16), ], products: - autoProducts.flatMap { - [ + autoProducts.flatMap { + [ .library( name: $0.name, type: .dynamic, @@ -103,9 +114,9 @@ let package = Package( .library( name: "\($0.name)-auto", targets: $0.targets - ) - ] - } + [ + ), + ] + } + [ .library( name: "XCBuildSupport", targets: ["XCBuildSupport"] @@ -166,7 +177,7 @@ let package = Package( name: "SourceKitLSPAPI", dependencies: [ "Build", - "SPMBuildCore" + "SPMBuildCore", ], exclude: ["CMakeLists.txt"], swiftSettings: [.enableExperimentalFeature("AccessLevelOnImport")] @@ -185,7 +196,10 @@ let package = Package( .product(name: "SwiftToolsSupport-auto", package: "swift-tools-support-core"), .product(name: "SystemPackage", package: "swift-system"), ], - exclude: ["CMakeLists.txt", "Vendor/README.md"] + exclude: ["CMakeLists.txt", "Vendor/README.md"], + swiftSettings: [ + .enableExperimentalFeature("StrictConcurrency=complete"), + ] ), .target( @@ -213,7 +227,7 @@ let package = Package( name: "SourceControl", dependencies: [ "Basics", - "PackageModel" + "PackageModel", ], exclude: ["CMakeLists.txt"] ), @@ -255,7 +269,7 @@ let package = Package( dependencies: [ "Basics", "PackageLoading", - "PackageModel" + "PackageModel", ], exclude: ["CMakeLists.txt", "README.md"] ), @@ -267,7 +281,7 @@ let package = Package( name: "PackageCollectionsModel", dependencies: [], exclude: [ - "Formats/v1.md" + "Formats/v1.md", ] ), @@ -301,7 +315,7 @@ let package = Package( ], exclude: ["CMakeLists.txt"] ), - + .target( name: "PackageSigning", dependencies: [ @@ -320,7 +334,7 @@ let package = Package( name: "SPMBuildCore", dependencies: [ "Basics", - "PackageGraph" + "PackageGraph", ], exclude: ["CMakeLists.txt"] ), @@ -517,7 +531,7 @@ let package = Package( "Commands", "SwiftSDKCommand", "PackageCollectionsCommand", - "PackageRegistryCommand" + "PackageRegistryCommand", ], linkerSettings: swiftpmLinkSettings ), @@ -562,7 +576,8 @@ let package = Package( .target( /** Test for thread-santizer. */ name: "tsan_utils", - dependencies: []), + dependencies: [] + ), // MARK: SwiftPM tests @@ -664,7 +679,7 @@ let package = Package( name: "XCBuildSupportTests", dependencies: ["XCBuildSupport", "SPMTestSupport"], exclude: ["Inputs/Foo.pc"] - ) + ), ], swiftLanguageVersions: [.v5] ) @@ -677,12 +692,13 @@ package.targets.append(contentsOf: [ name: "FunctionalPerformanceTests", dependencies: [ "swift-package-manager", - "SPMTestSupport" + "SPMTestSupport", ] ), ]) -// rdar://101868275 "error: cannot find 'XCTAssertEqual' in scope" can affect almost any functional test, so we flat out disable them all until we know what is going on +// rdar://101868275 "error: cannot find 'XCTAssertEqual' in scope" can affect almost any functional test, so we flat out +// disable them all until we know what is going on if ProcessInfo.processInfo.environment["SWIFTCI_DISABLE_SDK_DEPENDENT_TESTS"] == nil { package.targets.append(contentsOf: [ .testTarget( @@ -690,7 +706,7 @@ if ProcessInfo.processInfo.environment["SWIFTCI_DISABLE_SDK_DEPENDENT_TESTS"] == dependencies: [ "swift-package-manager", "PackageModel", - "SPMTestSupport" + "SPMTestSupport", ] ), diff --git a/Sources/Basics/Archiver/Archiver.swift b/Sources/Basics/Archiver/Archiver.swift index b76130b1500..e9d416ef21d 100644 --- a/Sources/Basics/Archiver/Archiver.swift +++ b/Sources/Basics/Archiver/Archiver.swift @@ -13,7 +13,7 @@ import _Concurrency /// The `Archiver` protocol abstracts away the different operations surrounding archives. -public protocol Archiver { +public protocol Archiver: Sendable { /// A set of extensions the current archiver supports. var supportedExtensions: Set { get } @@ -27,7 +27,7 @@ public protocol Archiver { func extract( from archivePath: AbsolutePath, to destinationPath: AbsolutePath, - completion: @escaping (Result) -> Void + completion: @escaping @Sendable (Result) -> Void ) /// Asynchronously compress the contents of a directory to a destination archive. @@ -40,7 +40,7 @@ public protocol Archiver { func compress( directory: AbsolutePath, to destinationPath: AbsolutePath, - completion: @escaping (Result) -> Void + completion: @escaping @Sendable (Result) -> Void ) /// Asynchronously validates if a file is an archive. @@ -51,7 +51,7 @@ public protocol Archiver { @available(*, noasync, message: "Use the async alternative") func validate( path: AbsolutePath, - completion: @escaping (Result) -> Void + completion: @escaping @Sendable (Result) -> Void ) } @@ -65,8 +65,8 @@ extension Archiver { from archivePath: AbsolutePath, to destinationPath: AbsolutePath ) async throws { - try await withCheckedThrowingContinuation { - self.extract(from: archivePath, to: destinationPath, completion: $0.resume(with:)) + try await withCheckedThrowingContinuation { continuation in + self.extract(from: archivePath, to: destinationPath, completion: { continuation.resume(with: $0) }) } } @@ -79,8 +79,8 @@ extension Archiver { directory: AbsolutePath, to destinationPath: AbsolutePath ) async throws { - try await withCheckedThrowingContinuation { - self.compress(directory: directory, to: destinationPath, completion: $0.resume(with:)) + try await withCheckedThrowingContinuation { continuation in + self.compress(directory: directory, to: destinationPath, completion: { continuation.resume(with: $0) }) } } @@ -91,8 +91,8 @@ extension Archiver { public func validate( path: AbsolutePath ) async throws -> Bool { - try await withCheckedThrowingContinuation { - self.validate(path: path, completion: $0.resume(with:)) + try await withCheckedThrowingContinuation { continuation in + self.validate(path: path, completion: { continuation.resume(with: $0) }) } } } diff --git a/Sources/Basics/Archiver/TarArchiver.swift b/Sources/Basics/Archiver/TarArchiver.swift index d1da7ed0c15..99948bf82a9 100644 --- a/Sources/Basics/Archiver/TarArchiver.swift +++ b/Sources/Basics/Archiver/TarArchiver.swift @@ -47,7 +47,7 @@ public struct TarArchiver: Archiver { public func extract( from archivePath: AbsolutePath, to destinationPath: AbsolutePath, - completion: @escaping (Result) -> Void + completion: @escaping @Sendable (Result) -> Void ) { do { guard self.fileSystem.exists(archivePath) else { @@ -84,7 +84,7 @@ public struct TarArchiver: Archiver { public func compress( directory: AbsolutePath, to destinationPath: AbsolutePath, - completion: @escaping (Result) -> Void + completion: @escaping @Sendable (Result) -> Void ) { do { guard self.fileSystem.isDirectory(directory) else { @@ -115,7 +115,7 @@ public struct TarArchiver: Archiver { } } - public func validate(path: AbsolutePath, completion: @escaping (Result) -> Void) { + public func validate(path: AbsolutePath, completion: @escaping @Sendable (Result) -> Void) { do { guard self.fileSystem.exists(path) else { throw FileSystemError(.noEntry, path.underlying) diff --git a/Sources/Basics/Archiver/UniversalArchiver.swift b/Sources/Basics/Archiver/UniversalArchiver.swift index cbd5d5d742f..d6ed496df97 100644 --- a/Sources/Basics/Archiver/UniversalArchiver.swift +++ b/Sources/Basics/Archiver/UniversalArchiver.swift @@ -73,7 +73,7 @@ public struct UniversalArchiver: Archiver { public func extract( from archivePath: AbsolutePath, to destinationPath: AbsolutePath, - completion: @escaping (Result) -> Void + completion: @escaping @Sendable (Result) -> Void ) { do { let archiver = try archiver(for: archivePath) @@ -86,7 +86,7 @@ public struct UniversalArchiver: Archiver { public func compress( directory: AbsolutePath, to destinationPath: AbsolutePath, - completion: @escaping (Result) -> Void + completion: @escaping @Sendable (Result) -> Void ) { do { let archiver = try archiver(for: destinationPath) @@ -98,7 +98,7 @@ public struct UniversalArchiver: Archiver { public func validate( path: AbsolutePath, - completion: @escaping (Result) -> Void + completion: @escaping @Sendable (Result) -> Void ) { do { let archiver = try archiver(for: path) diff --git a/Sources/Basics/Archiver/ZipArchiver.swift b/Sources/Basics/Archiver/ZipArchiver.swift index 9aab24e13ce..092752d8382 100644 --- a/Sources/Basics/Archiver/ZipArchiver.swift +++ b/Sources/Basics/Archiver/ZipArchiver.swift @@ -37,7 +37,7 @@ public struct ZipArchiver: Archiver, Cancellable { public func extract( from archivePath: AbsolutePath, to destinationPath: AbsolutePath, - completion: @escaping (Result) -> Void + completion: @escaping @Sendable (Result) -> Void ) { do { guard self.fileSystem.exists(archivePath) else { @@ -77,7 +77,7 @@ public struct ZipArchiver: Archiver, Cancellable { public func compress( directory: AbsolutePath, to destinationPath: AbsolutePath, - completion: @escaping (Result) -> Void + completion: @escaping @Sendable (Result) -> Void ) { do { guard self.fileSystem.isDirectory(directory) else { @@ -125,7 +125,7 @@ public struct ZipArchiver: Archiver, Cancellable { } } - public func validate(path: AbsolutePath, completion: @escaping (Result) -> Void) { + public func validate(path: AbsolutePath, completion: @escaping @Sendable (Result) -> Void) { do { guard self.fileSystem.exists(path) else { throw FileSystemError(.noEntry, path.underlying) diff --git a/Sources/Basics/Cancellator.swift b/Sources/Basics/Cancellator.swift index d2c808e88ef..e1e8f8d0434 100644 --- a/Sources/Basics/Cancellator.swift +++ b/Sources/Basics/Cancellator.swift @@ -18,9 +18,9 @@ import class TSCBasic.Thread import WinSDK #endif -public typealias CancellationHandler = (DispatchTime) throws -> Void +public typealias CancellationHandler = @Sendable (DispatchTime) throws -> Void -public final class Cancellator: Cancellable { +public final class Cancellator: Cancellable, Sendable { public typealias RegistrationKey = String private let observabilityScope: ObservabilityScope? @@ -119,7 +119,7 @@ public final class Cancellator: Cancellable { } @discardableResult - public func register(name: String, handler: @escaping () throws -> Void) -> RegistrationKey? { + public func register(name: String, handler: @escaping @Sendable () throws -> Void) -> RegistrationKey? { self.register(name: name, handler: { _ in try handler() }) } diff --git a/Sources/Basics/Concurrency/ConcurrencyHelpers.swift b/Sources/Basics/Concurrency/ConcurrencyHelpers.swift index 3a10d61a81c..7927d2e2dec 100644 --- a/Sources/Basics/Concurrency/ConcurrencyHelpers.swift +++ b/Sources/Basics/Concurrency/ConcurrencyHelpers.swift @@ -64,7 +64,7 @@ public func safe_async( } /// Bridges between potentially blocking methods that take a result completion closure and async/await -public func safe_async(_ body: @escaping (@escaping (Result) -> Void) -> Void) async -> T { +public func safe_async(_ body: @escaping @Sendable (@escaping (Result) -> Void) -> Void) async -> T { await withCheckedContinuation { continuation in // It is possible that body make block indefinitely on a lock, semaphore, // or similar then synchronously call the completion handler. For full safety diff --git a/Sources/Basics/Concurrency/TokenBucket.swift b/Sources/Basics/Concurrency/TokenBucket.swift index e9cf6ff4251..010da630a35 100644 --- a/Sources/Basics/Concurrency/TokenBucket.swift +++ b/Sources/Basics/Concurrency/TokenBucket.swift @@ -30,7 +30,7 @@ public actor TokenBucket { /// invocations of `withToken` will suspend until a "free" token is available. /// - Parameter body: The closure to invoke when a token is available. /// - Returns: Resulting value returned by `body`. - public func withToken( + public func withToken( _ body: @Sendable () async throws -> ReturnType ) async rethrows -> ReturnType { await self.getToken() diff --git a/Sources/Basics/FileSystem/FileSystem+Extensions.swift b/Sources/Basics/FileSystem/FileSystem+Extensions.swift index ac0c0ace718..f31604263e7 100644 --- a/Sources/Basics/FileSystem/FileSystem+Extensions.swift +++ b/Sources/Basics/FileSystem/FileSystem+Extensions.swift @@ -25,7 +25,7 @@ import var TSCBasic.localFileSystem import protocol TSCBasic.WritableByteStream public typealias FileSystem = TSCBasic.FileSystem -public var localFileSystem = TSCBasic.localFileSystem +public let localFileSystem = TSCBasic.localFileSystem // MARK: - Custom path diff --git a/Sources/Basics/FileSystem/TemporaryFile.swift b/Sources/Basics/FileSystem/TemporaryFile.swift index 756f5022f4a..a9f253b6f08 100644 --- a/Sources/Basics/FileSystem/TemporaryFile.swift +++ b/Sources/Basics/FileSystem/TemporaryFile.swift @@ -72,7 +72,7 @@ public func withTemporaryDirectory( dir: AbsolutePath? = nil, prefix: String = "TemporaryDirectory", removeTreeOnDeinit: Bool = false, - _ body: @escaping (AbsolutePath) async throws -> Result + _ body: @escaping @Sendable (AbsolutePath) async throws -> Result ) throws -> Task { try withTemporaryDirectory(fileSystem: fileSystem, dir: dir, prefix: prefix) { path, cleanup in defer { if removeTreeOnDeinit { cleanup(path) } } diff --git a/Sources/Basics/HTTPClient/LegacyHTTPClient.swift b/Sources/Basics/HTTPClient/LegacyHTTPClient.swift index b5294ccfabf..524459e54a9 100644 --- a/Sources/Basics/HTTPClient/LegacyHTTPClient.swift +++ b/Sources/Basics/HTTPClient/LegacyHTTPClient.swift @@ -25,7 +25,7 @@ public final class LegacyHTTPClient: Cancellable { public typealias Configuration = LegacyHTTPClientConfiguration public typealias Request = LegacyHTTPClientRequest public typealias Response = HTTPClientResponse - public typealias Handler = (Request, ProgressHandler?, @escaping (Result) -> Void) -> Void + public typealias Handler = (Request, ProgressHandler?, @escaping @Sendable (Result) -> Void) -> Void public typealias ProgressHandler = @Sendable (_ bytesReceived: Int64, _ totalBytes: Int64?) throws -> Void public typealias CompletionHandler = @Sendable (Result) -> Void diff --git a/Sources/Basics/HTTPClient/URLSessionHTTPClient.swift b/Sources/Basics/HTTPClient/URLSessionHTTPClient.swift index b706b173d39..7c27608749d 100644 --- a/Sources/Basics/HTTPClient/URLSessionHTTPClient.swift +++ b/Sources/Basics/HTTPClient/URLSessionHTTPClient.swift @@ -14,18 +14,41 @@ import _Concurrency import Foundation import struct TSCUtility.Versioning #if canImport(FoundationNetworking) -// FIXME: this brings OpenSSL dependency on Linux -// need to decide how to best deal with that +// FIXME: this brings OpenSSL dependency on Linux and needs to be replaced with `swift-server/async-http-client` package import FoundationNetworking #endif final class URLSessionHTTPClient: Sendable { + private let dataSession: URLSession + private let downloadSession: URLSession private let dataTaskManager: DataTaskManager private let downloadTaskManager: DownloadTaskManager init(configuration: URLSessionConfiguration = .default) { - self.dataTaskManager = DataTaskManager(configuration: configuration) - self.downloadTaskManager = DownloadTaskManager(configuration: configuration) + let dataDelegateQueue = OperationQueue() + dataDelegateQueue.name = "org.swift.swiftpm.urlsession-http-client-data-delegate" + dataDelegateQueue.maxConcurrentOperationCount = 1 + self.dataTaskManager = DataTaskManager() + self.dataSession = URLSession( + configuration: configuration, + delegate: self.dataTaskManager, + delegateQueue: dataDelegateQueue + ) + + let downloadDelegateQueue = OperationQueue() + downloadDelegateQueue.name = "org.swift.swiftpm.urlsession-http-client-download-delegate" + downloadDelegateQueue.maxConcurrentOperationCount = 1 + self.downloadTaskManager = DownloadTaskManager() + self.downloadSession = URLSession( + configuration: configuration, + delegate: self.downloadTaskManager, + delegateQueue: downloadDelegateQueue + ) + } + + deinit { + dataSession.finishTasksAndInvalidate() + downloadSession.finishTasksAndInvalidate() } @Sendable @@ -38,22 +61,28 @@ final class URLSessionHTTPClient: Sendable { let task: URLSessionTask switch request.kind { case .generic: - task = self.dataTaskManager.makeTask( + let dataTask = self.dataSession.dataTask(with: urlRequest) + self.dataTaskManager.register( + task: dataTask, urlRequest: urlRequest, authorizationProvider: request.options.authorizationProvider, progress: progress, completion: { continuation.resume(with: $0) } ) + task = dataTask case .download(_, let destination): - task = self.downloadTaskManager.makeTask( + let downloadTask = self.downloadSession.downloadTask(with: urlRequest) + self.downloadTaskManager.register( + task: downloadTask, urlRequest: urlRequest, - // FIXME: always using a synchronous filesystem, because `URLSessionDownloadDelegate` + // FIXME: always using synchronous filesystem, because `URLSessionDownloadDelegate` // needs temporary files to moved out of temporary locations synchronously in delegate callbacks. fileSystem: localFileSystem, destination: destination, progress: progress, completion: { continuation.resume(with: $0) } ) + task = downloadTask } task.resume() } @@ -69,101 +98,41 @@ final class URLSessionHTTPClient: Sendable { let task: URLSessionTask switch request.kind { case .generic: - task = self.dataTaskManager.makeTask( + let dataTask = self.dataSession.dataTask(with: urlRequest) + self.dataTaskManager.register( + task: dataTask, urlRequest: urlRequest, authorizationProvider: request.options.authorizationProvider, progress: progress, completion: completion ) + task = dataTask case .download(let fileSystem, let destination): - task = self.downloadTaskManager.makeTask( + let downloadTask = self.downloadSession.downloadTask(with: urlRequest) + self.downloadTaskManager.register( + task: downloadTask, urlRequest: urlRequest, fileSystem: fileSystem, destination: destination, progress: progress, completion: completion ) + task = downloadTask } task.resume() } } -/// A weak wrapper around `DataTaskManager` that conforms to `URLSessionDataDelegate`. -/// -/// This ensures that we don't get a retain cycle between `DataTaskManager.session` -> `URLSession.delegate` -> `DataTaskManager`. -/// -/// The `DataTaskManager` is being kept alive by a reference from all `DataTask`s that it manages. Once all the -/// `DataTasks` have finished and are deallocated, `DataTaskManager` will get deinitialized, which invalidates the -/// session, which then lets go of `WeakDataTaskManager`. -private class WeakDataTaskManager: NSObject, URLSessionDataDelegate { - private weak var dataTaskManager: DataTaskManager? - - init(_ dataTaskManager: DataTaskManager? = nil) { - self.dataTaskManager = dataTaskManager - } - - func urlSession( - _ session: URLSession, - dataTask: URLSessionDataTask, - didReceive response: URLResponse, - completionHandler: @escaping (URLSession.ResponseDisposition) -> Void - ) { - dataTaskManager?.urlSession( - session, - dataTask: dataTask, - didReceive: response, - completionHandler: completionHandler - ) - } - - func urlSession(_ session: URLSession, dataTask: URLSessionDataTask, didReceive data: Data) { - dataTaskManager?.urlSession(session, dataTask: dataTask, didReceive: data) - } - - func urlSession(_ session: URLSession, task: URLSessionTask, didCompleteWithError error: Error?) { - dataTaskManager?.urlSession(session, task: task, didCompleteWithError: error) - } - - func urlSession( - _ session: URLSession, - task: URLSessionTask, - willPerformHTTPRedirection response: HTTPURLResponse, - newRequest request: URLRequest, - completionHandler: @escaping (URLRequest?) -> Void - ) { - dataTaskManager?.urlSession( - session, - task: task, - willPerformHTTPRedirection: response, - newRequest: request, - completionHandler: completionHandler - ) - } -} - -private final class DataTaskManager: @unchecked Sendable { +private final class DataTaskManager: NSObject, URLSessionDataDelegate { private let tasks = ThreadSafeKeyValueStore() - private let delegateQueue: OperationQueue - private var session: URLSession! - - public init(configuration: URLSessionConfiguration) { - self.delegateQueue = OperationQueue() - self.delegateQueue.name = "org.swift.swiftpm.urlsession-http-client-data-delegate" - self.delegateQueue.maxConcurrentOperationCount = 1 - self.session = URLSession(configuration: configuration, delegate: WeakDataTaskManager(self), delegateQueue: self.delegateQueue) - } - deinit { - session.finishTasksAndInvalidate() - } - - func makeTask( + func register( + task: URLSessionDataTask, urlRequest: URLRequest, authorizationProvider: LegacyHTTPClientConfiguration.AuthorizationProvider?, progress: LegacyHTTPClient.ProgressHandler?, completion: @escaping LegacyHTTPClient.CompletionHandler - ) -> URLSessionDataTask { - let task = self.session.dataTask(with: urlRequest) + ) { self.tasks[task.taskIdentifier] = DataTask( task: task, progressHandler: progress, @@ -171,7 +140,6 @@ private final class DataTaskManager: @unchecked Sendable { completionHandler: completion, authorizationProvider: authorizationProvider ) - return task } public func urlSession( @@ -281,73 +249,17 @@ private final class DataTaskManager: @unchecked Sendable { } } -/// This uses the same pattern as `WeakDataTaskManager`. See comment on that type. -private class WeakDownloadTaskManager: NSObject, URLSessionDownloadDelegate { - private weak var downloadTaskManager: DownloadTaskManager? - - init(_ downloadTaskManager: DownloadTaskManager? = nil) { - self.downloadTaskManager = downloadTaskManager - } - - func urlSession( - _ session: URLSession, - downloadTask: URLSessionDownloadTask, - didWriteData bytesWritten: Int64, - totalBytesWritten: Int64, - totalBytesExpectedToWrite: Int64 - ) { - downloadTaskManager?.urlSession( - session, - downloadTask: downloadTask, - didWriteData: bytesWritten, - totalBytesWritten: totalBytesWritten, - totalBytesExpectedToWrite: totalBytesExpectedToWrite - ) - } - - func urlSession( - _ session: URLSession, - downloadTask: URLSessionDownloadTask, - didFinishDownloadingTo location: URL - ) { - downloadTaskManager?.urlSession(session, downloadTask: downloadTask, didFinishDownloadingTo: location) - } - - func urlSession( - _ session: URLSession, - task downloadTask: URLSessionTask, - didCompleteWithError error: Error? - ) { - downloadTaskManager?.urlSession(session, task: downloadTask, didCompleteWithError: error) - } -} - -private final class DownloadTaskManager: @unchecked Sendable { +private final class DownloadTaskManager: NSObject, URLSessionDownloadDelegate { private let tasks = ThreadSafeKeyValueStore() - 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) { - self.delegateQueue = OperationQueue() - self.delegateQueue.name = "org.swift.swiftpm.urlsession-http-client-download-delegate" - self.delegateQueue.maxConcurrentOperationCount = 1 - self.session = URLSession(configuration: configuration, delegate: WeakDownloadTaskManager(self), delegateQueue: self.delegateQueue) - } - - deinit { - session.finishTasksAndInvalidate() - } - - func makeTask( + func register( + task: URLSessionDownloadTask, urlRequest: URLRequest, fileSystem: FileSystem, destination: AbsolutePath, progress: LegacyHTTPClient.ProgressHandler?, completion: @escaping LegacyHTTPClient.CompletionHandler - ) -> URLSessionDownloadTask { - let task = self.session.downloadTask(with: urlRequest) + ) { self.tasks[task.taskIdentifier] = DownloadTask( task: task, fileSystem: fileSystem, @@ -356,7 +268,6 @@ private final class DownloadTaskManager: @unchecked Sendable { progressHandler: progress, completionHandler: completion ) - return task } func urlSession( @@ -430,11 +341,6 @@ private final class DownloadTaskManager: @unchecked Sendable { let task: URLSessionDownloadTask let fileSystem: FileSystem let destination: AbsolutePath - /// A strong reference to keep the `DownloadTaskManager` alive so it can handle the callbacks from the - /// `URLSession`. - /// - /// See comment on `WeakDownloadTaskManager`. - private let downloadTaskManager: DownloadTaskManager let progressHandler: LegacyHTTPClient.ProgressHandler? let completionHandler: LegacyHTTPClient.CompletionHandler @@ -451,7 +357,6 @@ private final class DownloadTaskManager: @unchecked Sendable { self.task = task self.fileSystem = fileSystem self.destination = destination - self.downloadTaskManager = downloadTaskManager self.progressHandler = progressHandler self.completionHandler = completionHandler } diff --git a/Sources/PackageRegistry/RegistryClient.swift b/Sources/PackageRegistry/RegistryClient.swift index a13fc4cd8f6..96445280282 100644 --- a/Sources/PackageRegistry/RegistryClient.swift +++ b/Sources/PackageRegistry/RegistryClient.swift @@ -983,7 +983,7 @@ public final class RegistryClient: Cancellable { package: PackageIdentity, version: Version, destinationPath: AbsolutePath, - progressHandler: ((_ bytesReceived: Int64, _ totalBytes: Int64?) -> Void)?, + progressHandler: (@Sendable (_ bytesReceived: Int64, _ totalBytes: Int64?) -> Void)?, timeout: DispatchTimeInterval? = .none, fileSystem: FileSystem, observabilityScope: ObservabilityScope, @@ -1009,7 +1009,7 @@ public final class RegistryClient: Cancellable { package: PackageIdentity, version: Version, destinationPath: AbsolutePath, - progressHandler: ((_ bytesReceived: Int64, _ totalBytes: Int64?) -> Void)?, + progressHandler: (@Sendable (_ bytesReceived: Int64, _ totalBytes: Int64?) -> Void)?, timeout: DispatchTimeInterval? = .none, fileSystem: FileSystem, observabilityScope: ObservabilityScope, @@ -1063,7 +1063,7 @@ public final class RegistryClient: Cancellable { package: PackageIdentity.RegistryIdentity, version: Version, destinationPath: AbsolutePath, - progressHandler: ((_ bytesReceived: Int64, _ totalBytes: Int64?) -> Void)?, + progressHandler: (@Sendable (_ bytesReceived: Int64, _ totalBytes: Int64?) -> Void)?, timeout: DispatchTimeInterval?, fileSystem: FileSystem, observabilityScope: ObservabilityScope, diff --git a/Sources/PackageRegistry/RegistryDownloadsManager.swift b/Sources/PackageRegistry/RegistryDownloadsManager.swift index 3c24234bfef..12462ff744b 100644 --- a/Sources/PackageRegistry/RegistryDownloadsManager.swift +++ b/Sources/PackageRegistry/RegistryDownloadsManager.swift @@ -163,7 +163,7 @@ public class RegistryDownloadsManager: Cancellable { observabilityScope: ObservabilityScope, delegateQueue: DispatchQueue, callbackQueue: DispatchQueue, - completion: @escaping (Result) -> Void + completion: @escaping @Sendable (Result) -> Void ) { if let cachePath { do { From f2388d6b908cb7b12483b6c3c955d3c085369b1c Mon Sep 17 00:00:00 2001 From: Max Desiatov Date: Mon, 15 Apr 2024 18:14:54 +0100 Subject: [PATCH 2/2] Update Package.swift --- Package.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Package.swift b/Package.swift index 01c580c4f75..ee74542952e 100644 --- a/Package.swift +++ b/Package.swift @@ -194,7 +194,7 @@ let package = Package( ], exclude: ["CMakeLists.txt", "Vendor/README.md"], swiftSettings: [ - .enableExperimentalFeature("StrictConcurrency=complete"), + .enableExperimentalFeature("StrictConcurrency"), ] ),