-
Notifications
You must be signed in to change notification settings - Fork 15
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
add GetAsync
method to ActorRegistry
#33
add GetAsync
method to ActorRegistry
#33
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some design comments
/// <summary> | ||
/// Used to implement "wait for actor" mechanics | ||
/// </summary> | ||
internal sealed class WaitForActorRegistration : IEquatable<WaitForActorRegistration> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Registration handle - we create one of these per-waiter.
/// <remarks> | ||
/// Have to store a collection of <see cref="WaitForActorRegistration"/>s here so each waiter gets its own cancellation token. | ||
/// </remarks> | ||
private readonly ConcurrentDictionary<Type, ImmutableHashSet<WaitForActorRegistration>> _actorWaiters = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contemplated just using a simple lock
here but it became tricky once we started having the CancellationTokenRegistration
delegates also competing to update entries. Opted for the compare-and-swap semantics of the ConcurrentDictionary
instead.
Man, this would have been so simple had I just encapsulated all of this inside an actor, but since we're trying to keep this data structure from being dependent on the
ActorSystem
itself I went with this.
return dict => | ||
{ | ||
// first step during timeout is to remove our registration | ||
var d = (ConcurrentDictionary<Type, ImmutableHashSet<WaitForActorRegistration>>)dict; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use a blend of closures and passing in state objects via parameters here - I think this is probably fine.
@@ -53,6 +101,8 @@ public bool TryRegister(Type key, IActorRef actor, bool overwrite = false) | |||
return _actorRegistrations.TryAdd(key, actor); | |||
else | |||
_actorRegistrations[key] = actor; | |||
|
|||
NotifyWaiters(key, _actorRegistrations[key]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notify any waiters each time we push an update.
src/Akka.Hosting/ActorRegistry.cs
Outdated
var registration = ct.Register(CancelWaiter(key, ct, waitingRegistration), _actorWaiters); | ||
waitingRegistration.CancellationRegistration = registration; | ||
|
||
_actorWaiters.AddOrUpdate(key, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ConcurrentDictionary add semantics make this clean on additions and on removes... I think I'll clean this up for cancellations too.
Leaving this open for comments in the interim - doubles as a design discussion issue and feature implementation. |
So I've finally seen a use-case where this feature is necessary out in the wild: user tried to access an actor in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'll address the merge conflict here |
Enables entries to be added to the `ActorRegistry` later in the future (i.e. actors not started explicitly via Akka.Hosting's delegates - child actors typically) without forcing consumers of the registry to poll for changes.
7b13add
to
0ce2994
Compare
Rebased, but I need to add some additional specs for this feature first @Arkatufus |
Somewhat of an experiment - considering adding this to allow non-Akka.NET consumers to
await
for anIActorRef
to be added to theActorRegistry
in the future. I'll be adding tests for theActorRegistry
overall in a separate PR but I have tested this functionality and it does work. However, I don't want to get users into the habit of explicitly awaiting everything from the registry just because we have it - all actors started via aWithActors
delegate are guaranteed to be created and available in theActorRegistry
before yourIHost.StartAsync
method exits, so you shouldn't need to await theActorRegsitry
when fetching any top-level actors created by the builder itself.However, if you needed a child actor of some kind - one that is started after the
IHost.StartAsync
method completes then this might be useful.Changes
Enables entries to be added to the
ActorRegistry
later in the future (i.e. actors not started explicitly via Akka.Hosting's delegates - child actors typically) without forcing consumers of the registry to poll for changes.Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):