update(forKey: ) is not thread safe

[ollitapa]

Seems that the update method is not thread safe, if using the storage on a concurrent DispatchQueue. I'm not sure if it is good to have mutable models stored anyway, but I happened to run into this ¯\_(ツ)_/¯

Workaround is to do the updates in own serial queue. Could not fix this on my own class conforming to StorageProtocol since the update-method is a protocol extension and using asStorage() always directs the linker to that implementation.

Small test to illustrate the issue:

import Dispatch
import Shallows

let storage = MemoryStorage<String, [Int]>().asStorage().defaulting(to: [])
let syncStorage = storage.makeSyncStorage()
let queue = DispatchQueue(label: "1", attributes: [.concurrent])

try? syncStorage.set([0], forKey: "Test")

queue.async {
    storage.update(forKey: "Test", { (value) in
        usleep(500)
        value.append(1)
    })
}
queue.async {
    storage.update(forKey: "Test", { (value) in
        value.append(2)
    })
}

print("Wait...")
usleep(50000)

storage.retrieve(forKey: "Test", completion: { (value) in
    print(value) // Prints success([0, 1]), but should print success([0, 1, 2])
})

[dreymonde]

This is a very interesting example. I’m away right now, but I’ll discuss this at length soon.

[dreymonde]

Thanks for waiting! So, I acknowledge the issue, but I don’t think it should be solved inside the library — Shallows was not designed to be thread-safe out of the box. To solve this, the whole library should be rethought, which, although considered, is out of the scope of this issue.

Anyway, thanks for bringing this up! Highly appreciate it.