Skip to content

Restrict uses of 'self' in actor initializers #37075

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.

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

Merged
merged 1 commit into from
Jun 24, 2021

Conversation

kavon
Copy link
Member

@kavon kavon commented Apr 27, 2021

Fixes the following:

  • Actors permit synchronous initializers, and initializers isolated to a global-actor, which makes it difficult to both create the actor-instance and switch to it within the initializer function. Thus, data races can be created with the initializer. This PR fixes this by broadly restricting the uses of self in such initializers to ensure that one unique reference to self exists throughout the init.

  • When I added support for async initializers, I deferred the appropriate insertion of hop_to_executor in actor initializers because it's not so simple to determine where that should take place. This PR fixes that and emits the hop. Deferred to a later PR

Resolves rdar://76620928

@kavon kavon added bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. concurrency Feature: umbrella label for concurrency language features labels Apr 27, 2021
@kavon kavon force-pushed the actor-initializers branch 4 times, most recently from 50e9e5f to d3a7ea4 Compare June 4, 2021 22:02
@ktoso ktoso self-requested a review June 7, 2021 03:48
@kavon kavon force-pushed the actor-initializers branch from d3a7ea4 to 61e7f76 Compare June 10, 2021 01:58
@kavon
Copy link
Member Author

kavon commented Jun 10, 2021

@swift-ci please smoke test

@kavon kavon force-pushed the actor-initializers branch 6 times, most recently from 1efbe77 to b2db522 Compare June 21, 2021 23:54
@kavon kavon force-pushed the actor-initializers branch 3 times, most recently from 351bcf7 to c696312 Compare June 22, 2021 23:15
@kavon kavon marked this pull request as ready for review June 22, 2021 23:16
@kavon
Copy link
Member Author

kavon commented Jun 22, 2021

@swift-ci please smoke test

@kavon kavon force-pushed the actor-initializers branch from c696312 to cdba933 Compare June 22, 2021 23:23
@kavon
Copy link
Member Author

kavon commented Jun 22, 2021

@swift-ci please smoke test

@kavon kavon changed the title Prevent races in actor initializers Restrict uses of 'self' in actor initializers Jun 22, 2021
@kavon kavon force-pushed the actor-initializers branch from cdba933 to af36655 Compare June 22, 2021 23:34
@kavon
Copy link
Member Author

kavon commented Jun 22, 2021

@swift-ci please smoke test

if (auto decl =
dyn_cast_or_null<ClassDecl>(getASTType()->getAnyNominal()))
// is it for an actor?
if (decl->isActor() && !decl->isDistributedActor()) // FIXME(78484431) skip distributed actors for now, until their initializers are fixed!
Copy link
Contributor

Choose a reason for hiding this comment

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

ok :)

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 AFAICS :-)

@kavon
Copy link
Member Author

kavon commented Jun 23, 2021

Turns out that I missed one case of escaping:

var globalVar: EscapeArtist?

actor EscapeArtist {
    var x: Int
    init() {
        self.x = 0
        globalVar = self
        Task { await globalVar!.isolatedMethod() }
        if self.x == 0 {
            fatalError("race detected.")
        }
    }

    func isolatedMethod() { x += 1 }
}

Working on getting the above rejected as an error on that store of self. Edit: This has been fixed now.

A uniqueness rule for 'self' is enforced throughout
the following types of actor initializers:

1. synchronous
2. global-actor isolated

This means that 'self' cannot be used for anything,
even after 'self' is fully initialized, other than
for access to stored properties. Method calls are
considered escaping uses of 'self', since it is passed
implicitly to the method of computed property when invoked.

Also, an actor's convenience init now treats self as
nonisolated.

This provides a way to perform follow-up work after
self is fully initialized, when creating the
actor from a synchronous context.

Resolves rdar://78790369
@kavon kavon force-pushed the actor-initializers branch from 2280fcd to f722ccd Compare June 23, 2021 21:17
@kavon
Copy link
Member Author

kavon commented Jun 23, 2021

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 735064a into swiftlang:main Jun 24, 2021
@kavon kavon deleted the actor-initializers branch June 24, 2021 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. concurrency Feature: umbrella label for concurrency language features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants