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")
}
}
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.