When building with targeted strict concurrency, I'm getting a warning that I'm wondering if there is anything I can do about other than make my type "unchecked Sendable".
Consider some thread safe property wrapper:
@propertyWrapper struct ThreadSafe<Value: Sendable>: Sendable {
private let lock: Lock<Value>
var wrappedValue: Value {
get { lock.withLock { $0 } }
set { lock.withLock { $0 = newValue } }
}
init(wrappedValue: Value) {
self.init(lock: .init(initialState: wrappedValue))
}
private init(lock: Lock<Value>) {
self.lock = lock
}
}
And consider the class that uses it to enforce thread-safety and sendability:
final class SomeSendable: Sendable {
@ThreadSafe
var someBool: Bool = false
}
Even though my class is thread-safe, I'm still getting this warning:
Stored property '_someBool' of 'Sendable'-conforming class 'SomeSendable' is mutable
It seems the compiler diagnostic isn't checking the property wrapper's Sendability.
Is there a way to appease this warning without making my type "unchecked"?
The problem is that any class with a var is always non-Sendable, and property wrappers don't allow let.
But really the problem, to me, is that SomeSendable really isn't all that safe to use, and in particular @ThreadSafe is not safe. It makes it far too easy to use mutable state in a way that is susceptible to races. Sure you won't get runtime crashes since the data is locked, but you can easily get incorrect results.
For example, something as simple as spinning up 1,000 tasks to toggle the boolean should always result in a true value at the end, but sometimes you get false and sometimes you get true:
let object = SomeSendable()
for _ in 1...1000 {
Task { object.toggle() }
}
try await Task.sleep(for: .seconds(1))
print(object.someBool)
That's a pretty big problem, and it's happening because @ThreadSafe allows direct writing to the underlying value. So something like:
object.someBool = !object.someBool
…is hiding a race condition.
Really you should probably just hold onto the Lock value directly in your class rather than using the @ThreadSafe property wrapper, and then only mutate through it's withValue. And of course if safety of the mutable data is an utmost concern, then really you should probably use an actor.
Furthermore I'm not sure that a struct can be used to implement a concurrent data structure due to the law of exclusivity (mutating the struct is the same as having an inout on self, but please someone correct me if I'm wrong). I'm certainly getting thread-sanitizer errors when trying, using a class instead makes those errors go away.
@mbrandonw@lukasa
I know this is quite old and crusty thread, but was wondering about the same - having propertyWrapper for managing manual synchronization.
What is actually wrong with such approach?
Is there any premise to Sendable types that would prevent interleaving events which we might have here?
For example actors - those are Sendable by definition - and you can still get interleaving type data race.
Whenever there is shared mutable state this is expected and inevitable so there is nothing wrong in above SomeSendable.
Am I missing some key part here?
Yeah, this isn't a requirement of Sendable types, it's just a thing you can get in a Sendable type.
Broadly, Sendable is a surprisingly weak guarantee. It says "If you use this type across isolation domains you won't get data races". It doesn't say "if you use this type across isolation domains you won't get bugs".
Consider this simple code, using this @ThreadSafe property wrapper:
// An `actor` that records the position of each word in a piece of text.
public class WordCounter {
@ThreadSafe public var elements: [Substring: [(Int, Int)]] = [:]
}
public func countWords(text: String, into counter: WordCounter) async {
let lines = text.split(whereSeparator: \.isNewline)
await withDiscardingTaskGroup { group in
for (lineNumber, line) in lines.enumerated() {
group.addTask {
let words = line.split(whereSeparator: \.isWhitespace)
for (wordNumber, word) in words.enumerated() {
// Bug in these lines! TOCTOU
if counter.elements[word] == nil {
counter.elements[word] = [(lineNumber, wordNumber)]
} else {
counter.elements[word]!.append((lineNumber, wordNumber))
}
}
}
}
}
}
This code shows the kind of bug you can see with this pattern. The code has no data races, but it has a logical bug: updates can be lost. Specifically, one thread can do the == nil check, while another thread is in the process of doing the initial insertion. That can lead you to throw some number of updates away.
This is a really trivial example so it's easy to say "well just don't write that buggy code", but the API here essentially begs you to write that buggy code. Notably, actors actually prevent you from doing this because they don't let you modify their state from outside the actor, and it's the modification where this is dangerous.
Generally speaking, you want all modification operations to be atomic (in the sense that they execute in one step), and to achieve that you tend to need to protect all your state by the same lock. That lets you enforce invariants properly.
Right, but it does not render ThreadSafe to be incorrect in general?
This mainly comes to the fact, that operations on elements from your example are atomic, but they use potentially stale data to make those operations hence we can loose data (as you marked: TOCTOU).
Actor only prevents that, because it forces to create isolated API to update its data (eg does not allow to modify variables outside of actor).
Nevertheless if you want you can still make same mistakes/bugs while using actors (especially global actors).
Right now I am basically only looking for Sendable compatible synchronization methods just to prevent "classic" data races (write while read) - for private properties and types non susceptible to TOCTOU and wondered if there is something hidden from me from intentions of Sendable types.
Would prefer to use OSAllocatedUnfairLock but it looks scary in my team
Smth like propertyWrapper handling that usecase would be more bearable and have nicer API as well.
To sum up my thoughts and based on what was already shared here - there is really nothing wrong with that ThreadSafe from POV of just being Sendable, right?
Regardless of that there is no way of making such property wrapper so that it satisfies Sendable conformance on declaration side as it is inherently a var
Protecting logical state of the system that operates using shared mutable state is very valid, but separate topic. For most of the use cases I have it is more important to protect against crash and TOCTOU is not to be considered. Worst case scenario some screen will be one iteration behind
Right. And in a way this is a disservice: having occasional crashes due to data races at least hints and reminds you about bug presence. Eliminate that and you've got no crashes, the app just doesn't work due to bugs which are now less exposed.
But actors do not prevent you from this type of high level races... A famous "crying cat" example from WWDC 2021 shows that: "Protect mutable state with Swift actors", time range: 9:00...13:00
So even though the cache was already populated with an image, we now get a different image for the same URL. That's a bit of a surprise. We expected that once we cache an image, we always get that same image back for the same URL so our user interface remains consistent, at least until we go and manually clear out of the cache. But here, the cached image changed unexpectedly. We don't have any low-level data races, but because we carried assumptions about state across an await, we ended up with a potential bug.
This is known as the Actor Reentrancy problem. The only way around it is to carefully write your actor logic, by considering what happens between each call to await. Actors guarantee that you won't be interrupted unless you await something again. So the ImageDownloader above can be improved by checking the cache a second time after try await downloadImage(...).
A more complex implementation of a downloader could keep a reference to Tasks that handle a single download, to make sure that multiple Tasks can't be started for the same URL.
And here be dragons... Notice that this snippet is trivial; real-world applications are larger, more complex, and far more susceptible to these errors. I wonder if this entire class of bugs could be eliminated completely by making actors strictly non-reentrant from the outset.
But then that just means you can have deadlocks, causing a different class of bugs. You can read the rationale for choosing reentrancy over deadlocks from the proposal:
Rationale: Reentrancy by default all but eliminates the potential for deadlocks. Moreover, it helps ensure that actors can make timely progress within a concurrent system, and that a particular actor does not end up unnecessarily blocked on a long-running asynchronous operation (say, downloading a file). The mechanisms for ensuring safe interleaving, such as using synchronous code when performing mutations and being careful not to break invariants across await calls, are already present in the proposal.
