SE-0313: Improved control over actor isolation

The review of SE-0313, " Improved control over actor isolation", begins now and runs through May 11, 2021.

Reviews are an important part of the Swift evolution process. All review feedback should be either on this forum thread or, if you would like to keep your feedback private, directly to the review manager. When emailing the review manager directly, please keep the proposal link at the top of the message.

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of Swift. When writing your review, here are some questions you might want to answer in your review:

  • What is your evaluation of the proposal?
  • Is the problem being addressed significant enough to warrant a change to Swift?
  • Does this proposal fit well with the feel and direction of Swift?
  • If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?

More information about the Swift evolution process is available at:

https://github.com/apple/swift-evolution/blob/master/process.md

13 Likes

@tkremenek I have been reading through the GitHub proposal and I’ve noticed an incomplete sentence in the ‘Effect on ABI stability’ section:
“This is purely additive to the ABI. Function parameters will additionally be”

In other news: my first comment on the forums. :slight_smile:

3 Likes

Nit:

All of the limitations described above stem from the fact that instance methods

Shouldn’t that be functions?

+1

I think these changes makes actors more practical usable - I spent 20 minutes reading through skipping back to more interesting parts. I have no practical experience with actors, but have worked with asynchronous message passing systems with with encapsulated state between entities, so some relevance.

It’s nice to have more fine grained control.

I particularly liked:

The custom executors proposal provides control over the concurrency domains in which actors execute, which could be used to dynamically ensure that two actors execute in the same concurrency domain. That proposal could be modified or extended to guarantee statically that some set of actors share a concurrency domain to make functions with more than one isolated parameter mo…

That would be a very nice extension!

1 Like

Obvious question: how do we bridge between isolated and nonisolated state? For instance, perhaps we want BankAccount's Equatable implementation to use both the accountNumber and balance. How can we achieve that? It definitely seems like we need a solution for providing synchronous access to state that's normally isolated.

2 Likes

I think we are conflating two different things here that are not clearly communicated with just adding non in front of isolated.

isolated is meant to be something like a binding to an actor. Why is this needed? Why is it desired not to use await when talking to self? Is there a strong use case other than ascetics?

What we are calling nonisolated is very nice and I am +1 but could we use a different term here? In fact when I think about the word isolated I think of what we are calling non isolated. It’s about perspective right? In git some people say pull request while others say merge request. When I think about isolated, I think about isolating stuff out - never about isolating in. It doesn’t work for me. Usually I would probably say insulating for the “in” case. Anyway. That’s the color I’d like for this shed.

+1, with one significant comment/question below. This fills in several missing pieces of the actors proposal. It was split out to make the Actor review more manageable.

Yes, this is a key part of the Actor design.

No, I haven't used a language or library with these sorts of type system features, but I've read about Pony and bits of this proposal are tangentially related to some of its features. How exciting :slight_smile:

Other comments:


I'm concerned about this in Closure isolation control:

" Note that a @Sendable closure can only be actor-isolated if it is also async . Such closures will implicitly await at the beginning of the closure body to ensure they are running on the actor, then execute the rest of the body."

I initially thought that this was a typo in the second example, where the first closures should be passed to acceptSendable and the second to acceptNonSendable for the example to be correct (they're swapped) because an isolated actor reference isn't Sendable.

My concern here is that the behavior to do an implicit suspend breaks the "all suspensions are marked with an await" design of async/await in Swift. This seems highly dangerous given actor reentrancy. Is there a reason for this magic behavior?

Why not just make it invalid by consequence of the general type system rule that "isolated actor references (from the capture list) are not sendable"?


Relatedly, my understanding of the design point here was that the goal was to keep "isolated" references to be parameter modifiers, not a general type system feature. Doesn't allowing them in capture lists break this?

If this is a general type system feature (e.g. where you could have arrays of isolated actor references), then we need to make sure that an isolated actor reference is considered to not be @Sendable, because that would break memory safety / isolation checking.


Thank you for including the "Multiple isolated parameters" section, while estoteric, I think this makes the language more uniform and does support power users.


Technically this is a source compatibility break from the base actor proposal, since it changes partially applied function types, but no one cares since actors haven't been adopted yet.


Overall, great work Doug. Thank you for doing all the work writing this up!

-Chris

2 Likes

+1, this looks great and has been cleaned up nicely :slight_smile:

It is somewhat novel as well, as far as I can tell. It is not quite as powerful as Pony’s reference capabilities or Verona’s model, but it fits Swift and the model we have very naturally.

I love the use of isolated / nonisolated terms for this. I would probably stick to always calling it “actor-isolated” but in code that would become annoying so isolated sounds good to me.

Some notes:

  • The “actor instance methods are implicitly self-isolated” rule is very elegant and makes the concept fit nicely into the language.
  • I agree with the @Sendable-closure impact on isolation.
  • I’m happy with nonisolated func in an actor being able to implement a synchronous protocol requirement. This is crucial and I’m glad we’re agreeing on this.
    • Question: It seems we lost the “nonisolated unsafe” piece of the proposal in the pitch. Earlier we were considering either nonisolated(unsafe) or some form of withUnsafeNonisolatedActorState<A: Actor, B>(of: A) -> B { ... }, is there still some backdoor for that or do we want to not offer this until we hit a wall that cannot be solved with just nonisolated? (I think all my needs are addressed by nonisolated so I’m happy to punt it for now if we prefer that).
  • I’m okey with how we’re presenting the multiple-isolated parameter semantics in the proposal now.
    • IMHO the current ability is unlocks is somewhat useless (passing self twice), but that’s okay and I’m okay with not banning the spelling.
    • The actual use of this is going to be unlocked with custom executors and we can address the static same-serial-executor requirement checking then :+1: This will make for a powerful codification of a pattern is a hand-wavy pattern in other actor runtimes and crucial tool for hop-avoiding performance sensitive code (which today is done by frantically “please check your code is all on the same event loop” without any compiler assistance) — I’m very much looking forward to it.
4 Likes

Actually, there is one other important change that needs to be mentioned in this proposal: it is important that isolated actor references are not sendable across actors and other concurrency domains. We need to explicitly mention this in the proposal (and check it in the compiler) because isolated isn't a type. Example:

actor MyActor {
  var state : Int
  func x(a: isolated MyActor) { // Ok
     // Ok
     a.state += 1
     state += 1
   }

   func y(other: MyActor) async {
     self.x(a: self) // ok

      // error: cannot pass isolated self across actor hop boundary.
      await other.x(a: self)
   }
}

The same thing happens with structured concurrency, but I believe this is handled by promoting the isolated references to non-isolated. If there are other cases (like capture lists) where this isn't happening, then we'll need explicit checks.

One other clarification about the point in my previous comment, what I'm saying is that instead of allowing this through an implicit suspension:

extension A {
  func h() {
     acceptSendable {
       // implicit suspension happens here to get an isolated self, possibly causing reentrancy!
       [isolated self] async in
      f() // synchronous call to f() is okay, because closure is explicitly isolated to `self`
    }
  }
}

That we instead add a generally useful function like this:

extension Actor {
  func doOnActor(_ fn: @Sendable (isolated Self) -> ()) {
    fn(self)
  }
}

And then implement the above as:

extension A {
  func h() {
     acceptSendable {
       // 'self' is implicitly non-isolated in this context because this is a sendable closure.

       // explicit await makes suspension and possible-reentrancy explicit.
       await self.doOnActor { isolatedSelf in
          isolatedSelf.f()
       }
    }
  }
}

The doOnActor sort of function is also helpful when you want to invoke multiple sync functions from outside of an actor without a suspention:

actor YourActor { func syncThing1() {..} func syncThing2() {..} }

func doTwoThings(a: YourActor) async {
  // Reentrancy is possible between these.
  await a.syncThing1()
  await a.syncThing2()

  await a.doOnActor { isolatedA in 
    // Reentrancy is not possible between these.
    isolatedA.syncThing1()
    isolatedA.syncThing2()
  }
}

Of course, doOnActor deserves a better name :-)

-Chris

1 Like

I've updated the proposal to fix this sentence, thank you!

I think "methods" is accurate here, since we're inside an actor. Is there some case you're thinking of that isn't covered?

I don't think there is a good way to provide synchronous access to isolated state. You need to get onto the actor somehow, and that bit has to be asynchronous.

The way I wrote this is indeed confusing, because it does sound like there's a magic await. The await that indicates we might have a suspension point is the one required to call this async closure. The actual suspension point is at the beginning of the async function implementation, where it ensures that it's running on the right actor. I updated the wording to sound less magical:

Such closures are like async functions on the actor itself, and will switch to the actor at the beginning of the closure body to ensure that are running on the actor, then execute the rest of the body.

No, it doesn't break this, because it isn't general. It's explicitly capturing an isolated parameter in a closure. This happens implicitly all the time with self when you form a (non-Sendable) closure within an actor function, and this is the proposed syntax for allowing it to be explicit.

Yes, that's true. I'd worry about that if these features spanned multiple Swift releases, but that doesn't look likely to happen.

Yes, that's essentially what we're thinking. Let's start with the safe model, and when we hit the limitations we'll have a better understanding of what kind of unsafe backdoors we actually need.

The isolated is part of the parameter, not its type, and the proposal talks about the requirement that one interact asynchronously if you're passing a non-isolated value to an isolated parameter. Your example above is similar to what's in the section on multiple isolated parameters. Your x(a:) has two isolated parameters, self and a, and the erroneous call site is trying to provide different values to those isolated parameters. The section in the proposal implies such a thing would be erroneous

Therefore, the only way to safely call f is to pass the same actor twice:

but isn't clear what happens if you don't do that. I've extended the example there to show a similar erroneous case where you pass two different actors to isolated parameters.

This is part of the reason why this proposal started out banning multiple isolated parameters, because it's easier to explain than the above.

We found that we needed essentially this API on the main actor as well, where it looks like this:

extension MainActor {
  static func run<T>(operation: @Sendable @MainActor () throws -> T) async rethrows -> T { 
    return try await operation()
  }
}

It's essentially API sugar for writing an extension of the actor type. I'm not opposed to having it on Actor.

I think your contention is that we should have this API instead of allowing isolated parameters explicitly in capture lists, as in [isolated self]? Do you still feel that way even with the explanation above that [isolated self] is only an explicit way to spell something that happens implicitly?

For everyone: where I said I've clarified something, it's in the now-merged pull request at [SE-0313] Clarifications by DougGregor ¡ Pull Request #1351 ¡ apple/swift-evolution ¡ GitHub

Doug

10 Likes

I'm not qualified to give an opinion on the proposal (not a PL dev), still i feel like i should emit a warning about the "closure isolation" paragraph :

  • first, despite having read a few posts around "@Sendable", i realized my understanding of that attribute was backward: i read "sendable" as "value type that can be safely sent to another execution context", and as such, i expected it to mean that the execution context for the closure would change (sorry if my explanation sounds blurry, but that accurately reflects my state of confusion around that attribute).

  • second, having an attribute on the closure itself and not its parameter made me realize i had absolutely no clue what kind of implication that attribute would have regarding what was allowed inside the closure, nor in which context it would actually be executed.

I'm pretty sure all my confusions could be easily lifted after spending a few hours rereading the whole actor proposal. however, i believe it is a fairly significant symptom that a senior developer (15+ years of xp), interested in actor design, that follows the swift forums from time to time, is unable to understand the code samples in the "closure isolation" paragraph from first and second read.

Knowing how much people like to use closures in swift, I'm highly confident actor will be prohibited to junior developers in my team with the current state of the proposal (whether this was expected or not is up to you).

Note : to be clear, this remark isn't specifically about the current proposal. This proposal made me realize how many different and sublte new concepts & keywords the actor system is going to bring to the language, as well as the complexity to get a good mental model of the code execution (which seems a bit of shame knowing how actors are actually supposed to make concurrency more straightforward)

As far as I can tell, this is still an issue in the proposal. You have this code, which has no awaits. I think this is a serious issue with the proposal, and I don't understand why there is a need for this extra magic. Here is an except from the section of the proposal in question to show the issue:

func acceptSendable(_ operation: @Sendable () async -> Void) { ... }
actor A {
  func f() { }

  func h() {
    acceptSendable { [isolated self] async in
      f() // synchronous call to f() is okay, because closure is explicitly isolated to `self`
    }
  }
}

There are no awaits in the code above. The acceptSendable closure is taking a sendable closure, which can only capture the actor reference in a nonisolated way. The closure is somehow doing an implicit bounce into the actor, which causes a suspend. There is nothing in the code that illustrates this suspension point, which is a source of an implicit actor reentrancy.

Ok, it is just more special case sema checks. I'm not thrilled with that, but we can always generalize this behavior in the future if there is a reason to. In the meantime, keeping the more narrow type system behavior makes sense.

Yeah, I was just being pedantic and nit-picking the writing. I think it is ok, it is just possible to clarify the situation in the writing that is is all.

+1

but that isn't the right constraint to check, and I'm pointing out that the proposal is pretty vague about this. The issue in question is how nonisolated actor references (self is one example, but other named parameters can have the same behavior as illustrated in the example I showed) promote across the "implicit async" actor message send jump. This jump is the jump that checks for @Sendable conformance. I am pointing out that this jump also need to check for isolated self references as well.

The same issue occurs with a single isolated parameter, self is enough to cause the memory safety hole. It is related to the async promotion of crossing actors, not related to multiple arguments.

Yeah that would be great. Swift has a general duality between named and anonymous things (structs vs tuples, closure exprs vs functions, structure concurrency vs actors, etc) and it makes sense to support this for actor calls as well.

No, I'm not opposed to supporting this if you want to add the special case sema checks, this is an incremental improvement to the model. I'm pointing out 1) implicit suspension being a major problem for the actor model, and 2) a memory safety hole due to insufficient sema checks. It is possible that your implementation have these covered, but I think is critical to fix in the proposal (and the implementation if not).

-Chris

2 Likes

It's an async function, so the caller always has to await. This is not any different from any other actor function — if you're not on the actor already, the function implicitly switches at the start (and switches back at the end), and we consider the await for the call itself to cover those switches.

1 Like

Sorry for the stupid question but the function marked as async is the closure passed to acceptSendable. Is the implicit await done internally when this closure is called?

They've been elided by the ...:

func acceptSendable(_ operation: @Sendable () async -> Void) {
  detach {
    await operation()
  }
}

This is no different from an await when calling a async method on an actor.

I suspect that you didn't understand my point. Consider this:

actor A {
  var array: [Int] = []
  var array2: [Int] = []

  func slowAppendToSecondArray() {
    array.forEach {
      array2.append($0)
    }    
  }
}

The capture of self in the closure passed to forEach must be isolated, or else we wouldn't be allowed synchronous access to array2. The proposal is allowing you to be explicit about that implicit behavior:

    array.forEach { [isolated self] in
      array2.append($0)
    }    

It is the right constraint to check: all isolated parameters in a call must be provided with the same argument value, or it is impossible to guarantee proper isolation.

You're pointing out an additional constraint: if that argument value is not itself isolated, then we have a cross-actor access. The call must be asynchronous is subject to Sendable checking. This is shown early on in the proposal, where we asynchronously call when passing a non-isolated argument to an isolated parameter.

Having separate parameters that must all be provided with the same argument value sounds silly, because we've put the multiple-isolated-parameters cart before the custom-executors horse. The actual rule would be that all of the argument values must have the same executor, and that's the executor we hop to if we aren't already guaranteed to be on that executor. The trick is in making it reasonable to share executors across different actors in a manner that allows us to statically determine that they all share an executor.

You're conflating two issues, that of guaranteeing that we have a consistent actor on which the code runs in the presence of multiple isolated parameters (what I thought you were asking) and that of hopping to the appropriate actor if we're not already on it (what you were apparently asking). Both of these are independently addressed in the proposal, and the model remains sound. I am happy to clarify the proposal text, add examples, and spell out in more detail how this checking works.

I've concluded that it is a mistake for this proposal to admit multiple isolated parameters at all. We should ban multiple isolated parameters. Without custom executors, multiple isolated parameters have no practical value without falling back to unsafe hacks. And once we do get custom executors, we'll have to go and revise the rule anyway to rewrite it in terms of executors (more importantly, statically relationships among executors). It's better to keep things simpler now and generalize when we have tools to do it well, rather than pre-emptively adding features that can only be used unsafely.

Doug

[EDIT: Fixed the definition of acceptSendable to add a detach]

8 Likes

I'm more than happy that you came to that conclusions in the end, Doug :grinning_face_with_smiling_eyes:

It is not a point I was willing to throw pour more gas into the fire for, but I never understood the argument for allowing these making the language simpler, if anything it is just adding more weirdness.

As I mentioned before the actual value of this, and how the same thing is used nowadays but without compiler help of course in actor and event loop systems, will only be unlocked with custom executors:

That's the actual useful piece, and we're still quite far away from it... :slight_smile:

1 Like

Shouldn’t acceptSendable be async?

1 Like

Ah, thanks. I've updated my other reply to put a detach in there so we can await where needed.

Doug

2 Likes

If running on the same executor (The Actor’s) is the goal of isolated parameters then why should we introduce this feature now? Seems to be it’s more about executors (custom|actor) than just about actor isolation. Specially since it seems that multiple isolated parameters it’s not desired.

Aha, got it. Thanks I missed that.

Thanks Doug, I missed that!

That said, I am still not certain this is the model we want. The concerns about bugs introduced by unexpected actor reentrancy are a major potential issue with the actors design. While I agree with you that there is an await in the call chain, I still have a few questions:

  1. Isn't it concerning that it isn't "in the actor code"? This is making the author of the actor anticipate suspensions/reentrancy at await sites as usual, but also makes them look for "closures that take an isolated self parameter and are async". This complicates / defeats the simplicity of the 'await marking' approach, and undermines a significant amount of local reasoning that would otherwise exist.

  2. Why is this a better model than not having the magic? I haven't heard rationale for why we'd want to introduce this -- it isn't essential to the model or to expressivity. It is just stated as included behavior without much rationale.

  3. Given that this is effectively another syntactic sugar extension, why put it into this essential proposal introducing the essential type system capabilities? Sugar proposals have a higher bar than utility proposals - beyond explaining "what they make better", they have to also defend "why they are worth it on balance" which is a more difficult discussion.

Overall, the proposal doesn't seem to include much or any discussion about these points. I'm very sensitive to making the actor reentrancy model more complicated given that this is one of the boldest design stakes taken by the actor design.

Ok, got it, makes sense! This is similar to how unowned captures work.

I think that we are trying to address different concerns. My read from your comment leads me to believe that you're interested in proving that two different isolated actor references are on the same executor for safety purposes. I'm trying for something weaker and more important (to me): I want to make sure we reject incorrect code. Incidentally, I misstated when I said that the behavior occurs with a single actor reference, you are correct that it requires two. Here's what I'm trying to ensure that we reject correctly with an analogy for comparison:

actor MyActor {
  func takeClass(_ x : NSMutableString) {...}
  func takeIsolated(_ x : isolated MyActor) {...}

  func test(a : isolated MyActor, otherActor: MyActor, str: NSMutableString) {
     // Ok to pass isolated things and classes internally to an actor.
     self.takeClass(str)   // ok
     self.takeIsolated(a) // ok

     // Not ok to pass these to other non-isolated actors.
    otherActor.takeClass(str) // error: NSMutableString isn't @Sendable
    otherActor.takeIsolated(a)  // error: cannot isolated actor references aren't @Sendable
  }
}

I'm specifically bringing up the last error there. I'm pointing out that the sendability check for cross-actor hops needs to be applied to isolated actor references like that.

Yes, I'm just asking for clarity around the rules being spelled out in the proposal.

I completely disagree. Banning this has no apparent value, undermines a future direction, and complicates the language. We discussed this quite a bit in previous rounds of the proposal. I think that this is really important for (admittedly advanced!) use cases, and I don't think that banning it has any value.

-Chris

2 Likes