Rework actor singleton #458
Conversation
Motivation: The current actor singleton implementation has some shortcomings as documented in #396. Modifications: - Support for proxy-only mode (i.e., without specifying behavior) - Proper implementation for Actorables. Before we can only obtain `Actorable` that is singleton, now we can actually define an `Actorable` to be singleton. Result: Resolves #396.
Tests/ActorSingletonPluginTests/ActorSingletonPluginTests.swift
Outdated
Show resolved
Hide resolved
| ref3.tell(.greet(name: "Charlie", _replyTo: replyProbe3.ref)) | ||
|
|
||
| // Spawn the singleton on `fourth` | ||
| _ = try fourth.singleton.ref(of: GreeterSingleton.Message.self, settings: singletonSettings, GreeterSingleton.makeBehavior(instance: GreeterSingleton("Hello-4"))) |
There was a problem hiding this comment.
We have to do this because of the changes. Is this annoying?
There was a problem hiding this comment.
See API idea above, perhaps that would be the most clean...?
So here we could keep just doing ref() without passing the behavior
There was a problem hiding this comment.
here we would need to call host actually to spawn the singleton because fourth will become the new leader. before this was done in system setup. just a trade-off between different implementations i guess.
| /// - SeeAlso: The `ActorSingleton` mechanism is conceptually similar to Erlang/OTP's <a href="http://erlang.org/doc/design_principles/distributed_applications.html">`DistributedApplication`</a>, | ||
| /// and <a href="https://doc.akka.io/docs/akka/current/cluster-singleton.html">`ClusterSingleton` in Akka</a>. | ||
| public final class ActorSingletonPlugin { | ||
| private var singletons: [String: BoxedActorSingleton] = [:] |
There was a problem hiding this comment.
Worry about this. This is mutable. Safe?
There was a problem hiding this comment.
Forgot to comment here -- yes this is not safe indeed...
We need to put a lock around accessing these and host() calls etc...
There was a problem hiding this comment.
How I'd really want to write all this is (but it's too annoying I think, until we have async/await things):
let ref = await plugin.ref()
since then all of the mutable state can actually be put inside an actor, and we'd implement by "asking the plugin actor about the ref" rather than doing it here where we are not safe from concurrent access...
We could do this today but it gets too annoying, since it'd be like -> AskResponse<ActorRef<...>> which I think is annoying... :-(
There was a problem hiding this comment.
Do we use NIO's lock in this project or something else?
ktoso
left a comment
ktoso
left a comment
There was a problem hiding this comment.
One API discussion I think... about if we should do ref()/actor() ONLY or ref()/actor() AND host()
| ref3.tell(.greet(name: "Charlie", _replyTo: replyProbe3.ref)) | ||
|
|
||
| // Spawn the singleton on `fourth` | ||
| _ = try fourth.singleton.ref(of: GreeterSingleton.Message.self, settings: singletonSettings, GreeterSingleton.makeBehavior(instance: GreeterSingleton("Hello-4"))) |
There was a problem hiding this comment.
See API idea above, perhaps that would be the most clean...?
So here we could keep just doing ref() without passing the behavior
|
Overall great though :-) Time to also document it some more with those semantics perhaps as well...? |
Sure. I believe you started some docs on it already if I remember correctly? |
It's just a placeholder really, nothing really documented in there, see |
okie, i will update it in follow-up PR |
|
Yay, thanks a ton :) No rush about it though 👍 |
Tests/ActorSingletonPluginTests/ActorSingletonPluginTests.swift
Outdated
Show resolved
Hide resolved
Tests/ActorSingletonPluginTests/ActorSingletonPluginClusteredTests.swift
Outdated
Show resolved
Hide resolved
|
Failure is #463 |
|
New (unrelated) failure: #464 |
|
test this please |
|
Thanks for the failed tickets, simple ones to follow up on. This looks good! |
2 participants
Motivation:
The current actor singleton implementation has some shortcomings as documented in #396.
Modifications:
Actorablethat is singleton, now we can actually define anActorableto be singleton.Result:
Resolves #396.