Specifying the controlling actor system with distributed actors (SE-0336/0344)

stevapple · 2022-02-23T08:45:15.215Z

I had probably missed the SE-0336 review, and I just found I was not so satisfied with the restriction of “distributed actor initializer must accept exactly one ActorSystem parameter”. Of course it is uncommon to pass multiple ActorSystems to the initializer, but such restriction and the initializer’s runtime behavior just didn’t fit in any existing Swift programming rules, and thus would be unexpected for anyone who didn’t know about it.

Given the implementation detail, I would suggest to let user specify self.actorSystem = … manually, and the runtime would synthesize self.id = self.actorSystem.assignID(Self.self) immediately after it. Users will be aware of what system is actually doing, and the additional restriction on the parameters will go.

EDIT: Can this also help with “long running initializer” problem by encouraging developers to postpone self.actorSystem initialization?

ktoso · 2022-02-23T11:42:56.969Z

Hmm, I don’t particularly mind revisiting this — the synthesis becomes much harder and I’m not sure if this would not incentivize bad patterns… “just make a global system variable and use it” (bad idea, don’t do this because testability).

But… this isn’t the topic of this review — shall we take this to a separate new topic in the distributed actors category on the forums? Happy to discuss there and keep this thread focused on the runtime, sounds good?

Wdyt @Joe_Groff, from a review process perspective?

// sorry typed on phone

xedin · 2022-02-23T18:11:36.735Z

@stevapple As @ktoso mentioned this is off-topic for runtime support but I'm curious what is the use-case for multiple actor systems you have in mind?

stevapple · 2022-02-24T03:37:31.071Z

They’re generally bad ideas, e.g. allocating a distributed actor into a random cluster (for load balancing), simply store an ActorSystem as a simple property, etc., but I don’t think they should ever be forbidden. The proposed synthesis rule adds a, IMO, quite bad exception into the Swift world, which asserts additional “burden” to everyone. You have to specify an argument that’s not used in the body, the compiler would reject you at some specific set of arguments, some property is implicitly initialized with a randomly-named argument in the initializer… We have to make the intention clearer, and remove such exceptions as many as possible.

xedin · 2022-02-24T22:01:55.701Z

The problem is that a system goes hand-in-hand with an actor because it's a source of all the requirements used for verification that's why I don't think it a good idea to accept multiple ones. That said, I don't have a strong opinion regarding system: argument, if there are no technical limitations (because the registration has to happen in a particular order and when actor is fully initialized) it might be worth it to allow supplying a system via self.actorSystem = ... in an initializer instead.

stevapple · 2022-02-26T13:04:42.019Z

Of course I don’t think it is a nice idea, but the proposed synthesis rule is counter-intuitive for Swifters. Again, I would quote my previous conclusion:

You have to specify an argument that’s not used in the body, the compiler would reject you at some specific set of arguments, some property is implicitly initialized with a randomly-named argument in the initializer…

These, I believe, are worse because they’re harming some basic rules of the language itself. This would confuse any Swift user who just came across a distributed actor implementation. Compiler makes some magic, but “consuming any DistributedActorSystem regardless of its name, type, label and position” makes the compiler behave like a drunken wizard.

ktoso · 2022-02-28T03:23:30.807Z

There's no need to reach for derogative "drunken wizard" wording, let's tone down it a bit. The approach is consistent with default initializers, and was the only thing implementable at the time.

I'm more than happy to lift such restrictions, but am not sure about the implementation complexity and will have to reach out for help across the team to make it happen I think... Thankfully, this is the right time to still tweak things like that as distributed actors right now are still going through evolution before removing the experimental flag - so, thanks for raising the (valid) concern!

To clarify the complexity I mean, here are two snippets:

Current synthesis:

// with implicitly assigned system property:
init(system: SomeActorSystem) { 
  // self.actorSystem = system
  // self.id = system.assignID(Self.self)
  self.name = "alice"
  // at end of init:
  // system.actorReady(self)
}

This is pretty simple synthesis, we just add the two assignment statements.

Now when end-users are in charge of assigning the properties we have a whole new world of problems related to it sadly... E.g.

init(system: SomeActorSystem) {
  self.actorSystem = system // we need to "detect" this was an assignment to actorSystem
  // self.id = system.assignID(Self.self) // inject this immediately after
  self.name = "a"
  // at end of init:
  // system.actorReady(self)
}

So this "detect this was a specific assignment" is somewhat painful... but I'll look into it with @kavon who has been working on (distributed) actor initializers in general.

Downsides:

Complexity of detecting the "actor system assignment" is a bit tricky and injecting the assignID is also harder then...
It remains "magical" that one has to assign to a property that was never declared anywhere
- we might have to offer special diagnostics explaining why that is
The default initializer still must accept a system, so the default generated init still would be init(actorSystem:)
- perhaps we take the stab here to rename it to actorSystem in line with the property then...?

Upsides:

we allow the anti pattern of init() { self.actorSystem = SomeGlobal.system }

So... I don't actually think it is such a big deal to be honest, and removing this limitation has a hefty complexity cost. Removing the limitation also only really invites people to do the wrong thing, and hardcode a system value into initializers, but it could be argued people should be allowed to do so if they really want.

I'll look into the implementation complexity some more with Kavon and we'll see what we can do.

stevapple · 2022-02-28T03:53:36.320Z

Huge thanks to you and @kavon for implementing this exciting feature!

To be clear, I was not proposing this change from the aspect of functionality (it may help at some circumstances, but not the main point). It is more of a language design challenge. Let’s look at the following “smelly” code from a code reviewer’s respective:

distributed actor DA {
  let uuid: UUID

  init(notUsing actorSystem: DistributedActorSystem) {
    self.uuid = UUID()
  }
}

What an innocent reviewer saw from this code is that there’s an “unused” but necessary initializer parameter. It’s easy to regard this as an effort to comfort the compiler, which is actually seeing it as the actor system to use at runtime.

I totally agree that we still need magic when using this feature. It would be better to make it more straightforward. It’s okay to accept actorSystem as some auto-synthesized stub of distributed actors IMO.

ktoso · 2022-02-28T03:55:16.054Z

Sure yeah, I get that this argument is annoying. We'll see if we're able to implement the "detect assignment".

benrimmington · 2022-03-01T10:29:30.603Z

ktoso:

Thankfully, this is the right time to still tweak things like that as distributed actors right now are still going through evolution before removing the experimental flag - so, thanks for raising the (valid) concern!

The availability of distributed types was changed in apple/swift#41182, but only for the main branch. Should the release/5.6 branch also be updated, or is the distributed module not part of that release?

ktoso · 2022-03-01T10:31:57.392Z

Distributed actors won't ship stable in 5.6 so it's not a concern.

The reason the code for them is even there is because development was ongoing since 5.5+ on main, and when main stable/5.6 forked off it inherited this work in progress feature. It is not worth removing it because it'd cause more issues with cherry picking other actor related changes to 5.6 etc, so we're just keeping it there but it is not used (and disabled via a feature flag).

ktoso · 2022-03-03T05:44:21.840Z

Looping back here, we (thanks @kavon!) spent some time looking into the ability to implement the suggestion:

stevapple:

I would suggest to let user specify self.actorSystem = … manually, and the runtime would synthesize self.id = self.actorSystem.assignID(Self.self) immediately after it.

We agree it's a good cleanup / direction and the implementation complexity is managable -- we'll pull this in as an amendment to the design in the ongoing review

Thanks @stevapple for reminding us about this wart!