First pass at Swift cancellation #8529
Conversation
Generated by 🚫 Danger |
|
|
||
| if Task.isCancelled { | ||
| print("xxxxxxxxx") | ||
| continuation.resume(throwing: StorageError.cancelled) |
There was a problem hiding this comment.
This makes the test pass, but only works if the cancellation happens early.
| print("\(progress.completedUnitCount) of \(progress.totalUnitCount)") | ||
| if Task.isCancelled { | ||
| print("zzzzzzzzzzzzz") | ||
| storageTask.cancel() |
There was a problem hiding this comment.
I can't figure out a test to get into this block.
Similar issue as https://forums.swift.org/t/how-can-i-get-the-current-task/50677
There was a problem hiding this comment.
I guess you may wrap the storage task into a parent task (I did not actually tested it myself):
let taskHandle = async { // Create a parent task.
return try await storage.putFileAsync(...) // The task is created and started.
}
taskHandle.cancel() // The storage task is being cancelled because the parent task has been cancelled.
See "Unstructured Concurrency" here and cancel() method docs.
Another approach could be using a TaskGroup which has cancelAll method.
There was a problem hiding this comment.
Thanks, but I'm not sure I follow. The first suggestion is how the invocation of the putFileAsync API is done in the test below. Here, I'm trying to figure out the implementation.
The challenge I'm having is finding a way to cancel the Objective C task that is running from GTMSessionFetcher. And I can't figure out a way to get a handle to the Swift task to be able to check if it has been cancelled to know that the Objective C task should be cancelled.
| return metadata | ||
| } | ||
|
|
||
| let result = try await task.value |
There was a problem hiding this comment.
Is this the best way for a client to call the putFileAsync API so that they can have a handle to cancel it?
| @@ -193,6 +193,26 @@ class StorageResultTests: StorageIntegrationCommon { | |||
| waitForExpectations() | |||
| } | |||
|
|
|||
| func testSimplePutFileCancel() throws { | |||
There was a problem hiding this comment.
Add a test for cancellation with the old APIs
| continuation.resume(with: result) | ||
|
|
||
| let metadata = | ||
| try await withCheckedThrowingContinuation { (continuation: MetadataContinuation) in |
There was a problem hiding this comment.
There is withTaskCancellationHandler but I'm not sure how to combine that with withCheckedThrowingContinuation and not sure it would work with cancelling the Objective C task anyway.
https://forums.swift.org/t/how-to-cancel-a-publisher-when-using-withtaskcancellationhandler/49688/5
Coverage ReportAffected SDKsNo changes between base commit (125a6fe) and head commit (2622a5c). Test Logs
|
|
Succeeded by #10161 |
Investigate feasibility of Swift cancellation implementation as a wrapper over existing APIs.
We're going to put this PR on hold pending more customer input and additional Swift Concurrency evolution. In the meantime, cancellation continues to be available via the original API.
Thanks to @maksymmalyhin 's last commit, inspired by https://github.com/apple/swift-evolution/blob/main/proposals/0300-continuation.md#additional-examples, we believe we're on track to a solution. The race condition in the comments could be solved by exposing the FIRStorageUploadTask init API https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseStorage/Sources/FIRStorageReference.m#L227, call it before to get the task, and then pass the task into a modified putFile API. The two new public APIs could be prefixed with double underscores to indicate they're not for public consumption.
Beyond that, it is still an open question about how to implement cancellation for the auto-generated APIs from Objective C. I don't see anything about overriding or modifying the generated functions at https://github.com/apple/swift-evolution/blob/main/proposals/0297-concurrency-objc.md.