Change PackageCollectionSigning API from callback-based to async/await #6609
Conversation
Motivation: Package collection signing crashes with code 139 on Linux (Ubuntu Jammy). Adding a signed collection also fails. Modifications: - Change `PackageCollectionSigning` API from callback-based to async/await - Certificate validation is async/await instead of callback-based - Change `PackageCollectionSigning` to actor since accessing `self.observabilityScope` also causes crash
yim-lee
requested review from
abertelrud,
neonichu,
tomerd and
MaxDesiatov
as code owners
|
@swift-ci please smoke test |
|
Tested |
| } else if !Self.isSignatureCheckSupported { | ||
| callback(.failure(StringError("Unsupported platform"))) | ||
| } else { | ||
| // Check the signature | ||
| let signatureResults = ThreadSafeArrayStore<Result<Void, Error>>() | ||
| certPolicyKeys.forEach { certPolicyKey in | ||
| self.signatureValidator.validate(signedCollection: signedCollection, certPolicyKey: certPolicyKey) { result in | ||
| let count = signatureResults.append(result) | ||
| Task { |
There was a problem hiding this comment.
👀 the key change in this file. The rest is mostly formatting.
There was a problem hiding this comment.
is the change to run this in a task context, or something more?
There was a problem hiding this comment.
To run async code in a Task yes. It is inside a loop and each task updates the signatureResults array.
There was a problem hiding this comment.
could we use a task group maybe to help collect the results and make waiting for all the tasks to complete easier? just an idea, not sure if helpful
There was a problem hiding this comment.
+1 for a task group or async let if either can be used here, otherwise we end up not utilizing structured concurrency benefits
|
@swift-ci please smoke test |
|
@swift-ci please test Windows platform |
| guard SignatureError.invalidSignature == error as? SignatureError else { | ||
| return XCTFail("Expected SignatureError.invalidSignature") | ||
| } | ||
| private func readTestCertData(path: (AbsolutePath) -> AbsolutePath, callback: (Result<Data, Error>) -> Void) { |
There was a problem hiding this comment.
Can this be made async? The tests calling it seem to be async already.
|
@swift-ci please smoke test |
|
@swift-ci please test Windows platform |
Motivation:
Package collection signing crashes with code 139 on Linux (Ubuntu Jammy). Adding a signed collection also fails.
Modifications:
PackageCollectionSigningAPI from callback-based to async/awaitPackageCollectionSigningto actor since accessingself.observabilityScopealso causes crash