Search code examples
swiftswift-concurrency

Is it safe to make a struct containing a closure Sendable?


I want to have a Sendable struct, which contains a closure. This closure takes in a reference type, but returns Void, therefore the Greeter doesn't directly stores the Person reference. However, closures themselves are still references anyway.

Current code:

class Person {
    let name: String

    init(name: String) {
        self.name = name
    }
}

struct Greeter: Sendable { // <- Trying to make this Sendable
    let greet: (Person) -> Void

    init(greeting: String) {
        greet = { person in
            print("\(greeting), \(person.name)!")
        }
    }
}
let person = Person(name: "George")
let greeter = Greeter(greeting: "Hello")
greeter.greet(person)

// Hello, George!

In my actual problem (this is simplified) I don't actually know Person's implementation and so can't mark it Sendable. It's actually a MTLRenderCommandEncoder, but for simplicity we just have Person.

On the greet definition, I get the following warning:

Stored property 'greet' of 'Sendable'-conforming struct 'Greeter' has non-sendable type '(Person) -> Void'

I can make the warnings go away, but I don't think it's the safe & correct solution:

struct Greeter: Sendable {
    let greet: @Sendable (Person) -> Void

    init(greeting: String) {
        greet = { @Sendable person in
            print("\(greeting), \(person.name)!")
        }
    }
}

How can I be sure this code is safe across threads?


Solution

  • I reason that you can’t. Someone else may have a reference to Person, modify it concurrently and break your assumptions.

    But you could create a PersonWrapper: @unchecked Sendable that duplicates Person if there is more than one reference or stores it as a serialized Sendable type. This may be expensive but it will be safe. You may also have to lock if you make changes, and return duplicates instead the real thing.

    A trivial example:

    public struct SendableURL: Sendable {
        private let absoluteString: String
        public init(_ url: URL) {
            self.absoluteString = url.absoluteString
        }
        public var url: URL {
            URL(string: absoluteString)! 
        }
    }
    

    The version that deals with non serializable objects would be:

    public final class SendablePerson: @unchecked Sendable {
        private let _person: Person
        private init(_ person: Person) {
            self._person = person
        }
        public static func create(_ person: inout Person) -> SendablePerson? {
            let person = isKnownUniquelyReferenced(&person) ? person : person.copy()
            return SendablePerson(person)
        }
        public func personCopy() -> Person {
            _person.copy()
        }
    }
    

    What do you think? I reason that as long as you avoid shared mutable state you should be fine. If you are unable to copy the object you depend on it not being modified.

    In practice, we do unsafe things every day (e.g. passing a Data/UIImage, etc.) through threads. The only difference is that SC is more restrictive to avoid data races in all cases, and let the compiler reason about concurrency.

    I’m trying to figure out this stuff in the face of ever increasing warnings levels in Xcode, and lack of guidance. 🤷🏻‍♂️


    Make it an actor:

    public final actor SendablePerson: @unchecked Sendable {
        // ...
        public func add(_ things: [Something]) -> Person {
            _person.array.append(contentsOf: things)
            return _person
        }
    }
    

    or start every instance method with a lock()/unlock().

    public final class SendablePerson: @unchecked Sendable {
        // ...
        private let lock = NSLock()
    
        public func add(_ things: [Something]) {
            lock.lock()
            defer { lock.unlock() }
            _person.array.append(contentsOf: things)
            return _person
        }
    
        // or 
        public func withPerson(_ action: (Person)->Void) {
            lock.lock()
            defer { lock.unlock() }
            action(_person)
        }
    }
    

    In both cases every method will execute fully before another method is called. If one locked method calls another locked method replace NSLock with NSRecursiveLock.

    If you can’t hand Person copies, be mindful not to pass references to code that stores and mutates Person outside your wrapper.

    The create/copy thing:

    • If a thread is changing the state of Person concurrently I have no guarantee that what I read from Person will still be true before I act on it. But If I hand copies, I know threads will at most modify their own copy.
    • The create is a way to create a wrapper to attempt to synchronize changes.

    The root of all concurrency problems is mutable shared state. And the way to solve them is to either prevent access, make the state immutable, or provide orderly access to the state.