@Atomic property wrapper for standard library?

You might be interested in @lorentey ‘s experimental design: https://github.com/apple/swift/pull/27229

6 Likes

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.

33 Likes

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.

3 Likes

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.

2 Likes

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.

1 Like

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.

3 Likes

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.

4 Likes

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
    }
}

What's the problem? Modeling locks as classes makes sense and works well. What do you think an NSLock is?

Here's Alamofire's UnfairLock wrapper for os_unfair_lock:

/// An `os_unfair_lock` wrapper.
final class UnfairLock {
    private let unfairLock: os_unfair_lock_t

    init() {
        unfairLock = .allocate(capacity: 1)
        unfairLock.initialize(to: os_unfair_lock())
    }

    deinit {
        unfairLock.deinitialize(count: 1)
        unfairLock.deallocate()
    }

    private func lock() {
        os_unfair_lock_lock(unfairLock)
    }

    private func unlock() {
        os_unfair_lock_unlock(unfairLock)
    }

    /// Executes a closure returning a value while acquiring the lock.
    ///
    /// - Parameter closure: The closure to run.
    ///
    /// - Returns:           The value the closure generated.
    func around<T>(_ closure: () -> T) -> T {
        lock(); defer { unlock() }
        return closure()
    }

    /// Execute a closure while acquiring the lock.
    ///
    /// - Parameter closure: The closure to run.
    func around(_ closure: () -> Void) {
        lock(); defer { unlock() }
        return closure()
    }
}
2 Likes

Doesn't that have an unneeded level of indirection? Why not make an instance variable of type os_unfair_lock?

final class UnfairLock {
    func around<Answer>(_ body: () throws -> Answer) rethrows -> Answer {
        os_unfair_lock_lock(&lock)
        defer { os_unfair_lock_unlock(&lock) }
        return try body()
    }

    private var lock = os_unfair_lock()
}

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.

5 Likes

os_unfair_lock needs to be manually heap allocated because Swift does not guarantee the address stability that it depends on.

2 Likes

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"!)

Correct me if I'm wrong, but property wrapper storage is part of the instance, and classes instances are always allocated on the heap. So as long as you use it in a classes and not in structs, it should be safe to use a simple ivar and don't bother with a wrapper and manual allocation, isn't it ?

My understanding is that this is not the case because Swift reserves the right to use a local temporary rather than referencing the ivar directly.

Is that actually guaranteed anywhere?

classes are reference type, and they are either allocated on the heap, or if they are allocated on the stack (as an optimisation), they must guarantee to not change their address, as being a reference type, they must guarantee that === is always true.

So it should be enough for the case discussed here.

That’s true, but as @anandabits rightly points out, Swift reserves the right to copy stored properties to temporaries as part of some operations, so struct properties are not guaranteed to have a stable address. This is why SwiftNIO’s builtin atomics use manually heap allocated data.

1 Like