swiftthread-safetyactor

Is my actor based store really thread safe?


I'm trying to understand if my code is really thread safe. The read/write to storeData should be, but is the call to sync in didSet also thread safe?

Also, is there a better practice to load initial data in actors? Feels weird to initialize Task in the constructor.

actor Store<Element: Codable>: StoreType {
    private let fileName: String
    private var storeData: [Element]? {
        didSet {
            sync()
        }
    }
    
    init(fileName: String) {
        self.fileName = fileName
        
        Task {
            await loadFromPlist()
        }
    }
    
    func read() async throws -> [Element]? {
        storeData
    }
    
    func write(_ value: [Element]) async throws {
        storeData = value
    }
    
    func remove() async throws {
        storeData = nil
    }
}

private extension Store {
    func loadFromPlist() {
        let decoder = PropertyListDecoder()
        
        guard let data = try? Data.init(contentsOf: plistURL),
              let elements = try? decoder.decode([Element].self, from: data) else {
            return
        }
        
        storeData = elements
    }
    
    private func sync() {
        let encoder = PropertyListEncoder()
        guard let data = try? encoder.encode(storeData) else {
            return
        }
        
        if FileManager.default.fileExists(atPath: plistURL.path) {
            try? data.write(to: plistURL)
        } else {
            FileManager.default.createFile(atPath: plistURL.path, contents: data, attributes: nil)
        }
    }
    
    var plistURL: URL {
        let documents = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask).first!
        return documents.appendingPathComponent("\(fileName).plist")
    }
}

Solution

  • This code is "thread-safe" in that at every point, storeData is a valid Swift value. You will never crash due to a low-level data race. Reading storeData will always return an array with "something" in it. And this code will never corrupt the plist file.

    This code is not "thread-safe" in that there is definitely a surprising situation during init.

    let store = Store<String>(fileName: "x")
    let oldValues = try await store.read()
    // What is oldValues? Might be empty. Might be the contents of the file.
    

    There is no promise that your init Task will be run before or after read. So you don't know what will be there.

    Since reading the file is inherently an async event that may fail, I'd recommend marking your init with async and throws. Then you don't need a Task, and everything makes sense. And there's no reason to make read() async or throws.

    Alternately, you could delay loading the file until read(), which is currently async and throws. I'd use the Optional to keep track of whether the data is loaded, rather than ever returning an Optional here.