Converting `nonisolated(nonsending)` closure to `@concurrent` closure may cause data race

[rayx]

Description

See this example. It compiles on both 6.2 and nightly build. It looks like passing the nonisolated(nonsending) closure fn to bar, which takes a @concurrent closure, causes data race.

• foo is called in multiple Tasks.
• In each call of foo, the closure is actually run on global executor and hence access A.ns simultaneously.

Perhaps compiler shouldn't convert a nonisolated(nonsending) closure to @concurrent if the closure captures value that is in region other than disconected region or task-isolated region?

class NS {}

actor A {
    let ns = NS()

    func foo() async {
        let fn: nonisolated(nonsending) () async -> Void = { _ = self.ns }
        await bar(fn)
    }

    func bar(_ fn: @concurrent () async -> Void) async {
        await fn()
    }

    func test() {
        Task {
            await foo()
        }

        Task {
            await foo()
        }
    }
}

Note it appears that the data race couldn't be observed in practice, but the result is still weird. See more details in reproduction section.

Reproduction

Below is a complete test program that I used to demonstrate the data race. However, Its output isn't what I expected. fn isn't excuted on global executor, instead it's actor isolated.

task1 | BEGIN fn [output.A]
task2 | BEGIN fn [output.A]
task2 | END fn
task1 | END fn

A few comments:

1. Based on the output there isn't data race.
2. However, the output doesn't make sense. While it's true that fn in foo is actor isolated, I wonder how it's converted to a @concurrent closure when passed to bar? IMO the convertion perhaps should fail in the first place. It looks like what happens is that the convertion succeeds but the result is still an actor-isolated closure, instead of a @concurrent closure? I think it's worth to investigate what really happens.

class NS {
    var value = 0
}

func currentIsolation(isolation actor: (any Actor)? = #isolation) -> String {
    return "\(actor, default: "non-isolated")"
}

func logStart(_ fnName: String, _ isolation: String) {
    print("\(Task.name ?? "") | BEGIN \(fnName) [\(isolation)]")
}

func logEnd(_ fnName: String) {
    print("\(Task.name ?? "") | END \(fnName)")
}

actor A {
    let ns = NS()

    func foo() async {
        let fn: nonisolated(nonsending) () async -> Void = {
            logStart("fn", currentIsolation()) 
            _ = self.ns 
            try? await Task.sleep(for: .seconds(1))
            logEnd("fn")
        }
        await bar(fn)
    }

    func bar(_ fn: @concurrent () async -> Void) async {
        await fn()
    }

    func test() {
        Task(name: "task1") {
            await foo()
        }

        Task(name: "task2") {
            await foo()
        }
    }
}

let a = A()
await a.test()
try? await Task.sleep(for: .seconds(3))

Expected behavior

The code shouldn't compile

Environment

It can be reproduced on 6.2 and nightly.

Additional information

No response

[NotTheNHK]

If I'm reading the SIL correctly, that should be fine, since @concurrent simply removes the implicit isolated parameter applied to nonisolated(nonsending) functions, i.e., it does not force code to run on the GCE. You can see this in the SIL. All functions are isolated to the same actor. I think this example should be equivalent, but without the actor noise: https://swift.godbolt.org/z/qWKazT3Yb

[rayx]

@NotTheNHK , thanks for your suggestion about looking at SIL. While looking at SIL of your example, I did some experiments and ran into #87694. As your example is simplied (the closure doesn't actually access anything in MainActor, there is no hop_to_executor instructions in the closure's SIL) and it isn't feasible to modify it to do that, I'll continue to use my original example.

Why it's not feasible to modify your example

Even though compiler tells from the isolated parameter that the closure is MainActor isolated, it doesn't allow the closure to access anything in MainActor (not sure if this is related to the fact that MainActor may have multiple instances or if it's because of other limitations). For example this code doesn't compile:

var n = 0

func foo(_ isolation: isolated MainActor) async {
    let c: () async -> Void = { 
        _ = isolation 
        await n += 1 // This doesn't compile.
    }

    await bar(isolation, c)
}

func bar(_ isolation: isolated MainActor, _ fp: @concurrent () async -> Void) async {
    await fp()
}

SIL of my original example: https://swift.godbolt.org/z/1d4517473

My analysis of how `nonisolated(nonsending` and the conversion are implemented in SIL

(SIL is a new thing to me. Feel free to correct me.)

There are one closure and two thunks:

• Closure 1: It implements capturing A's instance. It takes one argument: an instance of A. Its partial_apply result is a closure of () async -> Void type.

  func closure(_ actor: A) async {
      hop_to_executor actor
      /// do something
  }
  

• Thunk 1: It implements nonisolated(nonsending) modifer. It takes two argument: a value of (any Actor)?, and a closure of () async -> Void type, which is partial_apply result of the above closure. its partial_apply result is a closure of ((any Actor)?) -> Void type, which I believe represents a nonisolated(nonsending) closure.

  func thunk1(_ actor: (any Actor)?, _ closure_partial_applied: () async -> Void) {
      // `actor` parameter isn't used inside the closure body. Q: Is it because
      // compiler knows it's unnecessary to hop to it in this case?
      closure_partial_applied()
  }
  

• Thunk 2: It implements nonisolated(nonsending) to @concurrent conversion. It takes one argument: a closure of ((any Actor)?) -> Void type, which is the partial_apply result of thunk1. Its partial_apply result is a closure of () async -> Void type, which I believe represents a @concurrent closure.

  func thunk2(_ thunk1_partial_applied: ((any Actor)?) -> Void) async -> Void {
      thunk1_partial_applied(nil)
  }
  

So I agree with what you said. The convension itself just does something like:

@concurrent
func wrapper(_ fn: nonisolated(nonsending) () async -> Void) async -> Void {
    await fn()
}

The hop is implemented in the the innermost closure and this is how it's implemented in Swift 6.1 which doesn't support nonisolated(nonsending) behavior. Another thing I realized during the investigation is that the closure is considered nonescaping by compiler even though it's assigned to a local variable, so it can't be captured by, say, task closure, which is escaping.

There is one minor thing I'm not sure about. I wonder, when a closure is marked nonisolated(nonsending), if the innermost closure still should hop? In my intuition as a user, I didn't expect it to do that. On the other hand, however, now that I know how it's implemented in SIL, it seems that it have to be this way because that's how it captures an actor.

[NotTheNHK]

Re: “Why it's not feasible to modify your example”

My bad. I didn't explain that I used MainActor simply as a means to avoid polluting the SIL with all the actor setup instructions that come with a custom actor. I assume that in your example above n is @MainActor isolated, which is different from being MainActor isolated. The latter simply represents MainActor as an instance rather than as a global-actor. If we were to replace MainActor with a custom actor:

actor A {
    var count = 0
}

func foo(_ isolation: isolated A) async {
    let c = { 
        isolation.count += 1
    }

    await bar(isolation, c)
}

func bar(
    _ isolation: isolated A,
    _ fp: @concurrent () async -> Void) async {
    await fp()
}

This works, c is statically isolated to isolation. We can access count synchronously. However, since c's type is inferred as: () → Void you won’t find a hop_to_executor instructions in c's SIL. Also note that there seems to be a slight difference in where and when the hop_to_executor instruction is called between isolated asynchronous methods and functions, i.e., thin calling convention vs method calling convention.

Furthermore, the presence or absence of an executor hop doesn’t necessarily imply the boundary of an execution job. In the following example, even though executor hops occur, the code still executes as a single job:

func foo(_ isolation: isolated A) async {
    print(1)

    let c = {
        print(4)

        isolation.count += 1
    }

    print(2)

    await bar(isolation, c)
}

func bar(
    _ isolation: isolated A,
    _ fp: @concurrent () async -> Void) async {
    print(3)

    await fp()

    print(5)
}

Here is the full sample: https://swift.godbolt.org/z/dzaa1PrKq .

Re: “Is it because compiler knows it's unnecessary to hop to it in this case?“

I think that, similar to @concurrent, nonisolated(nonsending) has no noticeable effect when the closure itself is statically isolated, doesn't support it, or when the function parameter is not nonisolated(nonsending). However, because it is part of the type, we still need to wrap it in a thunk. I want to elaborate further on this, but I don’t have much time right now.

[rayx]

@NotTheNHK Thanks for the explanation! It is very helpful.

the presence or absence of an executor hop doesn’t necessarily imply the boundary of an execution job. In the following example, even though executor hops occur, the code still executes as a single job:

I know little about this, but maybe it's an optimization (multiple jobs are merged into a single one because compiler can determine statically that they run on the same executor)?

I think that, similar to @Concurrent, nonisolated(nonsending) has no noticeable effect when the closure itself is statically isolated, doesn't support it, or when the function parameter is not nonisolated(nonsending).

That's an interesting way to think about it. It fits what I obserserved. But I suspect it's an artifact of the current implementation. IIUC all async functions are always on global executor first and then hop to appropriate exectuors when they start running, that's why thunks that implement @concurrent or nonisolated(nonsending) modifiers don't need to hop. However IMO @concurrent has its own semantics and shouldn't be silently discarded.