mattmassicotte / ConcurrencyRecipes

Practical solutions to problems with Swift Concurrency
BSD 3-Clause "New" or "Revised" License
1.05k stars 33 forks source link

Recipes for Handling Non-Sendable Types in SwiftUI with MVVM Pattern #11

Open mylaluna opened 2 months ago

mylaluna commented 2 months ago

Here is a common MVVM pattern implementation in a swiftUI project, I try to fit swift concurrency into it:


import SwiftUI

final class MyManagerClass {
    func getData() async throws -> String {
        "Some Data!"
    }
}

actor MyManagerActor {
    func getData() async throws -> String {
        "Some Data!"
    }
}

@MainActor
final class MVVMConcurrencyViewModel: ObservableObject {
    let managerClass = MyManagerClass()
    let managerActor = MyManagerActor()

    @Published private(set) var myData: String = "Starting text"
    private var tasks: [Task<Void, Never>] = []

    func cancelTasks() {
        tasks.forEach({ $0.cancel() })
        tasks = []
    }

    func onCallToActionButtonPressed() {
        let task = Task {
            do {
                // Using non-actor class fetch method, causes a warning in concurrency checking:
                myData = try await managerClass.getData()  // Warning: Passing argument of non-sendable type 'MyManagerClass'

                // Using actor class fetch method does not produce a warning:
                // myData = try await managerActor.getData()
            } catch {
                print(error)
            }
        }
        tasks.append(task)
    }
}

struct MVVMConcurrencyPattern: View {
    @StateObject private var viewModel = MVVMConcurrencyViewModel()

    var body: some View {
        Button("Click me") {
            viewModel.onCallToActionButtonPressed()
        }
        .onDisappear() {
            viewModel.cancelTasks()
        }
    }
}

// SwiftUI Preview
#Preview {
    MVVMConcurrencyPattern()
}

In the above SwiftUI example, I am utilizing MyManagerClass to fetch data for the view model. However, this triggers a warning when the concurrency checking flag is turned on:

Warning Message: "Passing argument of non-sendable type 'MyManagerClass' outside of main actor-isolated context may introduce data races."

One potential solution is converting MyManagerClass into an actor type (MyManagerActor), or marking MyManagerClass as Sendable. However, converting all data manager classes to actor types is often impractical, and making an entire class Sendable might not be advisable.

Do you have any recommendations for addressing this issue, particularly when dealing with legacy code bases or when a complete restructuring of the class design isn’t feasible? Any specific patterns or best practices would be greatly appreciated.

mattmassicotte commented 2 months ago

I'm glad you asked!

Usually, when you start thinking "I want this class to be Sendable..." what you really want is to avoid needing it to be Sendable in the first place.

Here's your issue:

final class MyManagerClass {
    func getData() async throws -> String {
        "Some Data!"
    }
}

This is a non-isolated class that has async methods. This is almost never what you want. What I would do is just make MyManagerClass MainActor-isolated. If you find that you need to run slow, but synchronous stuff in there, you can selectively use nonisolated to get the work off the MainActor.

Don't be afraid of MainActor! I make all of my SwiftUI views MainActor too!

mylaluna commented 2 months ago

Thanks for the quick response. Let's consider MyManagerClass containing a suite of async functions such as getUserName(), updateUserName(), and getUserPosts(), etc. which are all typical backend communication tasks. Applying @MainActor to the entire class raises a couple of concerns for me:

             await MainActor.run {
                self.myData = try await managerClass.getData()
            }

In this example, only the data assignment is on the MainActor, while data fetching occurs in the background. This segregation helps keep the UI responsive.

Now, the suggestion to run not just the ViewModel but also the entire data manager class on the MainActor represents a significant shift. I'm concerned this might congest the main thread, especially with intensive backend operations involved.

Could you provide insights or alternative strategies that might help balance the need for UI responsiveness with efficient backend communication in an MVVM framework?

mattmassicotte commented 2 months ago

I want to challenge your assumption that this is a problematic pattern. In fact, I'll go even further, and say that I'd also recommend you rethink your use of MainActor.run. I've written about that. But, let's just focus on your concerns.

I'm going to re-iterate my recommenation from above:

If you find that you need to run slow, but synchronous stuff in there, you can selectively use nonisolated to get the work off the MainActor.

Take special note of the "slow, but synchronous". You need both of these things to be true to introduce unresponsiveness into your UI. So consider your:

try await managerClass.getData()

This function cannot block the main thread unless it does a synchronous networking call. I'm sure it does not. So, the only other opporintity for blocking would be set up for the call, or processing afterwards. That processing could be an issue. So you'd break it up like this:

@MainActor
final class MyManager {
  func getData() async throws -> RealValue {
    // cannot block main thread because it is async
    let rawValue = try await doActualNetworking()

    return try await transformValue(rawValue)
  }

  nonisolated func transformValue(_ rawValue: RawValue) async throws -> RealValue {
    ...
  }
}

Now you have changed the nature of the issue. Your appliction state can be isolated to the MainActor, but your value types, if you really do need to process them in the background, have to become Sendable. And even that will get better with Swift 6 + Region-Based isolation, which could remove the need to make both RealValue and RawValue Sendable.

mylaluna commented 2 months ago

Thank you for your insights. I appreciate the suggestion about using nonisolated functions for post-processing to prevent blocking the main thread. This certainly helps with the async part of doActualNetworking(). However, I'm contemplating the scenarios where getData() involves complex pre-processing steps as well.

If we apply a similar nonisolated approach to pre-processing, the structure of handling a single request might become cumbersome—especially if MyManager class manages a variety of such getData() operations. Each might then require three distinct handling stages: pre-processing, networking, and post-processing. This could lead to a bloated and hard-to-maintain class structure.

Given this, I’m leaning towards possibly making MyManager an actor. Although this class doesn’t manage mutable state that necessitates actor-based isolation per se, using an actor might streamline the handling of these operations by encapsulating them in a single concurrency-safe context. The results from getData() could then be passed to the main actor for UI updates, maintaining thread safety while potentially simplifying the flow.

Would this approach be a reasonable compromise between maintaining performance and ensuring code manageability? Or do you see a better method to handle these complexities without overly fragmenting the logic into multiple stages?

mattmassicotte commented 2 months ago

What really matters are the answers to two questions:

Actors are good, because they protect mutable state. But, they are bad, because they must be accessed asynchronously, and cannot accept or return non-Sendable types. That's a huge trade-off. I don't yet know if/why this type needs to be an actor. But maybe it does have private state!

Deciding between actor/non-isolated really comes down to these answers.

Update: sorry you addressed this! You said it does not have state. If that's true, you can mark it Sendable, make it an actor, or make it a struct. They should all be equivalent.