Skip to content

[distributed] Require explicit assignment of actorSystem #41740

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Mar 25, 2022
Merged

[distributed] Require explicit assignment of actorSystem #41740

merged 10 commits into from
Mar 25, 2022

Conversation

kavon
Copy link
Member

@kavon kavon commented Mar 9, 2022 •
edited
Loading

As discussed in https://forums.swift.org/t/specifying-the-controlling-actor-system-with-distributed-actors-se-0336-0344/55584

As of now, we require exactly one parameter of a designated initializer of a distributed actor to conform to DistributedActorSystem. Then we implicitly initialize self.actorSystem and self.id based using that parameter. The problem is that there is no explicit use of this parameter, so it seems / feels visually like it is not needed.

Instead, this PR changes things so that we require users to manually initialize the actorSystem property in the designated initializer's body, dropping the parameter requirement. Since we need to right-away initialize the ID after doing this, we would keep the implicit initialization of the ID.

Resolves rdar://89582844 and rdar://86543336

@ktoso ktoso self-requested a review March 9, 2022 09:04
@ktoso ktoso added the distributed Feature → concurrency: distributed actor label Mar 9, 2022
ktoso
ktoso reviewed Mar 9, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 9, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 9, 2022
View reviewed changes
kavon
kavon commented Mar 9, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 16, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 16, 2022
View reviewed changes
@kavon kavon force-pushed the assign-actorSystem branch from 55d67d2 to fbdb527 Compare March 23, 2022 01:17
@kavon
Copy link
Member Author

kavon commented Mar 23, 2022

What's left for this PR is:

  • suppressing missing assignments to @_compilerInitialized properties if DI there is / will be another missing assignment / error in DI that would cause the initializer to be rejected. I think the way to do this is to just delay emitting the error diagnostic and have a check at the end to see if we emitted an error already.
  • SILGen, runtime, and DI tests.

kavon
kavon commented Mar 23, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 23, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 23, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 23, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 23, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 23, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 23, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 23, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 23, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 23, 2022
View reviewed changes
ktoso
ktoso reviewed Mar 23, 2022
View reviewed changes
ktoso
ktoso approved these changes Mar 23, 2022
View reviewed changes
Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far! This is looking really great; the impl ended up very clean thanks to the new attr as well.

@kavon kavon force-pushed the assign-actorSystem branch from 1a82e75 to af5d9cd Compare March 24, 2022 02:09
@kavon
Copy link
Member Author

kavon commented Mar 24, 2022

puttin out a feeler: @swift-ci please smoke test

@ktoso
Copy link
Contributor

ktoso commented Mar 24, 2022

As we unlock the local init tests this also resolves rdar://86543336

kavon added 5 commits March 24, 2022 16:18
@kavon
remove restrictions on dist-actor init parameters
1f47442 
we will no longer require exactly one ActorSystem-conforming parameter.
@kavon
rename default initializer's ActorSystem parameter name to actorSystem
01a5faf 
Konrad, myself, and others feel the longer name would be better, rather than just "system"
@kavon
remove dependence on system parameter in SILGen/DefiniteInit
6c0cfac
@kavon
adding the @_compilerInitialized attribute
13cbe0d 
This attribute is designed for let-bound variables whose initializing
assignment is synthesized by the compiler. This assignment is
expected to happen at some point before DefiniteInitialization has
run, which is the pass that verifies whether the compiler truly
initialized the variable.

I generally expect that this will never be a user-facing feature, and
that the synthesized assignment happens in SILGen.
@kavon
basic implementation of performing an init of the id after an assign …
af683dc 
…of the actorSystem.

The resignID call within the initializer moved into DI, because an assignment to
the actorSystem, and thus initialization of the id, is no longer guaranteed to happen.
Thus, while we previously could model the resignation as a clean-up emitted on all
unwind paths in an initializer, now it's conditional, based on whether the id was
initialized. This is exactly what DI is designed to do, so we inject the resignation
call just before we call the identity's deinit.
kavon and others added 5 commits March 24, 2022 16:20
@kavon
suppress notes from DI about @_compilerInitialized when reasonable
a4a5d32 
Since the only user of `@_compilerInitialized` is distributed
actors, I decided to make DI's diagnostics saying that such a
var was not initialized a second-tier note. If there is some other
var associated with the same Memory that is _not_ compiler initialized
AND is also _not_ initialized, then we suppress the suggestion to initialize
the @_compilerInitialized thing, because something else is problematic. Only
when there are @_compilerInitialized things that are still not initialized,
do we emit a note talking about that.
@ktoso @kavon
update distributed_actor_init_local test
51e806c
@kavon
fix code completion test
053286a
@kavon
more test updates for actorSystem assignment
74b3582
@kavon
more coverage for actorSystem assign
456140b
@kavon kavon force-pushed the assign-actorSystem branch from 4f9a99e to 456140b Compare March 24, 2022 23:20
@kavon kavon marked this pull request as ready for review March 24, 2022 23:34
@kavon
Copy link
Member Author

kavon commented Mar 24, 2022

@swift-ci please test

kavon
kavon commented Mar 25, 2022
View reviewed changes
Copy link
Member Author

@kavon kavon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok this edition looks good. just spotted some really minor clean-ups

@ktoso
Copy link
Contributor

ktoso commented Mar 25, 2022

Cool, let's clean those up separately and land this :)
I'll likely have small PRs coming around so may include those..

@kavon kavon merged commit d58bd27 into swiftlang:main Mar 25, 2022
@ktoso ktoso mentioned this pull request Mar 25, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktoso ktoso ktoso approved these changes

Assignees
No one assigned
Labels
distributed Feature → concurrency: distributed actor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kavon @ktoso