Having spent the last week working with a heavily threaded project I’ve rediscovered just how pernicious problems with simultaneous mutation or properties across threads can be. The program works 99.9999% of the time but is subject to very occasional seemingly inexplicable crashes out of the blue, adding a key to a dictionary for example.
Property wrappers seem to be ideal to give developers a established solution to this problem and I’d like to propose we add something like following to the standard library:
@propertyWrapper
public struct Atomic<Value> {
let lock = NSLock()
var _stored: Value
public init(wrappedValue initialValue: Value) {
_stored = initialValue
}
@discardableResult
func synchronize<V>(block: () -> V) -> V {
lock.lock()
let value = block()
lock.unlock()
return value
}
public var wrappedValue: Value {
get {
return synchronize(block: { _stored })
}
set(newValue) {
synchronize {
_stored = newValue
}
}
}
}
Apologies if this has already ben thought of and implemented.
Uh oh. I don't think that property wrapper is atomic. Getter and setter are synchronized, but everything that happens inbetween isn't. Let's say you have an increment method on Int, and call it from two threads at the same time on @atomic var x: Int = 0
There's nothing preventing thread A from getting the value (zero), then thread B getting the value (still zero), then both incrementing it, and then both writing it (one)
The result would be one, instead of two. It would be thread safe, but not atomic.
Anyway, in general I like the idea, assuming it's possible to make a correct (99.9999% of the time is not enough), and fast implementation! Awesome way to use property wrappers
I think an atomic property wrapper is almost never what you want.
Leaving aside the difficulty of performing the synchronisation using Swift’s accessor model which generalized accessors solves, the bigger problem is that you usually want to hoist the scope of your synchronization. It’s rare that you want to synchronize one property; you usually want to synchronize multiple, or nested ones. You also often want to store that property in a local variable which now immediately loses synchronization. In general this property wrapper replaces data races with logic errors, which may not be the outcome you want.
It is in general better to design your way out of data races rather than slap a property wrapper on your properties until the crashes go away. You can find many threading issues using thread sanitizer, so these shouldn’t have to catch you by surprise.
You should make clear decisions about what thread or mutex owns what data, and then enforce those restrictions. When you don’t know what context you’re in, assume you’re in the wrong one and reacquire the owned context. Assume any callback may be invoked on arbitrary threads unless explicitly documented otherwise. All of these lead to better outcomes than synchronization decorators, in my view.
These types of wrappers will work fine, as long as they also expose APIs to wrap actions on state safely. We use a Protector class in Alamofire to do this. It allows usage like this:
protectedMutableState.write { mutableState in
mutableState.responseSerializers.append(closure)
if mutableState.state == .finished {
mutableState.state = .resumed
}
if mutableState.responseSerializerProcessingFinished {
underlyingQueue.async { self.processNextResponseSerializer() }
}
if mutableState.state.canTransitionTo(.resumed) {
underlyingQueue.async { if self.delegate?.startImmediately == true { self.resume() } }
}
}
The only caveat is that, if you're not using a recursive lock (we aren't, it uses os_unfair_lock under the hood), you have to be careful not to call other properties locked by the same lock.
Edit: Additionally, with Swift 5.1, these wrappers are even nicer paired with subscripting key path calling.
I think I support the general idea of an Atomic attribute and a Property Wrapper is currently the only real option, but I would be hesitant to start adding this kind of thing to Swift or that standard library before an actual concurrency model is adopted.
I'm not super familiar with the design space, but I believe there are approaches were a traditional Lock is not necessary and/or there may be APIs added as part of that model which accomplish the same thing at a deeper language/threading level without having to Wrap your Properties.
I agree with @lukasa, objc properties proved that atomic getter/setter almost never solve any problem.
Moreover, the proposed implementation is very inefficient. It requires a lock for each instance variable, which may result in thousand of locks allocated for a simple object graph. At least, use an os_unfair_lock instead of a NSLock.
Atomicity must be a property of the Type IMHO. Many simple types can use processor atomic instructions instead of lock, but only the compiler can know for sure what the processor supports, and what type can fit.
I guess this gets filed under the category of “Seemed like a good idea at the time”. Did Objective-C atomic really achieve so little? Getting something into stdlib is still on my bucket list though but I’d settle for Dispatch. Apologies for the noise. Making code thread safe is hard.
I think I can speak for others when I say no need to apologize! Thanks for the discussion. I think everyone who has participated or read along has enjoyed it.
Notwithstanding @lukasa’s sage advice, in particular the following which left quite an impression:
I have continued to look at and @Atomic property wrapper as an exercise and it solves a problem in the codebase I have. There's a reason I used NSLock() in the example instead of trying to use the more modern os_unfair_lock in that the latter results in a tasty mutability conundrum where the synchronized method needs to be non-mutating as it is used in the getter of the property wrapper but is mutating the lock. The best I’ve been able to come up with is the following which smells to high heaven:
private struct OSLock {
var _lock = os_unfair_lock()
let _lockPtr: UnsafeMutablePointer<os_unfair_lock>
init() {
_lockPtr = UnsafeMutablePointer(&_lock)
}
func synchronize<V>(block: () -> V) -> V {
os_unfair_lock_lock(_lockPtr)
let value = block()
os_unfair_lock_unlock(_lockPtr)
return value
}
}
extension NSLock {
func synchronize<V>(block: () -> V) -> V {
lock()
defer { unlock() }
return block()
}
}
@propertyWrapper
public struct Atomic<Value> {
var _stored: Value
private var lock = OSLock()
public init(wrappedValue initialValue: Value) {
_stored = initialValue
}
public var wrappedValue: Value {
get {
return lock.synchronize(block: { _stored })
}
set(newValue) {
lock.synchronize {
_stored = newValue
}
}
}
}
It would be less bad if ‘_lockPtr’ was a local variable, refreshed each time the lock was used rather than an dodgy UnsafeMutablePointer instance variable. I’ve tried the following:
func synchronize<V>(block: () -> V) -> V {
let lockPtr = UnsafeMutablePointer(mutating: &_lock)
// gives: Cannot pass immutable value as inout argument: 'self' is immutable
// etc
If you ask me, this cheat should work as UnsafeMutablePointer(mutating: &_lock) is taking taking a UnsafePointer as an argument rather a UnsafeMutablePointer. Alas, all pointer arguments are processed as inout arguments these days and don’t make the distinction.
Can anybody think of a way out of this maze of mirrors, short of making OSLock a class?
private class OSLock {
var _lock = pthread_mutex_t()
func synchronize<V>(block: () -> V) -> V {
pthread_mutex_lock(&_lock)
let value = block()
pthread_mutex_unlock(&_lock)
return value
}
}
I was told by @Philippe_Hausler at WWDC that that method may have a 0.1% - 1% crash rate due to Swift's automatic management of the os_unfair_lock. Manually allocating was the suggested fix.
Can somebody explain what the failure mode would be? What is the automatic management of os_unfair_lock and what about it would cause the failure described here? @Philippe_Hausler? Is this an alignment issue? A caching issue?
This is a two fold issue; 1) the compiler is completely at liberty to write something back to the taking of the address of a struct (say to write tombstone values) and 2) the allocation region may not be heap, in the regards types can be allocated on the stack if the compiler has knowledge of that type and the lifespan of the usage. So as it stands the only safe way (and by safe I mean way without a crash rate) is to allocate a region via something like os_unfair_lock_t.allocate(1) and use the resultant of that to pass into os_unfair_lock_lock.
Thanks everyone. This information was incredibly useful. Suddenly everything is working reliably! I still think you should be able to take a UnsafePointer without having to make your your method mutating but that’s a battle for another day… It’s a shame you can’t use property wrappers at the top level (the error says “yet"!)