-
Notifications
You must be signed in to change notification settings - Fork 16
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 GetAync
method to IRequiredActor<TActor>
to resolve issues where actor is not available upon injection (i.e. BackgroundService
s)
#264
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.
Reviewed my own code
@@ -17,6 +17,7 @@ | |||
</ItemGroup> | |||
|
|||
<ItemGroup> | |||
<ProjectReference Include="..\Akka.Hosting.TestKit\Akka.Hosting.TestKit.csproj" /> |
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.
Needed to add the Hosting.TestKit in order to test the BackgroundService
issue with #208
|
||
// assert | ||
|
||
// workaround for https://github.com/akkadotnet/Akka.Hosting/issues/265 |
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.
This is the reproduction for #265
|
||
public MyBackgroundService(IRequiredActor<TestActorKey> requiredActor) | ||
{ | ||
_testActor = requiredActor; |
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.
So we tried to resolve the IRequiredActor<TActor>.ActorRef
here, like users did in #208, but it won't work - if we block the constructor of a BackgroundService
here it'll stop the AkkaService
and another services from starting. It's too fragile of a fix for end-users, so instead we're going with throwing a clearer exception when an ActorRegistry
miss occurs and offer an additional API on the IRequiredActor<TActor>
interface to address it.
/// </summary> | ||
/// <param name="cancellationToken">Optional cancellation token.</param> | ||
/// <returns>A Task that will return the <see cref="IActorRef"/> using the given key.</returns> | ||
Task<IActorRef> GetAsync(CancellationToken cancellationToken = default); |
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.
This is the correct fix - offer a method on the IRequiredActor<TActor>
interface that uses the ActorRegistry.GetAsync
under the covers. Users will need to call this if they fall under the #208 edge case.
} | ||
|
||
|
||
throw new MissingActorRegistryEntryException( |
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.
Try our best to give the users a clear signal as to why this failed and what they should do instead. We can't really solve the problem for them and the normal, sync ActorRef
property works for 99.9% of cases so we don't want to remove that just to support an edge case.
GetAync
method to IRequiredActor<TActor>
to resolve issues where actor is not available upon injection (i.e. BackgroundService
s)
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
Changes
Fixes #208 - working on fix to ensure that other
BackgroundService
/IHostedService
implementations can still depend onIRequiredActor<TActor>
without getting blown up by MSFT.EXT.DI while it waits for theAkkaService
to start.Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):