Enabled concurrency checks and constrained Store to MainActor

[StefanCosminR]

After I enabled concurrency checks, the Store had many warnings because self is not Sendable but it was passed to MainActor.run {} closures.

In SwiftUI, is generally recommended to mark ObservableObjects with @MainActor as far as I know.

After I made this change, some tests started to fail but I fixed them by making the init async. While this is a big change, I think it might make sense because when the Store is initialised, it loads the persisted state from disk, so until that happens, the Store is in an inconsistent state.

Some alternatives to @MainActor Store:

• Constrain Store to a different global actor
• Add @unchecked Sendable and hope everything is ok

Some alternatives to init async:

• Store a reference to the task that performs allPersistedItems() in init and make sure all the other methods await this task before doing their work.

Also, I changed some tests to use XCTAssertEqual because it provides a better error message.

[mergesort]

Heya @StefanCosminR! Thank you for all of this work across both repos, I really appreciate you doing this work to try and improve Boutique. I wanted to let you know that it's going to be a day or two before I look at this, there are quite a few changes here and I want to look at them on a fresh head after a big ramp up to launching this project, and all of the attention that it drew. I plan on reading over all of the PRs and incorporating the many pieces of feedback people have suggested, so please know that I've looked at this and will absolutely get to it, but need to do so with a little more energy and focus.

Thanks again, it really, really means a lot to me.

[StefanCosminR]

Hello!

I realise that these are substantial changes and you have to evaluate their impact.

I wanted to put this out there so maybe more people could take a look if it's a good direction, so it's ok by me to take as much time as you need.

One more thing, I appreciate a lot that you open sourced this, it was exactly what I was looking for these days.

[mergesort]

Heya @StefanCosmina, I got a chance to look this over today and have few concerns with integrating this patch as is, but it's possible I didn't integrate it as you'd expected and maybe some context would help.

The first thing I noticed is that now objects have to conform to Sendable, which makes sense. I worry though that I have to add additional conformances, even to such common types as Date or URL, and this may provide some onus to users that they didn't expect or don't know how to deal with. I think this is more correct in the context of the actor model, but worry that it may be a bit burdensome.

Naturally this change also makes everything async. I'm not sure that's the direction I want to go, but I'm very open to all ideas, though since @Stored(in: Store) accepts a synchronous Store, so I'm not sure how to pass an async Store into it. I've had quite a few thrown at me over the last couple of days and I'm taking the time to hear them all out, take care of some other potential performance pitfalls and synthesize a plan of how I'd like to go forward. It's been a lot for me to consider since some of the ideas were ones I'd had, but some of them are completely new ideas to me.

One question about @MainActor, I believe that annotating a class with @MainActor doesn't make everything in a class run on the main actor but only some operations like changing an @Published property. Is that right or am I mistaken? Because if I'm mistaken I worry about annotating the Store as @MainActor, since some things can be run on background actors for performance reasons.

Looking forward to your thoughts, and thank you again!

[StefanCosminR]

The first thing I noticed is that now objects have to conform to Sendable, which makes sense. I worry though that I have to add additional conformances, even to such common types as Date or URL, and this may provide some onus to users that they didn't expect or don't know how to deal with. I think this is more correct in the context of the actor model, but worry that it may be a bit burdensome.

I forgot to check on the current version of Xcode as I've been running the beta, sorry. Those types are sendable in Swift 5.7, so when it will get released, those warnings will go away. In the meantime, one way to silence them is to use @preconcurrency import Foundation

Naturally this change also makes everything async. I'm not sure that's the direction I want to go, but I'm very open to all ideas, though since @Stored(in: Store) accepts a synchronous Store, so I'm not sure how to pass an async Store into it.

I'll have to investigate this further. I have ways to keep the init sync, but I didn't explore this option much.

One question about @MainActor, I believe that annotating a class with @MainActor doesn't make everything in a class run on the main actor but only some operations like changing an @Published property. Is that right or am I mistaken? Because if I'm mistaken I worry about annotating the Store as @MainActor, since some things can be run on background actors for performance reasons.

Annotating a class with MainActor will indeed force all functions and properties to run on the main thread. However when you have an "await" call anywhere, if the function that is called (or property that is accessed) is not marked with @MainActor, then that call will be performed on a background thread, but the response will be returned back on the Main Actor. I think this thread will clear up some things, especially the replies from Doug Gregor https://twitter.com/krzyzanowskim/status/1538647537720209408

You can also run the following code in an app to see exactly how the execution hops threads.

struct ContentView: View {
    var obj = MainActorTest()

    var body: some View {
        VStack {
            Button("Run test") {
                Task.detached {
                    await obj.compute()
                }
            }
        }
    }
}

@MainActor
class MainActorTest {
    private var randomNumber: Int {
        print("randomNumber", Thread.current)
        return Int.random(in: 1...10)
    }

    func compute() async {
        print("compute-start", Thread.current) // MainThread
        try? await Task.sleep(nanoseconds: 100_000)

        let firstNumber = randomNumber
        print("compute-middle", Thread.current) // MainThread

        let secondNumber = await getSecondNumber()

        print("compute-end", Thread.current) // MainThread

        let _ = await addNumbers(a: firstNumber, b: secondNumber)
    }

    nonisolated func getSecondNumber() async -> Int {
        print("getSecondNumber-start", Thread.current) // Some background thread
        try? await Task.sleep(nanoseconds: 100_000)
        return 2
    }
}

func addNumbers(a: Int, b: Int) async -> Int {
    print("addNumbers", Thread.current) // Some background thread
    try? await Task.sleep(nanoseconds: 100_000)

    return a + b
}

[StefanCosminR]

I made some changes to the PR. The warnings should be fixed now in the current version of Xcode and I added back the sync initializer.

Now I realize that maybe some methods should not be @MainActor, particularly add(_ items:), because it will compute the diff on the main thread. I still have to work on that

[StefanCosminR]

I tried removing @MainActor from the Store, but it's more complicated than I thought.

If the store is not on the MainActor, then it can't really be made Sendable. KeyPaths are not yet Sendable for example, so trying to conform the Store to Sendable generates a lot of warnings. And if it's not Sendable, then it can't be passed safely to self.$items.sink. You could also mark the Store with @unchecked Sendable and use locks to manage access, especially that they can offer better performance than actors, but I don't trust myself with these.

Marking the Store with a custom global actor didn't work either, because items should be marked with @MainActors to work with SwiftUI, but then the initializers can't be sync anymore.

However, I found in a talk from WWDC21 that Apple recommends marking ObservableObjects with @MainActor, so I wouldn't really want to fight that. (https://developer.apple.com/videos/play/wwdc2021/10019/ at 21:47)

I think the ideal way forward would be to make the Store be very simple and to move most of the logic in another class/actor, but personally I don't want to make changes that big to the project structure.

Here's how I image it. I'm using AsyncChannel to remove the dependency from Combine, but that won't compile until Swift 5.7 is released.

However, I think that we better close this PR and don't merge it as it is now. It was fun experimenting, but I feel a better solution will present itself as Swift concurrency tools evolve over time.

[mergesort]

I agree, I'm going to close this out due to the scope of changes, and because #22 seems to have vast improved the performance issues that this patch aimed to address.

Thank you for the contribution, it sparked a lot of ideas!