Skip to content

Calling an isolated function in a non-Sendable closure shouldn't be sending self #77067

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mattmassicotte opened this issue Oct 17, 2024 · 11 comments
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. concurrency Feature: umbrella label for concurrency language features

Comments

@mattmassicotte
Copy link
Contributor

Description

I'm not sure how it's possible that self could be sent in this situation. The closures involved are not @Sendable, so the isolation should never change.

- error: sending 'self' risks causing data races
- note: 'isolation'-isolated 'self' is captured by a actor-isolated closure. actor-isolated uses in closure may race against later nonisolated uses

Reproduction

class MyClass {
	var handler: (@escaping () -> Void) -> Void = { _ in }

	func doThing(isolation: isolated (any Actor)?) {
		handler {
			// error: sending 'self' risks causing data races
			self.doOtherThing(isolation: isolation)
		}
	}

	func doOtherThing(isolation: isolated (any Actor)?) {
	}
}

Expected behavior

I would expect this to compile error-free.

Environment

swift-driver version: 1.115 Apple Swift version 6.0.2 (swiftlang-6.0.2.1.2 clang-1600.0.26.4)
Target: arm64-apple-macosx14.0

Additional information

No response

@mattmassicotte mattmassicotte added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. triage needed This issue needs more specific labels labels Oct 17, 2024
@ollieatkinson
Copy link

ollieatkinson commented Oct 17, 2024

Strangely, marking handler as @Sendable makes the error go away

class MyClass {
    let handler: @Sendable (@escaping () -> Void) -> Void = { _ in }

    func doThing(isolation: isolated (any Actor)?) {
        handler {
            self.doOtherThing(isolation: isolation)
        }
    }

    func doOtherThing(isolation: isolated (any Actor)?) {
    }
}

Removing actor isolation parameters also makes it pass: (obvious, but wanted to provide a complete picture)

class MyClass {
    let handler: (@escaping () -> Void) -> Void = { _ in }

    func doThing() {
        handler {
            self.doOtherThing()
        }
    }

    func doOtherThing() {
    }
}

Also works when avoiding a closure's implicit capture of isolation:

class MyClass {
    var handler: (@escaping () -> Void) -> Void = { _ in }

    func doThing(isolation: isolated (any Actor)?) {
        func _doOtherThing() {
            doOtherThing(isolation: isolation)
        }
        handler(_doOtherThing)
    }

    func doOtherThing(isolation: isolated (any Actor)?) {
    }
}

@ktoso ktoso added concurrency Feature: umbrella label for concurrency language features and removed triage needed This issue needs more specific labels labels Oct 17, 2024
@mattmassicotte
Copy link
Contributor Author

Here is another workaround someone found for me, which I think pretty much confirms that this is a bug of some kind.

class MyClass {
	var handler: (@escaping () -> Void) -> Void = { _ in }

	func doThing(isolation: isolated (any Actor)?) {
		handler {
			self.doOtherThing(isolation: #isolation)
		}
	}

	func doOtherThing(isolation: isolated (any Actor)?) {
	}
}

@mattmassicotte
Copy link
Contributor Author

Welp, actually that workaround is actually a way to pass the non-Sendable type across boundaries.

class MyClass {
	var handler: (@escaping () -> Void) -> Void = { $0() }

	func doThing(isolation: isolated (any Actor)?) {
		print("in", isolation as Any)

		handler {
			self.doOtherThing(isolation: #isolation)
		}
	}

	func doOtherThing(isolation: isolated (any Actor)?) {
		print("out", isolation as Any)
	}
}

let value = MyClass()

value.doThing(isolation: #isolation)

output:

in Optional(Swift.MainActor)
out nil

@mattmassicotte
Copy link
Contributor Author

@gottesmm this is a potentially interesting one. This is both a data race safety hole, on top of being a difficult-to-workaround problem in general.

@CrystDragon
Copy link

Welp, actually that workaround is actually a way to pass the non-Sendable type across boundaries.

output:

in Optional(Swift.MainActor)
out nil

Hi, I just read your comments, and I'm confused how you deduced the code is "to pass the non-Sendable type across boundaries"?
If I understand it correctly, in a non-async caller, a nil-value argument for an isolated parameter tells nothing about the caller's dynamic isolation, because that argument can only be nil.

@mattmassicotte
Copy link
Contributor Author

I'm afraid I don't fully understand the question. Are you saying you think this isn't a data race hole? Or that the isolation differences make sense?

The type is non-Sendable, so it is should be impossible to access self from two different isolation domains.

@mattmassicotte
Copy link
Contributor Author

(this is also potentially similar to #77301)

@CrystDragon
Copy link

I'm afraid I don't fully understand the question. Are you saying you think this isn't a data race hole? Or that the isolation differences make sense?

The type is non-Sendable, so it is should be impossible to access self from two different isolation domains.

That't right, I think there isn't a hole in this case, the difference of the 2 printed #isolation is OK I believe.
Just the same as what you said in your original post, the actual dynamic isolation should never change, especially when the code is just calling non-async functions.

@mattmassicotte
Copy link
Contributor Author

hmm maybe, I'd have to think on this more. But my gut says that this is a bug. I don't see how differences between static isolation and runtime behavior should ever be ok...

@gottesmm
Copy link
Contributor

This is fixed by 0ece31e

@mattmassicotte
Copy link
Contributor Author

Confirmed - thank you!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. concurrency Feature: umbrella label for concurrency language features
Projects
None yet
Development

No branches or pull requests

5 participants