swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.16k stars 10.32k forks source link

Circumvention of Swift 6 data race detection. #74820

Open dkyowell opened 1 month ago

dkyowell commented 1 month ago

Description

Adding @MainActor annotation to a function allows for circumvention of Swift 6 data race safety.

In the following code, an error is detected without the @MainActor annotation on func testCircumvention(). With @MainActor added to func testCircumvention(), no error is flagged and a data race condition is achieved with the code.

Reproduction


class MutableState {
    var value: Int

    init(value: Int) {
        self.value = value
    }
}

actor MutableStateGuardian {
    private let state: MutableState

    init() {
        self.state = MutableState(value: 0)
        Task {
            await increment()
        }
    }

    func increment() {
        for _ in (1...100_000) {
            state.value += 1
        }
    }

    func stealState(thief: StateThief) {
        thief.state = state
    }
}

class StateThief {
    var state: MutableState?
}

let mutableStateGuardian = MutableStateGuardian()

@MainActor // <- without @MainActor annotation, data race is detected
func testCircumvention() async -> Int {
    let thief = StateThief()
    Task {
        await mutableStateGuardian.stealState(thief: thief)
    }
    while thief.state == nil {
        try? await Task.sleep(nanoseconds: 1)
    }
    for _ in (1...100_000) {
        thief.state!.value -= 1
    }
    return thief.state!.value
}

Expected behavior

Compiler should flag an error with this code.

When testCircumvention() is run, a random result is returned each time after concurrently adding 100,000 and subtracting 100,000 from a shared initial value of 0.

Environment

Version 16.0 beta (16A5171c) / Swift 6 language mode

Additional information

No response

Androp0v commented 1 month ago

FWIW, I think @MainActor is not the issue here. The error I get when compiling the code above (without @MainActor) is:

ERROR: Capture of 'thief' with non-sendable type 'StateThief' in a @Sendable closure

Indeed, this is fixed by adding @MainActor to the enclosing function, as that ensures that both the local thief and the closure that captures thief are in the same isolation context (both in the Main Actor).

The true problem (I think) happens later, when sending thief to a different actor inside the closure. The compiler is not emitting the error:

ERROR: Sending 'thief' risks causing data races

Removing the Task around await mutableStateGuardian.stealState(thief: thief) makes the error surface. So I think the key here is understanding why this code shows the expected error:

@MainActor func testCircumvention() async -> Int {
    let thief = StateThief()
    await mutableStateGuardian.stealState(thief: thief) // ❌ Sending 'thief' risks causing data races
    // Code that modifies 'thief.state'...
}

While this doesn't (at least, it doesn't show an error about sending thief):

@MainActor func testCircumvention() async -> Int {
    let thief = StateThief()
    Task {
        await mutableStateGuardian.stealState(thief: thief) // 🙁 Doesn't warn about sending 'thief'
    }
    // Code that modifies 'thief.state'...
}
dkyowell commented 1 month ago

Here is a reproduction of the issue without the @MainActor func testCircumvention() and without the extra Task encapsulated execution of await mutableStateGuardian.stealState(thief: thief). This version runs as a command line program.

class MoneyBag {
    var value: Int

    init(value: Int) {
        self.value = value
    }
}

let count = 10_000_000

actor SafteyVault {
    private let state: MoneyBag
    private var complete = false

    init() {
        print("MutableStateGuardian.init")
        self.state = MoneyBag(value: 0)
    }

    func increment() {
        complete = false
        print("actor start")
        for _ in (1...count) {
            state.value += 1
        }
        print("actor done")
        complete = true
    }

    func stealState(thief: StateThief) {
        thief.state = state
    }

    func waitForCompletion() async {
        while complete == false {
            try? await Task.sleep(nanoseconds: 1)
        }
    }

    func stealMoneyBag() -> MoneyBag {
        state
    }
}

class StateThief {
    var state: MoneyBag?
}

let safteyVault = SafteyVault()
let thief = StateThief()
// let moneyBag = await safteyVault.stealMoneyBag() // ❌ "Non-sendable type 'MoneyBag...
await safteyVault.stealState(thief: thief) // ✅ This line passes
Task.detached {
    await safteyVault.increment()
}
print("main thread start")
for _ in (1...count) {
    thief.state!.value -= 1
}
print("main thread done")
await safteyVault.waitForCompletion()
print("final count", thief.state!.value)

Neither MoneyBag, nor StateThief is Sendable. The main thread and the actor SafteyVault define different isolation domains. The Data Race Safety document states "Data isolation guarantees mutually exclusive access to mutable state." It seems to me from this example that it is rather trivial to share concurrent access of mutable state between one actor and a global actor.

mattmassicotte commented 1 month ago

I have reduced this down significantly. It looks like unsafe transfers in top-level code are not detected. Identical program flow within a function correctly produces an error.

class NonSendable {
    var mutableState = 1
}

actor MyActor {
    private var state = NonSendable()

    func acceptNonSendable(_ ns: NonSendable) {
        self.state = ns
    }
}

let topLevelNS = NonSendable()
let topLevelActor = MyActor()
await topLevelActor.acceptNonSendable(topLevelNS)

// this clearly is a race, but produces no error
topLevelNS.mutableState += 1

func withInFunction() async {
    let ns = NonSendable()
    let actor = MyActor()
    await actor.acceptNonSendable(ns)

    // error: sending 'ns' risks causing data races
    ns.mutableState += 1
}
dkyowell commented 1 month ago

I do not believe it is an issue with just top level code. Adding @MainActor to withinFunction AND placing await actor.acceptNonSendable(ns) within a Task puts it back into passing the compiler without an error. Perhaps the compiler is incorrectly applying Region based Isolation analysis since ns.mutableState += 1 happens after and outside the task.

EDIT All of these examples--both top level and those with @MainActor functions--give warnings within Xcode 15.4 but no errors in Xcode 16 beta. Perhaps another possibility for the error could be @isolated(any) of SE-0431. In Swift 6.0, the function passed to Task has @isolated(any), but it is not present in Swift 5.10.

class NonSendable {
    var mutableState = 1
}

actor MyActor {
    private var state = NonSendable()

    func acceptNonSendable(_ ns: NonSendable) {
        self.state = ns
    }
}

@MainActor
func withInFunction() async {
    let ns = NonSendable()
    let actor = MyActor()
    Task {
        await actor.acceptNonSendable(ns)
        // THIS WOULD RAISE AN ERROR HERE: ns.mutableState += 1
    }
    ns.mutableState += 1
}