Async repository fetching (#8860)

The RepositoryProvider protocol's fetch method is defined as
synchronous. Convert this to async to avoid concurrency workarounds in
clients that might be prone to deadlocks.

Author: plemarquand

Sources/PackageMetadata/PackageMetadata.swift

@@ -235,7 +235,7 @@ public struct PackageSearchClient {
                 return try await withTemporaryDirectory(removeTreeOnDeinit: true) { (tempDir: AbsolutePath) in
                     let tempPath = tempDir.appending(component: url.lastPathComponent)
                     let repositorySpecifier = RepositorySpecifier(url: url)
-                    try self.repositoryProvider.fetch(
+                    try await self.repositoryProvider.fetch(
                         repository: repositorySpecifier,
                         to: tempPath,
                         progressHandler: nil

Sources/SourceControl/GitRepository.swift

@@ -87,7 +87,7 @@ public struct GitRepositoryProvider: RepositoryProvider, Cancellable {
     private let cancellator: Cancellator
     private let git: GitShellHelper
 
-    private var repositoryCache = ThreadSafeKeyValueStore<String, Repository>()
+    private let repositoryCache = ThreadSafeKeyValueStore<String, Repository>()
 
     public init() {
         // helper to cancel outstanding processes
@@ -184,7 +184,7 @@ public struct GitRepositoryProvider: RepositoryProvider, Cancellable {
         repository: RepositorySpecifier,
         to path: Basics.AbsolutePath,
         progressHandler: FetchProgress.Handler? = nil
-    ) throws {
+    ) async throws {
         // Perform a bare clone.
         //
         // NOTE: We intentionally do not create a shallow clone here; the

Sources/SourceControl/Repository.swift

@@ -87,7 +87,7 @@ public protocol RepositoryProvider: Cancellable, Sendable {
     ///   - path: The destination path for the fetch.
     ///   - progress: Reports the progress of the current fetch operation.
     /// - Throws: If there is any error fetching the repository.
-    func fetch(repository: RepositorySpecifier, to path: AbsolutePath, progressHandler: FetchProgress.Handler?) throws
+    func fetch(repository: RepositorySpecifier, to path: AbsolutePath, progressHandler: FetchProgress.Handler?) async throws
 
     /// Open the given repository.
     ///
@@ -164,7 +164,7 @@ public protocol RepositoryProvider: Cancellable, Sendable {
 /// documented. The behavior when this assumption is violated is undefined,
 /// although the expectation is that implementations should throw or crash when
 /// an inconsistency can be detected.
-public protocol Repository {
+public protocol Repository: Sendable {
     /// Get the list of tags in the repository.
     func getTags() throws -> [String]
 

Sources/SourceControl/RepositoryManager.swift

@@ -327,7 +327,7 @@ public class RepositoryManager: Cancellable {
                             }
                             cacheUsed = true
                         } else {
-                            try self.provider.fetch(repository: handle.repository, to: cachedRepositoryPath, progressHandler: updateFetchProgress(progress:))
+                            try await self.provider.fetch(repository: handle.repository, to: cachedRepositoryPath, progressHandler: updateFetchProgress(progress:))
                         }
                         cacheUpdated = true
                         // extra validation to defend from racy edge cases
@@ -362,14 +362,14 @@ public class RepositoryManager: Cancellable {
                     )
                     // it is possible that we already created the directory from failed attempts, so clear leftover data if present.
                     try? self.fileSystem.removeFileTree(repositoryPath)
-                    try self.provider.fetch(repository: handle.repository, to: repositoryPath, progressHandler: updateFetchProgress(progress:))
+                    try await self.provider.fetch(repository: handle.repository, to: repositoryPath, progressHandler: updateFetchProgress(progress:))
                 }
             }
         } else {
             // it is possible that we already created the directory from failed attempts, so clear leftover data if present.
             try? self.fileSystem.removeFileTree(repositoryPath)
             // fetch without populating the cache when no `cachePath` is set.
-            try self.provider.fetch(repository: handle.repository, to: repositoryPath, progressHandler: updateFetchProgress(progress:))
+            try await self.provider.fetch(repository: handle.repository, to: repositoryPath, progressHandler: updateFetchProgress(progress:))
         }
         return FetchDetails(fromCache: cacheUsed, updatedCache: cacheUpdated)
     }

Sources/_InternalTestSupport/InMemoryGitRepository.swift

@@ -431,7 +431,7 @@ public final class InMemoryGitRepositoryProvider: RepositoryProvider {
     // MARK: - RepositoryProvider conformance
     // Note: These methods use force unwrap (instead of throwing) to honor their preconditions.
 
-    public func fetch(repository: RepositorySpecifier, to path: AbsolutePath, progressHandler: FetchProgress.Handler? = nil) throws {
+    public func fetch(repository: RepositorySpecifier, to path: AbsolutePath, progressHandler: FetchProgress.Handler? = nil) async throws {
         guard let repo = specifierMap[RepositorySpecifier(location: repository.location)] else {
             throw InternalError("unknown repo at \(repository.location)")
         }

Tests/SourceControlTests/GitRepositoryTests.swift

@@ -61,11 +61,11 @@ class GitRepositoryTests: XCTestCase {
     }
 
     /// Test the basic provider functions.
-    func testProvider() throws {
+    func testProvider() async throws {
         // Skipping all tests that call git on Windows.
         // We have a hang in CI when running in parallel.
         try XCTSkipOnWindows(because: "https://github.com/swiftlang/swift-package-manager/issues/8564", skipSelfHostedCI: true)
-        try testWithTemporaryDirectory { path in
+        try await testWithTemporaryDirectory { path in
             let testRepoPath = path.appending("test-repo")
             try! makeDirectories(testRepoPath)
             initGitRepo(testRepoPath, tag: "1.2.3")
@@ -75,7 +75,7 @@ class GitRepositoryTests: XCTestCase {
             let provider = GitRepositoryProvider()
             XCTAssertTrue(try provider.workingCopyExists(at: testRepoPath))
             let repoSpec = RepositorySpecifier(path: testRepoPath)
-            try provider.fetch(repository: repoSpec, to: testCheckoutPath)
+            try await provider.fetch(repository: repoSpec, to: testCheckoutPath)
 
             // Verify the checkout was made.
             XCTAssertDirectoryExists(testCheckoutPath)
@@ -87,18 +87,19 @@ class GitRepositoryTests: XCTestCase {
 
             let revision = try repository.resolveRevision(tag: tags.first ?? "<invalid>")
             // FIXME: It would be nice if we had a deterministic hash here...
-            XCTAssertEqual(revision.identifier,
-                try AsyncProcess.popen(
-                    args: Git.tool, "-C", testRepoPath.pathString, "rev-parse", "--verify", "1.2.3").utf8Output().spm_chomp())
+            let testRepoRevParsed = try await AsyncProcess.popen(args: Git.tool, "-C", testRepoPath.pathString, "rev-parse", "--verify", "1.2.3")
+                .utf8Output()
+                .spm_chomp()
+            XCTAssertEqual(revision.identifier, testRepoRevParsed)
+
             if let revision = try? repository.resolveRevision(tag: "<invalid>") {
                 XCTFail("unexpected resolution of invalid tag to \(revision)")
             }
 
             let main = try repository.resolveRevision(identifier: "main")
-
-            XCTAssertEqual(main.identifier,
-                try AsyncProcess.checkNonZeroExit(
-                    args: Git.tool, "-C", testRepoPath.pathString, "rev-parse", "--verify", "main").spm_chomp())
+            let mainRevParsed = try await AsyncProcess.checkNonZeroExit(args: Git.tool, "-C", testRepoPath.pathString, "rev-parse", "--verify", "main")
+                .spm_chomp()
+            XCTAssertEqual(main.identifier, mainRevParsed)
 
             // Check that git hashes resolve to themselves.
             let mainIdentifier = try repository.resolveRevision(identifier: main.identifier)
@@ -213,9 +214,9 @@ class GitRepositoryTests: XCTestCase {
     }
 
     /// Test the Git file system view.
-    func testGitFileView() throws {
+    func testGitFileView() async throws {
         try XCTSkipOnWindows(because: "https://github.com/swiftlang/swift-package-manager/issues/8564", skipSelfHostedCI: true)
-        try testWithTemporaryDirectory { path in
+        try await testWithTemporaryDirectory { path in
             let testRepoPath = path.appending("test-repo")
             try makeDirectories(testRepoPath)
             initGitRepo(testRepoPath)
@@ -242,7 +243,7 @@ class GitRepositoryTests: XCTestCase {
             let testClonePath = path.appending("clone")
             let provider = GitRepositoryProvider()
             let repoSpec = RepositorySpecifier(path: testRepoPath)
-            try provider.fetch(repository: repoSpec, to: testClonePath)
+            try await provider.fetch(repository: repoSpec, to: testClonePath)
             let repository = provider.open(repository: repoSpec, at: testClonePath)
 
             // Get and test the file system view.
@@ -323,7 +324,7 @@ class GitRepositoryTests: XCTestCase {
             let testClonePath = path.appending("clone")
             let provider = GitRepositoryProvider()
             let repoSpec = RepositorySpecifier(path: testRepoPath)
-            try provider.fetch(repository: repoSpec, to: testClonePath)
+            try await provider.fetch(repository: repoSpec, to: testClonePath)
 
             // Clone off a checkout.
             let checkoutPath = path.appending("checkout")
@@ -363,7 +364,7 @@ class GitRepositoryTests: XCTestCase {
             let testClonePath = path.appending("clone")
             let provider = GitRepositoryProvider()
             let repoSpec = RepositorySpecifier(path: testRepoPath)
-            try provider.fetch(repository: repoSpec, to: testClonePath)
+            try await provider.fetch(repository: repoSpec, to: testClonePath)
             let clonedRepo = provider.open(repository: repoSpec, at: testClonePath)
             XCTAssertEqual(try clonedRepo.getTags(), ["1.2.3"])
 
@@ -405,7 +406,7 @@ class GitRepositoryTests: XCTestCase {
             let testClonePath = path.appending("clone")
             let provider = GitRepositoryProvider()
             let repoSpec = RepositorySpecifier(path: testBareRepoPath)
-            try provider.fetch(repository: repoSpec, to: testClonePath)
+            try await provider.fetch(repository: repoSpec, to: testClonePath)
 
             // Clone off a checkout.
             let checkoutPath = path.appending("checkout")
@@ -614,7 +615,7 @@ class GitRepositoryTests: XCTestCase {
             try foo.tag(name: "1.0.0")
 
             // Fetch and clone repo foo.
-            try provider.fetch(repository: fooSpecifier, to: fooRepoPath)
+            try await provider.fetch(repository: fooSpecifier, to: fooRepoPath)
             _ = try await provider.createWorkingCopy(repository: fooSpecifier, sourcePath: fooRepoPath, at: fooWorkingPath, editable: false)
 
             let fooRepo = GitRepository(path: fooRepoPath, isWorkingRepo: false)
@@ -690,7 +691,7 @@ class GitRepositoryTests: XCTestCase {
             let testClonePath = path.appending("clone")
             let provider = GitRepositoryProvider()
             let repoSpec = RepositorySpecifier(path: testRepoPath)
-            try provider.fetch(repository: repoSpec, to: testClonePath)
+            try await provider.fetch(repository: repoSpec, to: testClonePath)
             let clonedRepo = provider.open(repository: repoSpec, at: testClonePath)
             XCTAssertEqual(try clonedRepo.getTags(), ["1.2.3"])
 
@@ -769,7 +770,7 @@ class GitRepositoryTests: XCTestCase {
             let testClonePath = path.appending("clone")
             let provider = GitRepositoryProvider()
             let repoSpec = RepositorySpecifier(path: testRepoPath)
-            try provider.fetch(repository: repoSpec, to: testClonePath)
+            try await provider.fetch(repository: repoSpec, to: testClonePath)
             let clonedRepo = provider.open(repository: repoSpec, at: testClonePath)
             XCTAssertEqual(try clonedRepo.getTags(), [])
 

Tests/SourceControlTests/InMemoryGitRepositoryTests.swift

@@ -93,7 +93,7 @@ struct InMemoryGitRepositoryTests {
         provider.add(specifier: specifier, repository: repo)
 
         let fooRepoPath = AbsolutePath("/fooRepo")
-        try provider.fetch(repository: specifier, to: fooRepoPath)
+        try await provider.fetch(repository: specifier, to: fooRepoPath)
         let fooRepo = try provider.open(repository: specifier, at: fooRepoPath)
 
         // Adding a new tag in original repo shouldn't show up in fetched repo.

Tests/SourceControlTests/RepositoryManagerTests.swift

@@ -508,7 +508,7 @@ final class RepositoryManagerTests: XCTestCase {
                 self.terminatedGroup.enter()
             }
 
-            func fetch(repository: RepositorySpecifier, to path: AbsolutePath, progressHandler: ((FetchProgress) -> Void)?) throws {
+            func fetch(repository: RepositorySpecifier, to path: AbsolutePath, progressHandler: ((FetchProgress) -> Void)?) async throws {
                 print("fetching \(repository)")
                 // startGroup may not be 100% accurate given the blocking nature of the provider so giving it a bit of a buffer
                 DispatchQueue.sharedConcurrent.asyncAfter(deadline: .now() + .milliseconds(100)) {
@@ -592,7 +592,7 @@ final class RepositoryManagerTests: XCTestCase {
                 self.repository = repository
             }
 
-            func fetch(repository: RepositorySpecifier, to path: AbsolutePath, progressHandler: ((FetchProgress) -> Void)?) throws {
+            func fetch(repository: RepositorySpecifier, to path: AbsolutePath, progressHandler: ((FetchProgress) -> Void)?) async throws {
                 assert(repository == self.repository)
                 self.fetch += 1
             }
@@ -716,7 +716,7 @@ private class DummyRepositoryProvider: RepositoryProvider, @unchecked Sendable {
         self.fileSystem = fileSystem
     }
 
-    func fetch(repository: RepositorySpecifier, to path: AbsolutePath, progressHandler: FetchProgress.Handler? = nil) throws {
+    func fetch(repository: RepositorySpecifier, to path: AbsolutePath, progressHandler: FetchProgress.Handler? = nil) async throws {
         assert(!self.fileSystem.exists(path), "\(path) should not exist")
         try self.fileSystem.createDirectory(path, recursive: true)
         try self.fileSystem.writeFileContents(path.appending("readme.md"), string: repository.location.description)