Automattic / pocket-casts-ios

Pocket Casts iOS app 🎧
Mozilla Public License 2.0
1.65k stars 131 forks source link

Replace DispatchQueue.Sync with Actors #1306

Open emilylaguna opened 10 months ago

emilylaguna commented 10 months ago

There are a number of places in the app that uses DispatchQueue.sync to prevent multi threaded race conditions that seem like good candidates to switch to Actors.

Example: https://github.com/Automattic/pocket-casts-ios/blob/402e8281c6668ef0cd679701057d5e21fb2b612e/podcasts/DownloadProgressManager.swift#L16

esnssr commented 9 months ago

Switching to Actors could be a good idea but it will most likely affect a lot of places in the app, especially if the places where we will switch to actors in use a lot of objects (e.g: Data Models), then you would have to make sure that all of your objects used in the Actor confirm to the Sendable protocol. Now this is optional of course (for now), but given the goal of using actors and to ensure thread safety, it's a good idea to use actors with Sendable.

another thing to keep in mind is how using actors could affect other objects in the app that talk with it. using an actor means that you will have to access all of your properties inside the actor in a concurrency context, and if that is happening inside a method, you will either mark it as async (which will affect callers who use this method as they will now need to switch to a concurrent context too), or use a Task. this could come in with some performance issues, as tasks are not thread safe and every suspension point in the task means a new thread.

not to mention that actors are generally not so friendly with delegates, unless the delegates are async, which in this case will also affect callers.

if the app codebase doesn't support the new concurrency system in any place yet i wouldn't recommend to start here with actors. because of the nature of actors and their thread safety, if you don't really confirm and adapt it all through with Sendable and be careful of how to use it, you could end up introducing new threading issues and race conditions.

emilylaguna commented 9 months ago

Thank you @esnssr for the detailed write up! We will definitely consider all of these before we commit to making any changes.