FHIRStore actor isolation
#26
Conversation
There was a problem hiding this comment.
Awesome job @philippzagar ⚡️⚡️⚡️
Fyi, I left some suggestions. But these are more or less like nice-to-have ones.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
==========================================
+ Coverage 33.50% 34.93% +1.44%
==========================================
Files 21 21
Lines 1466 1466
==========================================
+ Hits 491 512 +21
+ Misses 975 954 -21
Continue to review full report in Codecov by Sentry.
|
Sources/SpeziFHIR/FHIRStore.swift
Outdated
| public func insert(resource: FHIRResource) async { | ||
| _$observationRegistrar.willSet(self, keyPath: resource.storeKeyPath) | ||
| await storage.insert(resource: resource) | ||
| _$observationRegistrar.didSet(self, keyPath: resource.storeKeyPath) | ||
| } |
There was a problem hiding this comment.
Calling observation registrar from non-MainActor might currently crash SwiftUI when running iOS 18 and Swift 6 Language mode. We should make sure to call these on the main actor.
Did you test that doing an async mutation works with SwiftUI. I had issues in the past that would lead me to believe that SwiftUI marks the views dirty on the willSet and would therefore miss view updates.
There was a problem hiding this comment.
Interesting that this apparently(?) produces crashes in iOS 18 with Swift 6 language mode. Is that documented anywhere or is there a SO post? (except for your comment in the Swift forums)
I never experienced a crash in regards to the observation registrar in my testing, and I performed tons of bulk insertions (see UI tests).
Still, trusting your expertise (and as all of these functions are now on the MainActor already), I'll perform the mutation on the MainActor.
Yes, this behavior stayed the same here, even added an additional test case just to be sure.
Sources/SpeziFHIR/FHIRStore.swift
Outdated
| // Non-isolation required so that resource access via the `FHIRStore` stays sync (required for seamless SwiftUI access). | ||
| // Isolation is still guaranteed as the only modifying functions `insert()` and `remove()` are isolated on the `FHIRStore.Storage` actor. | ||
| nonisolated(unsafe) private var _resources: [FHIRResource] = [] |
There was a problem hiding this comment.
Synchronizing modifications to a single task but reading from other threads might still lead to race conditions as CPU caches might render outdated information (or out of order issues). The previous lock implementation would have protected against this as it makes sure that shared state is propagated to all CPU caches.
Just using a lock, however, doesn't prevent inconsistent reads. E.g., a view might read this property twice in its view body potentially receiving two different values (e.g., a isEmpty check might read the property a second time). Either, we need to make this behavior explicit (e.g., as part of documentation or through API design).
Most importantly, FHIRResource is not Sendable. So this is unsafe by definition and is all hidden by the nonisolated(unsafe). FHIRResources cannot be shared between two actors (they might be sent, however we always keep a reference through this array. So this is impossible to be provable correct by the compiler).
As it seems this can be semantically considered a view model, why not just synchronize to the MainActor? We might do initial population from the array on a different actor (if necessary?), as we can send the result to the array that is isolated on, e.g., MainActor.
There was a problem hiding this comment.
Thanks for giving me more input here, I wasn't aware of all of these caching issues with Swift isolation that you mentioned.
I marked the store as Sendable as mutations are only done via the synchronized insert / delete methods on the actor, just the read operation is done concurrently. I assumed that this makes it safe to pass around between different actors. But apparently, caching issues prevent that from being safe? Am I reading this right? Otherwise, I don't see a reason why this couldn't be Sendable.
I followed your suggestion of synchronizing most properties on the MainActor, except for the heavy lifting of ingesting new resources into the store, that's done on an arbitrary actor. That gives us a rather clean and still safe solution, which should also be performant. Lmk what you think
| guard let resource = _resources.first(where: { $0.id == resourceId }) else { | ||
| return | ||
| /// - Parameter resources: The `FHIRResource`s to be inserted. | ||
| public func insert<T: Collection>(resources: sending T) async where T.Element == FHIRResource { |
There was a problem hiding this comment.
Should we make this method just be isolated to MainActor (and sync) instead of requiring that the input must be supplied sending? Seems a bit overly restrictive?
| _$observationRegistrar.didSet(self, keyPath: \.conditions) | ||
| _$observationRegistrar.didSet(self, keyPath: \.diagnostics) | ||
| _$observationRegistrar.didSet(self, keyPath: \.encounters) | ||
| _$observationRegistrar.didSet(self, keyPath: \.immunizations) |
There was a problem hiding this comment.
Something that this PR is changing is that previously the observation pattern is much more granular. If you were just interested in a certain category, the view would just update if an element in this category was changed. Might this impact performance for our use case?
FHIRStoreactor isolation♻️ Current situation & Problem
As references in #25, the current
FHIRStoreimplementation is not ideal as it still uses preconcurrency locking mechanisms. Therefore, theFHIRStoreshould be reimplemented to use Swift's structured concurrency and isolation mechanisms.⚙️ Release Notes
FHIRStoreactor isolation📚 Documentation
Documented all isolation thoughts
✅ Testing
--
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: