Abstraction of ServiceProvider, Improving Akka.DependencyInjection #4814

SamEmber · 2021-03-06T21:23:05Z

Hi Guys,

I started using the new Akka.DependencyInjection library for MS-DI recently and I have run across a few issues that I would like to address.

The initial problem I faced was the tight coupling of the actor system DI to IServiceProvider. My team have a testing library where we substitute child actors for test probes, previously we did this via AddDependencyResolver however this has now been removed. Currently this coupling is making it very hard to achieve the previous functionality.

I propose a change to the ServiceProvider where instead of passing in an IServiceProvider we instead pass in a IDependencyResolver interface, by default this IDependencyResolver will be a SeriveProviderDependencyResolver that uses the IServiceProvider to resolve any DI dependencies. This will allow others to create their own implementations of the IDependencyResolver for testing.

I want to make it clear that I don't want to change the functionality of the current DI service resolution, its still extremely important that users manage the lifetime of their dependencies and this change will not effect that.

Secondly I have added the ability to resolve actor props from a Type param, this should make it easier to create actors dynamically.

I am very open to any feedback. I have not properly commented the code yet as I want to make sure this is a reasonable proposal from the community before I spent more time to complete this.

Thanks,

Sam

Aaronontheweb · 2021-03-08T15:13:32Z

Thanks @SamEmber - we'll take a look at this soon

Aaronontheweb

So I approve of the concept and the changes, but we need to introduce these in a more backwards-compatible way - along with the changes we're making to the ServiceProviderProps in a separate PR.

I'll help make this happen as part of the 1.4.18 release.

Aaronontheweb · 2021-03-16T14:49:28Z

src/contrib/dependencyinjection/Akka.DependencyInjection/DependencyResolverSetup.cs

+    ///
+    /// The constructor is internal. Please use <see cref="Create"/> to create a new instance.
+    /// </summary>
+    public class DependencyResolverSetup : Setup


It would be far better if we kept the ServiceProvider setup so we can avoid breaking the public API.

Aaronontheweb · 2021-03-17T13:32:23Z

src/contrib/dependencyinjection/Akka.DependencyInjection/DependencyResolver.cs

+    /// Provides users with immediate access to the <see cref="IDependencyResolver"/> bound to
+    /// this <see cref="ActorSystem"/>, if any.
+    /// </summary>
+    public sealed class DependencyResolver : IExtension


We probably shouldn't rename the extension here, for API compatibility reasons

Aaronontheweb · 2021-03-17T13:33:57Z

src/contrib/dependencyinjection/Akka.DependencyInjection/IDependencyResolver.cs

+
+namespace Akka.DependencyInjection
+{
+    public interface IDependencyResolver


So I'm supportive of adding this abstraction, after discussing it with @to11mtm and reviewing some of Mark Seeman's advice here: https://blog.ploeh.dk/2014/05/19/di-friendly-framework/

We've already run into "conforming container" problems with Microsoft.Extensions.DependencyInjection 3.0 vs. 5.0 - so an abstraction like yours would be helpful for users who need to roll-forward before the library itself can.

Aaronontheweb · 2021-03-17T13:38:32Z

src/contrib/dependencyinjection/Akka.DependencyInjection/ServiceProviderProps.cs

+    ///
+    /// Relies on having an active <see cref="IServiceProvider"/> implementation available
+    /// </summary>
+    internal class ServiceProviderProps : Props


We're going to remove this concept altogether in #4853 and replace it with an IIndirectActorProducer, similar to what we did in the original Akka.DI.Core library.

We should probably merge that PR in first once it gets submitted, then rebase this on top of it.

Aaronontheweb · 2021-03-17T13:38:53Z

src/contrib/dependencyinjection/Akka.DependencyInjection/ServiceProviderScope.cs

+        IDependencyResolver Resolver { get; }
+    }
+
+    public class ServiceProviderScope : IResolverScope


Fine with this abstraction too

SamEmber · 2021-03-17T16:04:17Z

Thanks for taking the time to look at this Aaron, these all sound like sensible changes to me. Pleased that my suggestion will be taken into account, please let me know if there is anything I can do to help out.

Aaronontheweb · 2021-03-17T16:07:57Z

Thanks @SamEmber - we'll be discussing these changes at our community standup today: #4856 - my hope is to get them rolled up and out soon

Aaronontheweb · 2021-03-17T17:44:31Z

Need to get this one merged and approved into dev first, then we can integrate this PR: #4858

SamEmber · 2021-04-26T16:01:40Z

Hey @Aaronontheweb, was this ever integrated?

Aaronontheweb · 2021-04-26T16:38:22Z

@SamEmber not yet - was up to my neck in Akka.Cluster issues over the past month. We will probably bump this to 1.4.20

chris-sung · 2021-05-25T01:34:31Z

Secondly I have added the ability to resolve actor props from a Type param, this should make it easier to create actors dynamically.

I thought we need this too. there's no easy way to dynamically create an actor with ServiceProvider and that is my pain point writing test with ServiceProvider

Also, I've been writing using ServiceProvider = Akka.DependencyInjection.ServiceProvider; everywhere I use ServiceProvider due to conflicting class name and is quite a pain, could we mark the old class obsolete and let it sit backward compatible way and moving on?

Aaronontheweb · 2021-05-25T01:59:17Z

Also, I've been writing using ServiceProvider = Akka.DependencyInjection.ServiceProvider; everywhere I use ServiceProvider due to conflicting class name and is quite a pain, could we mark the old class obsolete and let it sit backward compatible way and moving on?

We'll give it our best shot.

Aaronontheweb · 2021-05-25T02:37:12Z

Ok, just introduced my non-breaking API implementation version of this - but someone else (@chris-sung @SamEmber included) will have to review it before we can merge it.

Old APIs should be marked as obsolete
Should offer type-checking (i.e. generics) all the way down the stack up until we reach Props if wanted
New APIs should be painless to switch to
Documentation should feature new APIs
No breaking API changes if we can absolutely help it

Pretty sure I did all of that, but need the review to verify.

Arkatufus

Everything looks fine, I just have a little suggestion for one of the methods.

src/contrib/dependencyinjection/Akka.DependencyInjection/ServiceProviderDependencyResolver.cs

Aaronontheweb

LGTM

SamEmber · 2021-05-26T21:24:17Z

Awesome, this looks great @Aaronontheweb. Thanks for taking the time to make the required changes.

Aaronontheweb · 2021-05-26T21:25:18Z

Crap, looks like I broke the build - that's what I get for attempting to edit a PR using the Github editor :p

Implemented his suggestions

Aaronontheweb · 2021-05-26T21:42:18Z

No problem @SamEmber - thank you for your contribution and sorry it took me so long to get around to this. Didn't take me long to implement the changes we discussed at all, just had a full plate up until this week.

christallire · 2021-05-27T02:41:31Z

Fantastic. Thanks for the quick resolution

Abstraction of ServiceProvider, Improving Akka.DependencyInjection (akkadotnet#4814) * Abstraction of ServiceProvider * introduced non-breaking Akka.DependencyInjection API changes * fixed unit tests / Props bug * fixed up DelegateInjectionSpecs * Added type checking for `Props(Type type, params object[] args)` * fixed non-generic `Props()` method Co-authored-by: Aaron Stannard <aaron@petabridge.com> completed work on ActorPath parsing

* `Span<char>`-ifying ActorPath and Address parsing Abstraction of ServiceProvider, Improving Akka.DependencyInjection (#4814) * Abstraction of ServiceProvider * introduced non-breaking Akka.DependencyInjection API changes * fixed unit tests / Props bug * fixed up DelegateInjectionSpecs * Added type checking for `Props(Type type, params object[] args)` * fixed non-generic `Props()` method Co-authored-by: Aaron Stannard <aaron@petabridge.com> completed work on ActorPath parsing * restore Parse performance to ActorPath.Parse * fixed `ActorPath.ToString()`

SamEmber changed the title ~~Abstraction of ServiceProvider~~ Abstraction of ServiceProvider, Improving Akka.DependencyInjection Mar 6, 2021

Aaronontheweb self-requested a review March 8, 2021 15:13

Aaronontheweb added akka-dependency-injection Akka.DI-related issues enhancement labels Mar 8, 2021

Aaronontheweb requested changes Mar 17, 2021

View reviewed changes

Aaronontheweb modified the milestones: 1.4.18, 1.4.19 Mar 18, 2021

Aaronontheweb self-assigned this Apr 14, 2021

Aaronontheweb modified the milestones: 1.4.19, 1.4.20 Apr 28, 2021

Aaronontheweb modified the milestones: 1.4.20, 1.4.21 May 12, 2021

SamEmber and others added 2 commits May 24, 2021 21:06

Abstraction of ServiceProvider

a5bfb93

introduced non-breaking Akka.DependencyInjection API changes

7815c96

Aaronontheweb force-pushed the 4813-abstraction-of-serviceprovider-in-a branch from ded7717 to 7815c96 Compare May 25, 2021 02:34

Aaronontheweb self-requested a review May 25, 2021 02:37

Aaronontheweb added 3 commits May 24, 2021 21:43

fixed unit tests / Props bug

30dcffb

fixed up DelegateInjectionSpecs

5cde0e2

Merge branch 'dev' into 4813-abstraction-of-serviceprovider-in-a

ea8a396

Arkatufus previously requested changes May 26, 2021

View reviewed changes

src/contrib/dependencyinjection/Akka.DependencyInjection/ServiceProviderDependencyResolver.cs Outdated Show resolved Hide resolved

Added type checking for Props(Type type, params object[] args)

32152fc

Aaronontheweb approved these changes May 26, 2021

View reviewed changes

Aaronontheweb requested a review from Arkatufus May 26, 2021 20:57

fixed non-generic Props() method

902d528

Aaronontheweb enabled auto-merge (squash) May 26, 2021 21:41

Aaronontheweb merged commit c663bc2 into akkadotnet:dev May 26, 2021

Aaronontheweb mentioned this pull request Jun 3, 2021

v1.4.21-beta1 Release Notes #5071

Merged

This was referenced Jun 16, 2021

v1.4.21 final release notes #5095

Merged

v1.4.21 Release #5096

Merged

Aaronontheweb mentioned this pull request Aug 4, 2021

Akka.DependencyInjection: add a TestResolver to use during unit testing? #5167

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstraction of ServiceProvider, Improving Akka.DependencyInjection #4814

Abstraction of ServiceProvider, Improving Akka.DependencyInjection #4814

SamEmber commented Mar 6, 2021

Aaronontheweb commented Mar 8, 2021

Aaronontheweb left a comment

Aaronontheweb Mar 16, 2021

Aaronontheweb Mar 17, 2021

Aaronontheweb Mar 17, 2021

Aaronontheweb Mar 17, 2021

Aaronontheweb Mar 17, 2021

SamEmber commented Mar 17, 2021

Aaronontheweb commented Mar 17, 2021

Aaronontheweb commented Mar 17, 2021

SamEmber commented Apr 26, 2021

Aaronontheweb commented Apr 26, 2021

chris-sung commented May 25, 2021 •

edited

Loading

Aaronontheweb commented May 25, 2021

Aaronontheweb commented May 25, 2021

Arkatufus left a comment

Aaronontheweb left a comment

SamEmber commented May 26, 2021

Aaronontheweb commented May 26, 2021

Aaronontheweb commented May 26, 2021

christallire commented May 27, 2021

Abstraction of ServiceProvider, Improving Akka.DependencyInjection #4814

Abstraction of ServiceProvider, Improving Akka.DependencyInjection #4814

Conversation

SamEmber commented Mar 6, 2021

Aaronontheweb commented Mar 8, 2021

Aaronontheweb left a comment

Choose a reason for hiding this comment

Aaronontheweb Mar 16, 2021

Choose a reason for hiding this comment

Aaronontheweb Mar 17, 2021

Choose a reason for hiding this comment

Aaronontheweb Mar 17, 2021

Choose a reason for hiding this comment

Aaronontheweb Mar 17, 2021

Choose a reason for hiding this comment

Aaronontheweb Mar 17, 2021

Choose a reason for hiding this comment

SamEmber commented Mar 17, 2021

Aaronontheweb commented Mar 17, 2021

Aaronontheweb commented Mar 17, 2021

SamEmber commented Apr 26, 2021

Aaronontheweb commented Apr 26, 2021

chris-sung commented May 25, 2021 • edited Loading

Aaronontheweb commented May 25, 2021

Aaronontheweb commented May 25, 2021

Arkatufus left a comment

Choose a reason for hiding this comment

Aaronontheweb left a comment

Choose a reason for hiding this comment

SamEmber commented May 26, 2021

Aaronontheweb commented May 26, 2021

Aaronontheweb commented May 26, 2021

christallire commented May 27, 2021

chris-sung commented May 25, 2021 •

edited

Loading