Async git repository opening #8721

plemarquand · 2025-05-24T16:45:00Z

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

This involved porting RepositoryManager.lookup to structured concurrency. The method has some concurrency constraints that are preserved:

There is a limit (maxConcurrentOperations) on how many simultaneous lookups can be performed concurrently.
If a lookup is requested and one is in flight for the same RepositorySpecifier, the in flight request completes before the new one is started.

The `RepositoryProvider` protocol's `open` method is defined as synchronous. Convert this to `async` to avoid concurrency workarounds in clients that might be prone to deadlocks. This involved porting `RepositoryManager.lookup` to structured concurrency. The method has some concurrency constraints that are preserved: - There is a limit (`maxConcurrentOperations`) on how many simultaneous lookups can be performed concurrently. - If a lookup is requested and one is in flight for the same `RepositorySpecifier`, the in flight request completes before the new one is started. This PR also moves away from using a `DispatchQueue` to make delegate calls, instead opting to use a `Task` that calls through an `actor` based proxy to the underlying delegate. Gating these calls through an `actor` allows us to ensure the calls are processed sequentially as they were with the `DispatchQueue`.

Sources/Basics/Concurrency/ConcurrencyHelpers.swift

FranzBusch · 2025-05-26T14:07:06Z

Sources/Basics/Concurrency/ConcurrencyHelpers.swift

+
+    private func waitIfNeeded() async {
+        if activeTasks >= concurrentTasks {
+            await withCheckedContinuation { continuation in


We should add a withTaskCancellationHandler here and potentially a withTaskPriorityEscalationHandler

@FranzBusch would it be sufficient to propagate cancellation by wrapping the waitingTasks.append in a Task.isCancelled check?

if !Task.isCancelled { waitingTasks.append(continuation) }

No that's not enough since the task can be cancelled after the continuation has been appended. In general, to be structured concurrency and cancellation compliant whenever you create a continuation you need to wrap it into a withTaskCancellationHandler. In this case you probably want to remove that continuation from the waitingTasks and resume it with a CancellationError.

Since you are currently using an actor this won't work easily without an unstructured task. So I would recommend you to switch over to a class and protect the state with a Mutex.

Ah, makes sense. Unfortunately SwiftPM's supported platforms is set to .macOS(.v13) and Mutex is only available on >= .v15, so I'll have to use an NSLock as we do elsewhere in SwiftPM.

Is there a reason why we can't bump to macOS 15.0?

Yes. We cannot require macOS 15 at this time.

I've updated the AsyncOperationQueue to respect cancellation of the parent task. Incorporating a Mutex polyfill can come later and bulk replace the state: T/stateLock: NSLock pattern used throughout SwiftPM.

Sources/Basics/Concurrency/ConcurrencyHelpers.swift

jakepetroules · 2025-05-27T04:11:33Z

Sources/Basics/Concurrency/ConcurrencyHelpers.swift

+                self.waitingTasksLock.withLock {
+                    if let taskIndex = self.waitingTasks.firstIndex(where: { $0.0 == taskId }) {
+                        let task = self.waitingTasks.remove(at: taskIndex)
+                        task.1.resume(throwing: CancellationError())


resuming continuations while a lock is held is dangerous and can lead to deadlocks. Note withTaskCancellationHandler's documentation:

Cancellation handlers which acquire locks must take care to avoid deadlock. The cancellation handler may be invoked while holding internal locks associated with the task or other tasks. Other operations on the task, such as resuming a continuation, may acquire these same internal locks. Therefore, if a cancellation handler must acquire a lock, other code should not cancel tasks or resume continuations while holding that lock.

I think here you can return the continuation out of the lock and then resume it after releasing the lock.

Good catch, I've fixed this up in two places

Sources/Basics/Concurrency/ConcurrencyHelpers.swift

FranzBusch · 2025-05-27T07:10:04Z

Sources/Basics/Concurrency/ConcurrencyHelpers.swift

+            try await withTaskCancellationHandler {
+                try await withCheckedThrowingContinuation { (continuation: CheckedContinuation<Void, any Error>) in
+                    if !Task.isCancelled {
+                        waitingTasksLock.withLock {
+                            waitingTasks.append((taskId, continuation))
+                        }
+                    } else {
+                        continuation.resume(throwing: CancellationError())
+                    }
+                }
+            } onCancel: {
+                // If the parent task is cancelled then we need to manually handle resuming the
+                // continuation for the waiting task with a `CancellationError`.
+                self.waitingTasksLock.withLock {
+                    if let taskIndex = self.waitingTasks.firstIndex(where: { $0.0 == taskId }) {
+                        let task = self.waitingTasks.remove(at: taskIndex)
+                        task.1.resume(throwing: CancellationError())
+                    }
+                }
+            }


@jakepetroules raised a good point with resuming while a lock is held. In general, never do any outcalls while a lock is held. FWIW, we have run into exactly this deadlock before.

Now the first part of this code is also not 100% correct. There is subtle race that can happen in this code in this scenario:

Task is running not cancelled

You are checking Task.isCancelled returns false

Something is canceling your task

The task cancellation handler runs and acquires the lock. Tries to remove the continuation but it hasn't been stored yet so resumes nothing

The code after the Task.isCancelled runs and stores the continuation

In this case you now stored a continuation of a cancelled task and the task cancellation handler already ran. What you have to do instead is to model a tri-state for a waiter:

enum Waiter { case creating case waiting(Continuation) case cancelled }

This makes sense, thanks. I've adopted a tri-state and made sure that the continuation always resumes with a cancellation error when appropriate.

…in .creating state

plemarquand · 2025-05-29T17:55:22Z

@FranzBusch @jakepetroules I've addressed comments and added some tests for AsyncOperationQueue. Could you take another look?

jakepetroules · 2025-05-29T21:00:57Z

Sources/Basics/Concurrency/ConcurrencyHelpers.swift

+
+    deinit {
+        waitingTasksLock.withLock {
+            if !waitingTasks.filter({ $0.continuation != nil }).isEmpty {


Isn't it unexpected if there is anything at all in waitingTasks, not just those with a non-nil continuation?

Yep, you're right, I've updated to revert this check.

FranzBusch · 2025-06-01T11:10:17Z

Sources/Basics/Concurrency/ConcurrencyHelpers.swift

+        let taskId = ID()
+        waitingTasksLock.withLock {
+            waitingTasks.append(.creating(taskId))
+        }


We could move this into the above lock and return an optional UUID. This way we don't need to acquire the lock more than once for the initial setup

FranzBusch · 2025-06-01T11:13:54Z

Sources/Basics/Concurrency/ConcurrencyHelpers.swift

+    fileprivate typealias WaitingContinuation = CheckedContinuation<Void, any Error>
+
+    private let concurrentTasks: Int
+    private var activeTasks: Int = 0


Do we need this counter at all or can we just use waitingTasks.count? Keeping the two in sync can be tricky and I had to triple check that the code does it correctly. That requires us to change the WaitingTask to be just WorkTask with a new case but IMO that would make this code a lot clearer.

We can, but it that would mean the waitingTasks would no longer be able to be modelled as a queue since the actively running tasks would be contained within it.

I think an OrderedDictionary could work instead, but runs in to the swift-collections in swift-build question.

FranzBusch · 2025-06-01T11:15:28Z

Sources/Basics/Concurrency/ConcurrencyHelpers.swift

+
+                    // If the task was cancelled in between creating the task cancellation handler and aquiring the lock,
+                    // we should resume the continuation with a `CancellationError`.
+                    if case .cancelled = waitingTasks[index] {


Instead of writing an if case I would prefer if we exhaustively switch over the returned task here similar to how you did it in the onCancel case. I always recommend doing that since it makes sure every case is properly handled.

FranzBusch · 2025-06-01T11:17:57Z

Sources/Basics/Concurrency/ConcurrencyHelpers.swift

+                case .waiting:
+                    activeTasks -= 1
+                    // Begin the next waiting task
+                    return waitingTasks.remove(at: 0).continuation


Since we are operating under FIFO here it is worth using a Deque instead of an array since this remove(at:) is going to reallocate the entire array.

I was trying to keep in mind this will make its way back in to swift-build, which has no dependency on swift-collections and looks to be trying to keep its dependencies minimal. @jakepetroules is that accurate, or would swift-build accept a swift-collections dependency for this?

FranzBusch · 2025-06-01T11:21:11Z

Sources/Basics/Concurrency/ConcurrencyHelpers.swift

+            try await withCheckedThrowingContinuation { (continuation: WaitingContinuation) -> Void in
+                let action: TaskAction? = waitingTasksLock.withLock {
+                    guard let index = waitingTasks.firstIndex(where: { $0.id == taskId }) else {
+                        // If the task was cancelled in onCancelled it will have been removed from the waiting tasks list.


This comment is not true right? We are always going to get an index by just looking at the code in onCancel. Either we find a .cancelled case or we have run before. There is no way for onCancel and this check to interleave in the way you describe here. Now, what can interleave is the signalCompletion that also appears to remove cancelled() tasks. Can we update this comment?

Thanks, I've tightend up this comment to reflect how this guard code would actually get called.

plemarquand · 2025-06-02T14:30:39Z

@FranzBusch I've tried to simplify the code as suggested by removing the counter state. It did complicate the code in signalCompletion a bit because we can't start processing immediately from the head of the collection.

FranzBusch

LGTM. Just one small nit

Sources/Basics/Concurrency/ConcurrencyHelpers.swift

plemarquand · 2025-06-03T13:21:49Z

Thank you for your tireless reviews @FranzBusch!

The `RepositoryProvider` protocol's `open` method is defined as synchronous. Convert this to `async` to avoid concurrency workarounds in clients that might be prone to deadlocks. This involved porting `RepositoryManager.lookup` to structured concurrency. The method has some concurrency constraints that are preserved: - There is a limit (`maxConcurrentOperations`) on how many simultaneous lookups can be performed concurrently. - If a lookup is requested and one is in flight for the same `RepositorySpecifier`, the in flight request completes before the new one is started.

plemarquand requested review from jakepetroules, dschaefer2, shawnhyam, bripeticca, owenv, bkhouri, cmcgee1024 and daveyc123 as code owners May 24, 2025 16:45

jakepetroules reviewed May 24, 2025

View reviewed changes

Sources/Basics/Concurrency/ConcurrencyHelpers.swift Outdated Show resolved Hide resolved

Use AsyncOperationQueue from swift-build

c88e9c0

FranzBusch reviewed May 26, 2025

View reviewed changes

dschaefer2 approved these changes May 26, 2025

View reviewed changes

Handle cancellation in AsyncOperaionQueue

87395c4

jakepetroules requested changes May 27, 2025

View reviewed changes

jakepetroules reviewed May 27, 2025

View reviewed changes

Sources/Basics/Concurrency/ConcurrencyHelpers.swift Outdated Show resolved Hide resolved

FranzBusch reviewed May 27, 2025

View reviewed changes

Address comments

de316cc

plemarquand requested a review from jakepetroules May 27, 2025 15:44

plemarquand added 2 commits May 28, 2025 10:11

Proper queue instead of FIFO, properly handle next task to run being …

865551f

…in .creating state

Fixup active task count

647e955

plemarquand requested a review from FranzBusch May 29, 2025 17:55

jakepetroules reviewed May 29, 2025

View reviewed changes

Ensure immediately cancelled tasks are removed

1859b03

plemarquand mentioned this pull request May 30, 2025

Reduce usage of callback and dispatch methods in favor of async/await #8744

Open

plemarquand requested a review from jakepetroules May 30, 2025 15:06

FranzBusch reviewed Jun 1, 2025

View reviewed changes

plemarquand added 2 commits June 1, 2025 15:35

Single lock in waitIfNeeded setup

aa16f9e

Remove counter state, store all tasks in waitingTasks

d821730

Cleanup comment

aac0a6a

FranzBusch approved these changes Jun 3, 2025

View reviewed changes

Sources/Basics/Concurrency/ConcurrencyHelpers.swift Outdated Show resolved Hide resolved

Remove Sendable requirement on operation arg

03cc553

plemarquand enabled auto-merge (squash) June 3, 2025 19:14

jakepetroules approved these changes Jun 9, 2025

View reviewed changes

plemarquand merged commit 5489d55 into swiftlang:main Jun 9, 2025
6 checks passed

plemarquand deleted the async-repository-open branch June 9, 2025 16:12

Async git repository opening #8721

Async git repository opening #8721

Uh oh!

Conversation

plemarquand commented May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plemarquand May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plemarquand commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plemarquand commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FranzBusch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

plemarquand commented Jun 3, 2025

Uh oh!

Uh oh!

Uh oh!

plemarquand commented May 24, 2025 •

edited

Loading

plemarquand May 26, 2025 •

edited

Loading

plemarquand commented Jun 2, 2025 •

edited

Loading