PM-10836: Make PolicyService thread safe #825
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #825 +/- ##
=======================================
Coverage 88.48% 88.48%
=======================================
Files 600 600
Lines 30268 30268
=======================================
Hits 26782 26782
Misses 3486 3486 ☔ View full report in Codecov by Sentry. |
|
No New Or Fixed Issues Found |
| @@ -51,7 +51,7 @@ protocol PolicyService: AnyObject { | |||
| /// A default implementation of a `PolicyService` which manages syncing and updates to the user's | |||
| /// policies. | |||
| /// | |||
| class DefaultPolicyService: 10000 PolicyService { | |||
| actor DefaultPolicyService: PolicyService { | |||
There was a problem hiding this comment.
🎨 Could a test be added accessing the dictionary from different threads concurrently proving that is now thread safe?
There was a problem hiding this comment.
That's a great idea! Do you know if there's already an example of that somewhere in the app?
There was a problem hiding this comment.
Oh sorry, I missed this earlier. See the tests now I think they're correct but were you able to confirm that they fail if you leave this as a class instead of an actor?
There was a problem hiding this comment.
Yes, mostly. It was always a sporadic issue, so if I set this as a class instead of an actor, I have to run the test repeatedly before it fails
There was a problem hiding this comment.
❓ Are we going to have to change more classes into actors for managing thread safety with the Swift 6 concurrency model? Is a bunch of actors an ideal solution?
There was a problem hiding this comment.
That's probably a good idea, unless anyone knows of any drawbacks to using actors? I'm not entirely sure where to look in the app for which classes would need to be changed, though. Should we just go through and update every service?
There was a problem hiding this comment.
My understanding is that actors will execute on non-main threads (aside from @MainActor-marked things) which could potentially produce unexpected behavior? There also seems to be performance implications by having too many actors calling each other, but I'm not sure we're doing enough calls to hit that in a noticeable manner.
It might make more sense to mark the particular properties/methods as isolated rather than making the whole object into an actor? But there's a lot about the Swift 6 concurrency model that I still don't fully grok, so I'm not completely sure of the implications one way or another.
There was a problem hiding this comment.
Services should only need to be actors if they are managing shared state (like the policies dictionary here). Most services don't do this though; they mostly interact with other services rather than managing state of their own. StateService is already an actor and the database is accessed on a background context.
@KatherineInCode Those are good points; it's hard to say if we would run into performance implications without profiling, but my gut feeling as well is that we don't do enough of actor hopping that it would make a noticeable difference. I'm still figuring out isolation as well, but I guess I'm not following how that would help us here. Actor is going to isolate all methods/properties here; but most(all?) methods here end up relying on the policies dictionary so I'm not sure if we could make any methods be non-isolated.
There was a problem hiding this comment.
That all makes sense, and I think we're good with just making this an actor for now. It's something for us to keep in mind over time, though, especially if we continue to run into these sorts of thread errors.
👍🏻
6b863b7 to
1c75fa9
Compare
🎟️ Tracking
PM-10836
📔 Objective
Fix a crash that was reported through crashlytics. No reproduction steps that I know of.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes