Are enums with stored properties not thread safe even when mutated using a serial queue?

This issue relates to another issue reported on the Apple Developer forum titled How to avoid Data Races in deinit 4 years ago. I am seeing crashes in production related to associated data and outlined consume code added by the compiler to deinitialize enums with associated data that contain reference types.

When trying to find if someone else has faced the same issue, I stumbled across the above post. I reduced the example from that post to a simpler example below and reliably get a crash due to bad access. (I ran this in Playground and got the crash but no stack trace)

import Foundation
class Racer {
    let queue = DispatchQueue(label: "Racer-\(UUID())")
    var value: String? = "" // Desugars to Optional<String>, an enum
    func race() {
        queue.async { // Crashes with or without using [weak self] 
            self?.value = UUID().uuidString
        }
    }
}
let racer = Racer()
while true {
    racer.race()
}

error: Execution was interrupted, reason: EXC_BAD_ACCESS (code=1, address=0x357d20cc94c0).

1 Like

No mutable values are thread-safe in Swift.

1 Like

I know that. But if you see in the example code, we are guarding access to it using a serial queue

1 Like

I suspect (but haven't tried to prove) that the race is happening due to the logging code that Playgrounds inserts. I can't get this to crash when built as a plain file, with or without Thread Sanitizer enabled. If it is Playgrounds-specific, I think that'd be worth filing a bug with Apple.

2 Likes

Even I could not get it to crash when run from terminal using an executable swift file. I have filed a bug with Apple. But I am trying to steer attention towards, How to avoid Data Races in deinit, which has gotten no resolution. And I have seen crash due to race on deinit multiple times with enums. So this is still happening and an issue. Can you please take a look at the original forum post?

The response you got there is correct. The compiler does not know that you are accessing value in a multi-threaded way. If Swift had to synchronize all accesses to stored properties during deinitialization, it (a) would add overhead to every access everywhere else, even if it was cheaper than a full lock, and (b) wouldn't be the solution you want anyway, because you already have something that synchronizes access to value: queue.

There are a handful of good solutions to that problem, but the simplest one would be to not use [weak self] when putting closures on a queue owned by self. That's a temporary retain cycle, sure, but it'll be broken once the queue executes that closure.

In our production code we are NOT using [weak self]. We do use closures that mutate an optional struct (which is an enum of type .none initially). And in the example above the code still crashed when using just self.

Hmm. There shouldn't be a race here because deinit should happen-after all accesses to the object: all accesses should happen-before some release of the object, and releases of the object should all synchronize with the start of the destruction because of barriers we do in the runtime.

It's possible that TSan isn't aware of that, although that would be surprising.

I agree with Jordan that the crash is probably something else. It might be different in different configurations — maybe it's injected tracking code in Playgrounds and something else in your production code.

The reason I think it's the same error as the original forum post is because nowhere in our production code do we explicitly deinitialize values, we mutate an optional struct. And based on the forum post, a mutation needs deinitialization of the first value and then writing of the new value. If the enum is complex enough, then an inline deinit won't suffice and an outlined deinit will be added. This in turn leads to issues that won't occur for an inline deinit if I understand Crash : Outlined Destroy Of Object correctly.

Outlined/inlined should not affect anything from a thread-safety perspective.

The reason why I called out outline vs inline is the crash seems to occur for outlined and not inline. The working theory here is in case of inline the compiler can just overwrite the old value without having to worry about custom deinit logic. Which it has to do in case of the outlined deinit.

This seems to be a version of the Deallocation Problem, described in Apple's TN2109 where UIKit objects may be dealloced on a background thread, which can cause a problem.

Having said that in playing around with your code and the code from that apple forum post I never see a crash. You should file a bug at bugs.swift.org (the bugs filed at Apple are private). Without a reproducible case or a crash log ???

1 Like

Is bugs.swift.org something different from bugreports.apple.com? I have already filed a bug report at bugreports.apple.com: Swift: Enums with stored properties not thread safe even when mutated using a serial queue. Also I get a crash when running the code in Playground. And can you send a link to the Deallocation Problem?

Also, in playground I only get a crash popup message at racer.race(). It does not give me a stack trace.

This is the link for the Deallocation Problem. It's probably not exactly what you're describing but similar.

Apple's bug reporter is only for Apple and is private. Nobody else can view the bugs you've filed there. bugs.swift.org is part of the Swift open source project and anyone can view it. I think it's fine to file the same bug in both places.

Apple's response for this issue. Here's the open radar for it radar?id=4993916906504192

Thanks for contacting us. This behaves as expected.

While passing around value types, such as enums, avoids race conditions, this does not mean shared references to a variable holding a value type (such as stored properties of a class, as shown here) can be safely simultaneously mutated from multiple threads. Such behavior is a data race, and can lead to crashes.

Please close this feedback report, or let us know if this is still an issue for you. Thank you.

We are mutating the object via a serial queue so I don't fully agree with this response.

Indeed. But note that the post you're referring to from the developer forums seems to be mistaken: a strong reference is needed to call the modify() function, the asynced block will also capture a strong reference to self and hold it until after the block is executed, therefore it would be impossible for the deinit() and assignment to _value to occur concurrently. Something else (probably not obvious and possibly in code elsewhere) seems to be at play.

Couldn't this just be something blowing a limit of some kind? You're infinitely enqueuing work asynchronously and there must be a limit to the number of work items you can have enqueued.

@Jon_Shier I saw it crash within a few thousand runs