[Concurrency] Distributed actor's unownedExecutor should be optional #64499
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ktoso
commented
Mar 21, 2023
ktoso
added
concurrency
ktoso
commented
Mar 21, 2023
| @@ -255,6 +260,29 @@ public protocol DistributedActor: AnyActor, Identifiable, Hashable | |||
| /// be introduced when not strictly required. Visible side effects | |||
| /// are therefore strongly discouraged within this property. | |||
| @available(SwiftStdlib 5.9, *) | |||
| nonisolated var localUnownedExecutor: UnownedSerialExecutor? { get } | |||
There was a problem hiding this comment.
This is the core of this PR, everything else is just in order to enable it.
ktoso
commented
Mar 21, 2023
| @@ -11,7 +11,7 @@ | |||
| //===----------------------------------------------------------------------===// | |||
|
|
|||
| import Swift | |||
| import _Concurrency | |||
| @_spi(ConcurrencyExecutors) import _Concurrency | |||
There was a problem hiding this comment.
TODO: cant make use of it after all because we need the executor initializer from an inlinable function.
ktoso
force-pushed
the
wip-unownedExecutor-optional-on-da
branch
from
b668f95 to
dfc373a
Compare
|
@swift-ci please smoke test |
|
This should work fine, though I'm working to get rid of the |
|
This weird failure again: |
|
@swift-ci please clean smoke test macOS |
ktoso
commented
Mar 21, 2023
| } | ||
|
|
||
| @available(SwiftStdlib 5.9, *) | ||
| public var _unwrapLocalUnownedExecutor: UnownedSerialExecutor { |
There was a problem hiding this comment.
Or we could make this a free function... perhaps easier than a full impl in SIL
|
@swift-ci please clean smoke test |
ktoso
commented
Mar 21, 2023
| @@ -255,7 +260,7 @@ public protocol DistributedActor: AnyActor, Identifiable, Hashable | |||
| /// be introduced when not strictly required. Visible side effects | |||
| /// are therefore strongly discouraged within this property. | |||
| @available(SwiftStdlib 5.9, *) | |||
| nonisolated var unownedExecutor: UnownedSerialExecutor { get } | |||
There was a problem hiding this comment.
This requirement has never shipped and cannot have been used by anyone yet, so making sure we have the right signature/requirement in the API/ABI for distributed actors by merging this now.
|
rdar://84054793 |
etcwilde
pushed a commit
to etcwilde/swift
that referenced
this pull request
Apr 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements the Swift Evolution feedback that it'd be better to implement this customization point properly with an
UnownedSerialExecutor?which makes it more logical (and possible!) to implement correctly.@kavon's question https://forums.swift.org/t/se-0392-custom-actor-executors/63599/13
and my writeup: https://forums.swift.org/t/se-0392-custom-actor-executors/63599/16
Note that we never shipped distributed actors that had an
unownedExecutorsynthesized, so we don't have to be compatible with past implementations which had it configurable or synthesized -- it never was.The tests show various use-cases.
I took care to not break compatibility with previous implementations and we still detect "is default actor" using the old synthesized method. Though previously ALL distributed actors were default actors, even if you had provided the method yourself.
I believe this yields the right semantics but would really really ask for a review if I got the default implementation of
unownedExecutorright -- the availability dance there I was unsure of.