Async git repository opening #8721
Conversation
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`.
plemarquand
requested review from
jakepetroules,
dschaefer2,
shawnhyam,
bripeticca,
owenv,
bkhouri,
cmcgee1024 and
daveyc123
as code owners
|
|
||
| private func waitIfNeeded() async { | ||
| if activeTasks >= concurrentTasks { | ||
| await withCheckedContinuation { continuation in |
There was a problem hiding this comment.
We should add a withTaskCancellationHandler here and potentially a withTaskPriorityEscalationHandler
There was a problem hiding this comment.
@FranzBusch would it be sufficient to propagate cancellation by wrapping the waitingTasks.append in a Task.isCancelled check?
if !Task.isCancelled {
waitingTasks.append(continuation)
}
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Is there a reason why we can't bump to macOS 15.0?
There was a problem hiding this comment.
Yes. We cannot require macOS 15 at this time.
There was a problem hiding this comment.
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.
| 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()) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good catch, I've fixed this up in two places
| 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()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
@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.isCancelledreturnsfalse - 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.isCancelledruns 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
}
There was a problem hiding this comment.
This makes sense, thanks. I've adopted a tri-state and made sure that the continuation always resumes with a cancellation error when appropriate.
|
@swift-ci please test windows |
|
@FranzBusch @jakepetroules I've addressed comments and added some tests for |
|
|
||
| deinit { | ||
| waitingTasksLock.withLock { | ||
| if !waitingTasks.filter({ $0.continuation != nil }).isEmpty { |
There was a problem hiding this comment.
Isn't it unexpected if there is anything at all in waitingTasks, not just those with a non-nil continuation?
There was a problem hiding this comment.
Yep, you're right, I've updated to revert this check.
|
@swift-ci please test |
|
@swift-ci please test windows |
1 similar comment
|
@swift-ci please test windows |
|
@swift-ci please test Linux |
The
RepositoryProviderprotocol'sopenmethod is defined as synchronous. Convert this toasyncto avoid concurrency workarounds in clients that might be prone to deadlocks.This involved porting
RepositoryManager.lookupto structured concurrency. The method has some concurrency constraints that are preserved:maxConcurrentOperations) on how many simultaneous lookups can be performed concurrently.RepositorySpecifier, the in flight request completes before the new one is started.