From 4fbd38505fe1b0779856ab3b9adb00538581a0c9 Mon Sep 17 00:00:00 2001 From: Andreas Bauer Date: Thu, 29 Aug 2024 15:06:50 +0200 Subject: [PATCH 1/2] Ensure access to PDFDocument is always concurrency safe --- .../SpeziOnboarding/ConsentConstraint.swift | 2 +- .../ConsentView/ConsentDocumentExport.swift | 40 ++++++++++++------- .../OnboardingConstraint.swift | 2 +- .../OnboardingDataSource.swift | 24 +++++++++-- Tests/UITests/TestApp/ExampleStandard.swift | 40 ++++++++++--------- 5 files changed, 70 insertions(+), 38 deletions(-) diff --git a/Sources/SpeziOnboarding/ConsentConstraint.swift b/Sources/SpeziOnboarding/ConsentConstraint.swift index 5fc7b9f..b2f9b3b 100644 --- a/Sources/SpeziOnboarding/ConsentConstraint.swift +++ b/Sources/SpeziOnboarding/ConsentConstraint.swift @@ -17,5 +17,5 @@ public protocol ConsentConstraint: Standard { /// /// - Parameters: /// - consent: The exported consent form represented as `ConsentDocumentExport` that should be added. - func store(consent: ConsentDocumentExport) async throws + func store(consent: consuming sending ConsentDocumentExport) async throws } diff --git a/Sources/SpeziOnboarding/ConsentView/ConsentDocumentExport.swift b/Sources/SpeziOnboarding/ConsentView/ConsentDocumentExport.swift index 18a9545..618f7b1 100644 --- a/Sources/SpeziOnboarding/ConsentView/ConsentDocumentExport.swift +++ b/Sources/SpeziOnboarding/ConsentView/ConsentDocumentExport.swift @@ -8,40 +8,50 @@ import PDFKit + /// A type representing an exported `ConsentDocument`. It holds the exported `PDFDocument` and the corresponding document identifier String. -public actor ConsentDocumentExport { +public struct ConsentDocumentExport: ~Copyable { /// Provides default values for fields related to the `ConsentDocumentExport`. public enum Defaults { /// Default value for a document identifier. + /// /// This identifier will be used as default value if no identifier is provided. public static let documentIdentifier = "ConsentDocument" } - private var cachedPDF: PDFDocument - + private let cachedPDF: PDFDocument + /// An unique identifier for the exported `ConsentDocument`. + /// /// Corresponds to the identifier which was passed when creating the `ConsentDocument` using an `OnboardingConsentView`. public let documentIdentifier: String - /// The `PDFDocument` exported from a `ConsentDocument`. - /// This property is asynchronous and accesing it potentially triggers the export of the PDF from the underlying `ConsentDocument`, - /// if the `ConsentDocument` has not been previously exported or the `PDFDocument` was not cached. - /// For now, we always require a PDF to be cached to create a ConsentDocumentExport. In the future, we might change this to lazy-PDF loading. - public var pdf: PDFDocument { - get async { - cachedPDF - } - } - + /// Creates a `ConsentDocumentExport`, which holds an exported PDF and the corresponding document identifier string. /// - Parameters: - /// - documentIdentfier: A unique String identifying the exported `ConsentDocument`. + /// - documentIdentifier: A unique String identifying the exported `ConsentDocument`. /// - cachedPDF: A `PDFDocument` exported from a `ConsentDocument`. init( documentIdentifier: String, - cachedPDF: PDFDocument + cachedPDF: sending PDFDocument ) { self.documentIdentifier = documentIdentifier self.cachedPDF = cachedPDF } + + /// Consume the exported `PDFDocument` from a `ConsentDocument`. + /// + /// This method consumes the `ConsentDocumentExport` by retrieving the exported `PDFDocument`. + /// + /// This property is asynchronous and accessing it potentially triggers the export of the PDF from the underlying `ConsentDocument`, + /// if the `ConsentDocument` has not been previously exported or the `PDFDocument` was not cached. + /// + /// - Note: For now, we always require a PDF to be cached to create a ConsentDocumentExport. In the future, we might change this to lazy-PDF loading. + public consuming func consumePDF() async -> sending PDFDocument { + // Something the compiler doesn't realize here is that we can send the `PDFDocument` because it is located in a non-Sendable, non-Copyable + // type and accessing it will consume the enclosing type. Therefore, the PDFDocument instance can only be accessed once + // and that is fully checked at compile time by the compiler :rocket: + nonisolated(unsafe) let cachedPDF = cachedPDF + return cachedPDF + } } diff --git a/Sources/SpeziOnboarding/OnboardingConstraint.swift b/Sources/SpeziOnboarding/OnboardingConstraint.swift index 3d14dec..346634c 100644 --- a/Sources/SpeziOnboarding/OnboardingConstraint.swift +++ b/Sources/SpeziOnboarding/OnboardingConstraint.swift @@ -33,5 +33,5 @@ public protocol OnboardingConstraint: Standard { Please use `ConsentConstraint.store(consent: PDFDocument, identifier: String)` instead. """ ) - func store(consent: PDFDocument) async + func store(consent: sending PDFDocument) async } diff --git a/Sources/SpeziOnboarding/OnboardingDataSource.swift b/Sources/SpeziOnboarding/OnboardingDataSource.swift index 55fa7ee..eb1cc95 100644 --- a/Sources/SpeziOnboarding/OnboardingDataSource.swift +++ b/Sources/SpeziOnboarding/OnboardingDataSource.swift @@ -11,6 +11,11 @@ import Spezi import SwiftUI +private protocol DeprecationSuppression { + func storeInLegacyConstraint(for standard: any Standard, _ consent: sending PDFDocument) async +} + + /// Configuration for the Spezi Onboarding module. /// /// Make sure that your standard in your Spezi Application conforms to the ``OnboardingConstraint`` @@ -33,13 +38,14 @@ import SwiftUI /// } /// } /// ``` -public class OnboardingDataSource: Module, EnvironmentAccessible { +public final class OnboardingDataSource: Module, EnvironmentAccessible, @unchecked Sendable { @StandardActor var standard: any Standard public init() { } + @available(*, deprecated, message: "Propagate deprecation warning") public func configure() { guard standard is any OnboardingConstraint || standard is any ConsentConstraint else { fatalError("A \(type(of: standard).self) must conform to `ConsentConstraint` to process signed consent documents.") @@ -49,11 +55,23 @@ public class OnboardingDataSource: Module, EnvironmentAccessible { /// Adds a new exported consent form represented as `PDFDocument` to the ``OnboardingDataSource``. /// /// - Parameter consent: The exported consent form represented as `ConsentDocumentExport` that should be added. - public func store(_ consent: PDFDocument, identifier: String = ConsentDocumentExport.Defaults.documentIdentifier) async throws { + public func store(_ consent: sending PDFDocument, identifier: String = ConsentDocumentExport.Defaults.documentIdentifier) async throws { if let consentConstraint = standard as? any ConsentConstraint { let consentDocumentExport = ConsentDocumentExport(documentIdentifier: identifier, cachedPDF: consent) try await consentConstraint.store(consent: consentDocumentExport) - } else if let onboardingConstraint = standard as? any OnboardingConstraint { + } else { + // By down-casting to the protocol we avoid "seeing" the deprecation warning, allowing us to hide it from the compiler. + // We need to call the deprecated symbols for backwards-compatibility. + await (self as DeprecationSuppression).storeInLegacyConstraint(for: standard, consent) + } + } +} + + +extension OnboardingDataSource: DeprecationSuppression { + @available(*, deprecated, message: "Suppress deprecation warning.") + func storeInLegacyConstraint(for standard: any Standard, _ consent: sending PDFDocument) async { + if let onboardingConstraint = standard as? any OnboardingConstraint { await onboardingConstraint.store(consent: consent) } else { fatalError("A \(type(of: standard).self) must conform to `ConsentConstraint` to process signed consent documents.") diff --git a/Tests/UITests/TestApp/ExampleStandard.swift b/Tests/UITests/TestApp/ExampleStandard.swift index c3fb232..abe500b 100644 --- a/Tests/UITests/TestApp/ExampleStandard.swift +++ b/Tests/UITests/TestApp/ExampleStandard.swift @@ -21,25 +21,28 @@ actor ExampleStandard: Standard, EnvironmentAccessible { extension ExampleStandard: ConsentConstraint { // Example of an async function using MainActor and Task - func store(consent: ConsentDocumentExport) async throws { + func store(consent: consuming sending ConsentDocumentExport) async throws { // Extract data outside of the MainActor.run block - let documentIdentifier = await consent.documentIdentifier - let pdf = await consent.pdf - + let documentIdentifier = consent.documentIdentifier + let pdf = await consent.consumePDF() + // Perform operations on the main actor - try await MainActor.run { - if documentIdentifier == DocumentIdentifiers.first { - self.firstConsentData = pdf - } else if documentIdentifier == DocumentIdentifiers.second { - self.secondConsentData = pdf - } else { - throw ConsentStoreError.invalidIdentifier("Invalid Identifier \(documentIdentifier)") - } - } - + try await self.store(document: pdf, for: documentIdentifier) + try? await Task.sleep(for: .seconds(0.5)) } - + + @MainActor + func store(document pdf: sending PDFDocument, for documentIdentifier: String) throws { + if documentIdentifier == DocumentIdentifiers.first { + self.firstConsentData = pdf + } else if documentIdentifier == DocumentIdentifiers.second { + self.secondConsentData = pdf + } else { + throw ConsentStoreError.invalidIdentifier("Invalid Identifier \(documentIdentifier)") + } + } + func resetDocument(identifier: String) async throws { await MainActor.run { if identifier == DocumentIdentifiers.first { @@ -49,12 +52,13 @@ extension ExampleStandard: ConsentConstraint { } } } - + + @MainActor func loadConsentDocument(identifier: String) async throws -> PDFDocument? { if identifier == DocumentIdentifiers.first { - return await self.firstConsentData + return self.firstConsentData } else if identifier == DocumentIdentifiers.second { - return await self.secondConsentData + return self.secondConsentData } // In case an invalid identifier is provided, return nil. From 4b165e3bd5ccc0cc00cab374d7bf36ba08564b5d Mon Sep 17 00:00:00 2001 From: Andreas Bauer Date: Thu, 29 Aug 2024 15:32:37 +0200 Subject: [PATCH 2/2] Minor annotations --- .../SpeziOnboarding/ConsentView/ConsentDocumentExport.swift | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Sources/SpeziOnboarding/ConsentView/ConsentDocumentExport.swift b/Sources/SpeziOnboarding/ConsentView/ConsentDocumentExport.swift index 618f7b1..6c44664 100644 --- a/Sources/SpeziOnboarding/ConsentView/ConsentDocumentExport.swift +++ b/Sources/SpeziOnboarding/ConsentView/ConsentDocumentExport.swift @@ -49,8 +49,9 @@ public struct ConsentDocumentExport: ~Copyable { /// - Note: For now, we always require a PDF to be cached to create a ConsentDocumentExport. In the future, we might change this to lazy-PDF loading. public consuming func consumePDF() async -> sending PDFDocument { // Something the compiler doesn't realize here is that we can send the `PDFDocument` because it is located in a non-Sendable, non-Copyable - // type and accessing it will consume the enclosing type. Therefore, the PDFDocument instance can only be accessed once + // type and accessing it will consume the enclosing type. Therefore, the PDFDocument instance can only be accessed once (even in async method) // and that is fully checked at compile time by the compiler :rocket: + // See similar discussion: https://forums.swift.org/t/swift-6-consume-optional-noncopyable-property-and-transfer-sending-it-out/72414/3 nonisolated(unsafe) let cachedPDF = cachedPDF return cachedPDF }