Un-explained crash inside DispatchQueue.sync with Swift RefCountBits

realityworks · 2023-09-04T07:28:01.048Z

Running on iOS 16, built with Xcode 14.3

I have a crash that looks like it's deep inside a swift RefCount, it makes very little sense, and is not descriptive at all.

DispatchQueue.sync(barrier: true) should make the following code thread safe. So not sure what the crash is and how it happens.

It's only occurred twice in a month of testing.

Here is the code :

    var queue: DispatchQueue = DispatchQueue(label: "global_state", attributes: .concurrent)
    struct GlobalState: Equatable, Hashable {
        var dataStateXX ... // This is always an equatable/hashable value type
    }

    var queue: DispatchQueue = DispatchQueue(label: "global_state", attributes: .concurrent)

    fileprivate var unsafeState: GlobalState = .init()
    var state: GlobalState {
        set {
            // perform writes on data synchronously to keep the data thread safe
            let threadValue = newValue
            queue.sync(flags: .barrier) {
                self.unsafeState = threadValue
                DispatchQueue.main.async {
                    self.stateSubject
                        .send(self.unsafeState)
                }
            }
        }

        get {
            unsafeState
        }
    }

    private let stateSubject: CurrentValueSubject<GlobalState, Never>

Here is the error : #8 (null) in bool swift::RefCounts<swift::RefCountBitsT<(swift::RefCountInlinedness)1> >::doDecrementSlow<(swift::PerformDeinit)1>(swift::RefCountBitsT<(swift::RefCountInlinedness)1>, unsigned int) ()

Here is the stack :

image1510×740 180 KB

Any ideas or suggestions would be most appreciated. This one has me stumped, especially the error descriptor, I have never seen it before.

tclementdev · 2023-09-04T07:57:52.700Z

I think you forgot to protect access in the getter.

get {
    queue.sync { unsafeState }
}

Likewise you should not use unsafeState in your notify code, but rather send(self.state) or send(newValue).

But note that the whole concurrent barrier queue is a known anti-pattern (see this post by David Smith). You'd be better off simply using a lock.

realityworks · 2023-09-04T23:44:55.322Z

Thanks for the suggestions. It doesn't really explain the issue however, since you can read memory while writing to a block from different threads. But I can see that forcing it onto the same thread for the read and write would be the safest option.

In terms of optimisation, I'm not too worried, if I wanted an optimised system, I wouldn't be touching either SwiftUI or Combine. :)

realityworks · 2023-09-04T23:53:23.025Z

tclementdev:

You'd be better off simply using a lock.

I have a version using an NSLock, instead of the dispatch queue, let's see how it performs. :)

Thanks again.

wadetregaskis · 2023-09-05T00:43:05.460Z

realityworks:

It doesn't really explain the issue however, since you can read memory while writing to a block from different threads.

But what you read may be gibberish because it may be a random mix of multiple versions of the value; you might read the value when it's only partly updated during a set. Mutations are only implicitly atomic if the data structure in question is quite small (typically machine-word size, so 64-bit on most current platforms), and even then only if fully aligned (usually but not always the case for objects and values), among other prerequisites.

realityworks · 2023-09-05T00:56:11.942Z

realityworks:

Yes, that's correct. What I'm saying however, is that the exception being raised, does not describe this issue. It fails on the write.

My goal in this post is to identify what and how that exception occurred, along with understanding why RefCountBits would throw a null exception.

Yes, that's correct. What I'm saying however, is that the exception being raised, does not describe this issue. It fails on the write.

My goal in this post is to identify what and how that exception occurred, along with understanding why RefCountBits would throw a null exception.

jrose · 2023-09-05T01:00:20.847Z

Reading an owned value includes retaining it. It is not an atomic operation in Swift. After that, all bets are off where you’re going to crash. (If you run under TSan, it should tell you this, but I haven’t actually tested it.)

This is something people forget because the corresponding code in Objective-C is thread-safe…for atomic properties. But there’s nothing clever going on there; atomic ObjC properties have a lightweight lock in case there’s actually a conflict. For better or worse, Swift doesn’t provide that for you.

realityworks · 2023-09-05T01:16:37.378Z

In that case, I would expect the following approach would solve it ->

let stateLock = NSLock()

    fileprivate var unsafeState: GlobalState = .init()
    var state: GlobalState {
        set {
            // perform writes on data synchronously to keep the data thread safe
            stateLock.withLock {
                self.unsafeState = newValue
            }

            DispatchQueue.main.async {
                self.stateSubject
                    .send(newValue)
            }
        }

        get {
            let safeState: GlobalState

            stateLock.lock()
            safeState = unsafeState
            stateLock.unlock()

            return safeState
        }

tera · 2023-09-05T11:19:41.412Z

Yes, I'd do this. Getter could be further simplified to a shorter:

        get {
            stateLock.withLock {
                unsafeState
            }
        }

Two things to note about this implementation:

you'll be posting main thread updates even if the state hasn't been actually changed (assigned to the same value). A mitigation her could be making the state equatable and comparing it, although that would add processing time, especially if there are many duplicate updates.
you'd be posting multiple main thread updates when you change state frequently (e.g. in a for i in 0 ... 1000 loop), in other words there's no "don't spam main thread too often" throttling. A mitigation here could be implementing a throttling to not update main thread too often (say, not faster than 120 times per second).