Changed NIOAsyncChannel.executeThenClose to accept actor-isolated par…

[rafaelcepeda]

Improve usability of NIOAsyncChannel.executeThenClose function from an Actor

Motivation:

When calling executeThenClose from inside an actor, the closure which we pass is not isolated to the same actor
This makes it hard to use

e.g. the following code won’t compile on Swift 6 because the closure is self-isolated so can’t be passed

await withDiscardingTaskGroup { group in
            do {
                try await serverChannel.executeThenClose { inboundStream, _ in
                    try await self.handleInboundStream(inboundStream: inboundStream, group: &group)
                }
             } catch {}
}

Making the closure as @sendable allows it to be passed, but then we can’t access group anymore

Modifications:

Added actor isolation parameter to the executeThenClose function.

Result:

executeThenClose will be able to be asynchronously called from inside an Actor, as it will now be part of the Actor's isolation domain.

[gjcairo]

Nit: the parameter name here is actor - isolatedTo is the label (you can think of it as a part of the method's name).

[rafaelcepeda]

Fixed the issue.

[gjcairo]

This change is actually API-breaking: even if the parameter you add is defaulted, the function signature changes.

For instance, imagine you have:

func someFunc(foo: Int) { ... }

func doSomething() {
  let f: (Int) -> Void  = someFunc(foo:)
  // ...
}

Say you add a new defaulted parameter...

func someFunc(foo: Int, bar: String = "baz") { ... }

func doSomething() {
  let f1: (Int) -> Void  = someFunc(foo:)  //  this will fail to build: "no member named someFunc(foo:)"
  // Should be: 
  let f2: (Int, String?) -> Void  = someFunc(foo:bar:)
}

You will have to add a new executeThenClose overload that takes a (non-optional) isolated param.

[rafaelcepeda]

Great point. I've added the old API back and have implemented it using the new one.

[hamzahrmalik]

Thanks for this change, it will make it much easier to use asyncchannels!

Couple of points:

1. this will not work on older compilers, so probably needs some #if compiler >= 6.0 sprinkled around
2. following on from gus's comment, are we sure the overload resolution will pick the right thing here? Can we add a test which uses this from an actor to make sure that compiles fine?

The testcase would be something like having an actor-isolated function call executeThenClose and then attempt to reference self inside the closure

[rnro]

We seem to have dropped the defaulted parameter here which I think we want.

[gjcairo]

The problem is the compiler picking the right func if we default this parameter (i.e. if you call executeThenClose { ... } are we calling the old version, or the one defaulting isolation?).
We could use @_disfavoredOverload, but have we agreed that is the route we generally want to take?

Specifically in this case, if what we want is to enable adopters to use executeThenClose from actors, I don't think it's particularly bad to have to explicitly pass self as isolation, as it makes it clear you want to run this on the actor, even though you're awaiting and would otherwise be hopping off the isolation domain.

[rafaelcepeda]

As spoken offline, we decided to favour the use of @_disfavoredOverload with this API. I'll change the code.

[rnro]

Looks good in general, unfortunately there are some fiddly considerations here. Here's a link to a similar block of code which includes the compiler check which Hamzah mentions https://github.com/apple/swift-service-context/blob/8946c930cae601452149e45d31d8ddfac973c3c7/Sources/ServiceContextModule/ServiceContext.swift#L232-L266

[FranzBusch]

I think we should also deprecate those methods since in theory those are not concurrency safe and we want users to adopt a Swift 6 compiler to make those methods safe again.