Make the actor transport an associated type of the DistributedActor protocol #39985

DougGregor · 2021-10-29T22:50:26Z

Eliminate the required use of existentials in distributed actors by
introducing the Transport associated type into the
DistributedActor protocol. Each distributed actor has a known
(concrete) actor transport type, reducing storage requirements and
transport dynamism when it isn't needed.

Distributed actors can manually specify their Transport associated
type or pick up a default by looking for a type named
DefaultActorTransport. A library that vends an actor transport can
make create a public typealias DefaultActorTransport referring to
its transport, so importing that library and defining a distributed
actor will use that library's transport.

Introduce a type-erased AnyActorTransport type to provide an
explicitly dynamic actor transport. This is still an important option,
e.g., for cases where one wants to be able to dynamically change the
transport for testing or different kinds of deployment. For now, we
default to this transport in the library (via DefaultActorTransport),
but we may very well want to eliminate this because it will be
ambiguous with client libraries that vend their own
DefaultActorTransport.

stdlib/public/Distributed/DistributedActor.swift

ktoso · 2021-11-01T03:10:21Z

lib/Sema/DerivedConformanceDistributedActor.cpp

+
+  // Return the default transport type.
+  return defaultTransportTypeDecl->getDeclaredInterfaceType();
+}


So... this means that we look in the current module for DefaultActorTransport, and that dictates the type of the Transport property... I remain worried about this somehow... What happens if there is no default one defined, you'd suggest that each actor must explicitly define the typealias, right?

// no default transport distributed actor X {} // error: no DefaultActorTransport, provide `typealias Transport` pls

typealias DefaultActorTransport = ClusterTransport // example distributed actor X {} // ok X(transport: SomeOtherTransport()) // error, expected ClusterTransport

right?

This piece I like, that makes sense for applications which will usually have one transport.

I was hoping we can derive it from the init declarations:

distributed actor A { init(t: MyTransport) {} // therefore: typealias Transport = MyTransport }

and also once all transport types must "match":

distributed actor A { init(t: MyTransport) {} init(number: Int, other: OtherTransport) {} // ERROR: distributed actor initializers must accept // the same transport type in all initializers

The DefaultActorTransport we could still keep though! It's really be the default, but this init declaration dance I wonder if would not be worth it...? Maybe let's ignore for now hm...

As I've noted in my newly-written comment, I expect that any library providing a transport would provide a DefaultActorTransport typealias pointing at that transport, so importing the library + defining a distributed actor would use that transport. Inference from init(transport:...) would also be possible.

ktoso · 2021-11-01T03:12:33Z

stdlib/public/Distributed/DistributedActor.swift

@@ -100,9 +103,9 @@ extension CodingUserInfoKey {
 @available(SwiftStdlib 5.5, *)
 extension DistributedActor {
  nonisolated public init(from decoder: Decoder) throws {
-    guard let transport = decoder.userInfo[.actorTransportKey] as? ActorTransport else {
+    guard let transport = decoder.userInfo[.actorTransportKey] as? Transport else {


Hmmm... I see, this is more restrictive but I think it'll be fine -- realistically people should not be mixing completely random transports and sending an actor from one transport to another transport, I suppose...

Oh, that's a good point, this might be a bug! We probably need AnyActorTransport to be Codable over top of the underlying transport, so we can decide it.

Nah, transports are not intended to be Codable; This change seems fine.

I was thinking about a weird/obscure use-case that to be honest we don't have to support IMHO: "mixing transports". It would be very complicated and I don't think we really need it in any real use-case to be honest.

Here is the imagined problem:

you could receive a websocket://12.3.4:####/greeter identified actor... the WebSocketTransport decodes this fine...

you are ALSO part of the cluster, and send the same ref to some cluster actor...

encoding just works, it's Codable by the websocket address...

the recipient system, attempts to decode it

puts self into .actorTransportKey, that's the Cluster transport

that as? will fail, since this is Cluster and not WebSocket transport after all

To be honest this is likely good, you can't just randomly share refs like that as the connections and forwarding take a role in all that too... I think this is okey to keep as such runtime `"failed to decode websocket:/// actor using Cluster transport" it makes total sense and is easily fixed by developers with a "forwarder" actor if they need such chain of messages.

Why all of this doesn't really matter:

the connection of course didn't "move" with the actor as we sent it over like that...

so it does not make sense for that other system to resolve the "actual connection backed actor", since it can't, the connection is on the first node -- that accepted that websocket actor.

So yeah, such weird thing could happen, but honestly I think it's a weird edge case, and by means of our transports being explicit in the types -- we'd see that "huh, why am I sending this websocket actor to the cluster, does this make sense?" One can spin up a simple "forwarder" actor if we needed to support that.

ktoso

Overall looking good, definitely nice to have the typealias explicitly like that; while still being able to support existential if we need to hmmm

…rotocol. Eliminate the required use of existentials in distributed actors by introducing the `Transport` associated type into the `DistributedActor` protocol. Each distributed actor has a known (concrete) actor transport type, reducing storage requirements and transport dynamism when it isn't needed. Distributed actors can manually specify their `Transport` associated type or pick up a default by looking for a type named `DefaultActorTransport`. A library that vends an actor transport can make create a public typealias `DefaultActorTransport` referring to its transport, so importing that library and defining a distributed actor will use that library's transport. Introduce a type-erased `AnyActorTransport` type to provide an explicitly dynamic actor transport. This is still an important option, e.g., for cases where one wants to be able to dynamically change the transport for testing or different kinds of deployment. For now, we default to this transport in the library (via `DefaultActorTransport`), but we may very well want to eliminate this because it will be ambiguous with client libraries that vend their own `DefaultActorTransport`.

DougGregor · 2021-11-02T05:39:12Z

@swift-ci please test

DougGregor · 2021-11-02T05:39:19Z

@swift-ci please build toolchain

swift-ci · 2021-11-02T06:00:43Z

Build failed
Swift Test Linux Platform
Git Sha - c2ba06c

swift-ci · 2021-11-02T07:31:34Z

Linux Toolchain (Ubuntu 16.04)
Download Toolchain
Git Sha - c2ba06c

Install command
tar zxf swift-PR-39985-723-ubuntu16.04.tar.gz
More info

swift-ci · 2021-11-02T07:35:08Z

Build failed
Swift Test OS X Platform
Git Sha - c2ba06c

ktoso · 2021-11-02T12:37:42Z

Could be real issues?

DougGregor · 2021-11-03T04:11:45Z

@swift-ci please test

ktoso

Fantastic, LGTM!

DougGregor · 2021-11-03T16:18:24Z

I have a generalization of this PR that also de-existential-ifies the actor identity. I assume we want that on top of this, but didn't want to mess with this directly, so it's over at #40033.

xedin · 2021-11-03T18:05:14Z

lib/SILGen/SILGenDistributed.cpp

+    VarDecl *property) {
+  Type formalType = SGF.F.mapTypeIntoContext(property->getInterfaceType());
+  SILType loweredType = SGF.getLoweredType(formalType).getAddressType();
+#if false


Is this just a left-over from debugging?

Whoop, I'll remove that in a follow up

ktoso

Merging and I'll remove the #if false and the Default typealias in a followup now.

DougGregor marked this pull request as draft October 29, 2021 22:50

ktoso added the distributed Feature → concurrency: distributed actor label Nov 1, 2021

ktoso reviewed Nov 1, 2021

View reviewed changes

stdlib/public/Distributed/DistributedActor.swift Show resolved Hide resolved

ktoso reviewed Nov 1, 2021

View reviewed changes

DougGregor force-pushed the actor-transport-associated-type branch from 2218c04 to 0e257f1 Compare November 2, 2021 05:37

DougGregor force-pushed the actor-transport-associated-type branch from 0e257f1 to c2ba06c Compare November 2, 2021 05:37

DougGregor changed the title ~~WIP make actor transport an associated type of distributed actors~~ Make the actor transport an associated type of the DistributedActor protocol Nov 2, 2021

Fix a few tests for the actor transport associated type

f83775b

ktoso approved these changes Nov 3, 2021

View reviewed changes

ktoso marked this pull request as ready for review November 3, 2021 12:27

xedin reviewed Nov 3, 2021

View reviewed changes

ktoso approved these changes Nov 4, 2021

View reviewed changes

ktoso merged commit 37d46fa into swiftlang:main Nov 4, 2021

ktoso mentioned this pull request Nov 4, 2021

[Distributed] remove public default AnyActorTransport predefined typealias #40049

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the actor transport an associated type of the DistributedActor protocol #39985

Make the actor transport an associated type of the DistributedActor protocol #39985

DougGregor commented Oct 29, 2021 •

edited

Loading

ktoso Nov 1, 2021 •

edited

Loading

DougGregor Nov 2, 2021

ktoso Nov 1, 2021

DougGregor Nov 2, 2021

ktoso Nov 2, 2021 •

edited

Loading

ktoso left a comment

DougGregor commented Nov 2, 2021

DougGregor commented Nov 2, 2021

swift-ci commented Nov 2, 2021

swift-ci commented Nov 2, 2021

swift-ci commented Nov 2, 2021

ktoso commented Nov 2, 2021

DougGregor commented Nov 3, 2021

ktoso left a comment

DougGregor commented Nov 3, 2021

xedin Nov 3, 2021

ktoso Nov 4, 2021

ktoso left a comment

Make the actor transport an associated type of the DistributedActor protocol #39985

Make the actor transport an associated type of the DistributedActor protocol #39985

Conversation

DougGregor commented Oct 29, 2021 • edited Loading

ktoso Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

DougGregor Nov 2, 2021

Choose a reason for hiding this comment

ktoso Nov 1, 2021

Choose a reason for hiding this comment

DougGregor Nov 2, 2021

Choose a reason for hiding this comment

ktoso Nov 2, 2021 • edited Loading

Choose a reason for hiding this comment

ktoso left a comment

Choose a reason for hiding this comment

DougGregor commented Nov 2, 2021

DougGregor commented Nov 2, 2021

swift-ci commented Nov 2, 2021

swift-ci commented Nov 2, 2021

swift-ci commented Nov 2, 2021

ktoso commented Nov 2, 2021

DougGregor commented Nov 3, 2021

ktoso left a comment

Choose a reason for hiding this comment

DougGregor commented Nov 3, 2021

xedin Nov 3, 2021

Choose a reason for hiding this comment

ktoso Nov 4, 2021

Choose a reason for hiding this comment

ktoso left a comment

Choose a reason for hiding this comment

DougGregor commented Oct 29, 2021 •

edited

Loading

ktoso Nov 1, 2021 •

edited

Loading

ktoso Nov 2, 2021 •

edited

Loading