Migrate Store to Actor

[mergesort]

Currently this requires only a few code changes but adds warnings that I honestly am not familiar enough with to figure out the right solution, so I would be grateful for help from anyone who understands the Actor model well.

[levi]

I just found a ton of race conditions in my app because I implicitly assumed the items access in the store was thread safe. Moving to an actor would be ideal to remove all these issues in one go.

It's a fairly straight forward process, since all the operations are already marked as async. However, we will lose main thread access to the $items publisher in its current configuration. @mergesort it's been a few years since you opened this issue, and perhaps you're more familiar with the Actor api. Curious if you had any insight in what you'd like to do now?

[mergesort]

Hey @levi, sorry for the delayed reply — I'm just digging myself out of a lot of work (though heading right into my wedding so I may be busy for the next week or so as well). 😅

If it isn't too much trouble I would absolutely love to have some more examples of the race conditions you ran into, my assumption was that Boutique generally is thread safe. It would also be helpful to know if you used the SQLiteStorageEngine or the DiskStorageEngine.

More generally I plan to take a deep dive into Boutique this summer, starting with a concurrency upgrade to hopefully quell issues like this. I have from time to time ran Boutique through Swift's strict concurrency checking and only found minor warnings, and so I generally decided to wait on changes until Swift 6. There's a chance that the diagnostics didn't trigger for whatever reason and I suspect that once Swift 6 is official a lot more issues will come up given how difficult a subject concurrency is, so I thought it best to wait given how fast the concurrency tooling has been changing over the last few months.

I also consider a migration to Swift 6 to be a good time to make breaking changes, for example if $items has to change. I am hoping to migrate Boutique to Observable so that may not be a huge deal, though there are a lot of caveats behind that statement given how many different types there are in Boutique that will likely have to be converted to macros.

I would also like to say that I am super open to suggestions, I consider this a collaborative process, so if you have any ideas for how to make this work I am all ears! Sorry again for the race conditions you ran into, I hope it wasn't too painful of an issue.

[levi]

@mergesort hi sorry just getting back into this now.

The issue I saw is the performInsert method in the store snapshots the items via currentItems, mutates that set via the ordered dictionary and then sets it back on the items via the main queue. Unless I put my calls to insert into the store on a serial queue, having concurrent writes to the store will lead to issues where the last call wins, which is not always the most up to date version of the set of items.

[levi]

@mergesort I migrated to something like this in my own projects:

import Bodega
import OrderedCollections
import Foundation

actor Store<Item: Codable & Sendable> {
    private let storageEngine: StorageEngine
    private let cacheIdentifier: KeyPath<Item, String>

    // Replace @Published with a regular property
    private var items: [Item] = []

    // Add a method to get items, since we can't use @Published
    func getItems() -> [Item] {
        items
    }

    nonisolated init(storage: StorageEngine, cacheIdentifier: KeyPath<Item, String>) {
        self.storageEngine = storage
        self.cacheIdentifier = cacheIdentifier

        // Begin loading items in the background.
        Task { await self.loadItems() }
    }

    init(storage: StorageEngine, cacheIdentifier: KeyPath<Item, String>) async throws {
        self.storageEngine = storage
        self.cacheIdentifier = cacheIdentifier
        try await self.loadItems()
    }

    private func loadItems() async throws {
        do {
            self.items = try await self.storageEngine.readAllData()
                .map({ try JSONCoders.decoder.decode(Item.self, from: $0) })
        } catch {
            self.items = []
            throw error
        }
    }

    func insert(_ item: Item) async throws {
        try await self.performInsert(item)
    }

    func insert(_ items: [Item]) async throws {
        try await self.performInsert(items)
    }

    func remove(_ item: Item) async throws {
        try await self.performRemove(item)
    }

    func remove(_ items: [Item]) async throws {
        try await self.performRemove(items)
    }

    func removeAll() async throws {
        try await self.performRemoveAll()
    }

    // Internal functions (performInsert, performRemove, etc.) remain largely the same,
    // but remove MainActor.run calls as they're no longer necessary in an actor

    private func performInsert(_ item: Item, firstRemovingExistingItems existingItemsStrategy: ItemRemovalStrategy<Item>? = nil) async throws {
        var currentItems = self.items

        if let strategy = existingItemsStrategy {
            var removedItems = [item]
            try await self.removeItemsFromStorageEngine(&removedItems, withStrategy: strategy)
            self.removeItemsFromMemory(&currentItems, withStrategy: strategy, identifier: cacheIdentifier)
        }

        let identifier = item[keyPath: self.cacheIdentifier]
        let currentItemsKeys = currentItems.map({ $0[keyPath: self.cacheIdentifier] })
        var currentValuesDictionary = OrderedDictionary<String, Item>(uniqueKeys: currentItemsKeys, values: currentItems)
        currentValuesDictionary[identifier] = item

        try await self.persistItem(item)

        self.items = Array(currentValuesDictionary.values)
    }

    // Other methods (performInsert for multiple items, performRemove, performRemoveAll) 
    // would be updated similarly, removing MainActor.run and directly modifying self.items

    // Helper methods like persistItem, removePersistedItem, etc. remain largely unchanged
}

import SwiftUI

@Observable
class ObservableStore<Item: Codable & Sendable> {
    private let store: Store<Item>
    
    var items: [Item] = []
    
    init(store: Store<Item>) {
        self.store = store
        
        // Initial load of items
        Task {
            await self.refreshItems()
        }    
    }
    
    func refreshItems() async {
        self.items = await store.getItems()
    }
    
    func insert(_ item: Item) async throws {
        try await store.insert(item)
        await refreshItems()
    }
    
    func insert(_ items: [Item]) async throws {
        try await store.insert(items)
        await refreshItems()
    }
    
    func remove(_ item: Item) async throws {
        try await store.remove(item)
        await refreshItems()
    }
    
    func remove(_ items: [Item]) async throws {
        try await store.remove(items)
        await refreshItems()
    }
    
    func removeAll() async throws {
        try await store.removeAll()
        await refreshItems()
    }
}

The downside of this current implementation is that you can never access the Store actor directly, you must always go through the @observable class to keep it in sync. Def a simple GPT written solution, but solves most of my needs at the moment.