dazinator / Dazinator.Extensions

Useful additions to Microsoft.Extensions.DependencyInjection such as Named Services.
Other
38 stars 1 forks source link

Using .AddNamed incompatible with singleton dependees #6

Closed JosXa closed 4 years ago

JosXa commented 4 years ago

.NET Controllers are Scoped dependencies by default, thus the library's normal behavior in AddNamed of adding the resolvers Scoped aswell works. However, when trying to instantiate Singleton dependees (in my case a global Repository), I get:

Dependency Dazinator.Extensions.DependencyInjection.Tests.NamedServiceResolver<Microsoft.Azure.Storage.Blob.CloudBlobContainer> {ReturnDefault} as parameter "blobContainers" reuse CurrentScopeReuse {Lifespan=100} lifespan shorter than its parent's: singleton XX.YY.Repository: XX.YY.IRepository {ReturnDefault} #167
  from ambiently scoped container with {no name}
To turn Off this error, specify the rule with new Container(rules => rules.WithoutThrowIfDependencyHasShorterReuseLifespan()).

I "fixed" this by making my own AddNamedSingleton extension method that adds the resolvers as singletons aswell. Is this the right approach to take?

On another note, I'm confused about two more things:

I'm on Prerelease alpha 27.

dazinator commented 4 years ago

Hi, thanks for checking out this project..

I "fixed" this by making my own AddNamedSingleton extension method that adds the resolvers as singletons aswell. Is this the right approach to take?

My initial thoughts are that this is probably not the approach I would take but its hard to say without knowing a bit more about your application.

Usually you'd be looking to create a DI scope in your application before resolving any services. For example if this was a console app, you'd create a DI scope around the command being executed. In aspnet core, there is a scope per request. For a background service in an aspnet core application, for example one that runs jobs, you'd create a new DI scope per job. Its usually not good to resolve services from the root IServiceProvider because over time, it tracks transient IDisposables, and they will never be disposed of until the root service provider is itself disposed, which doesn't usually happen until your app terminates. This can result in more and more IDisposable instances that were registered as transient building up in the tracking list of the root IServiceProvider - giving the same symptoms as a memory leak. I havent looked too closely at your error yet, but are you resolving your singleton named service from the root IServiceProvider? If so, is there an appropriate point in your application where you could create a DI Scope and resolve from that instead, and then dispose of it when the work is done?

dazinator commented 4 years ago

Closing for now, but feel free to carry on the conversation

JosXa commented 4 years ago

@dazinator Getting back to you soon! In the meantime, what about my other two concerns?

I don't seem to be able to get the NamedServiceResolver from anywhere but the Dazinator.Extensions.DependencyInjection.Tests namespace... Am I doing something wrong? Is the namespace not updated? The GetNamed IServiceCollection is nowhere to be found in the NuGet package, so your README examples kinda break down.

JosXa commented 4 years ago

Aha, I can see you're actively working on it as we speak :) So I'll hold off for now and test your most recent alpha, seems things got added 👍

JosXa commented 4 years ago

Usually you'd be looking to create a DI scope in your application before resolving any services. For example if this was a console app, you'd create a DI scope around the command being executed. In aspnet core, there is a scope per request. For a background service in an aspnet core application, for example one that runs jobs, you'd create a new DI scope per job. Its usually not good to resolve services from the root IServiceProvider because over time, it tracks transient IDisposables, and they will never be disposed of until the root service provider is itself disposed, which doesn't usually happen until your app terminates. This can result in more and more IDisposable instances that were registered as transient building up in the tracking list of the root IServiceProvider - giving the same symptoms as a memory leak. I havent looked too closely at your error yet, but are you resolving your singleton named service from the root IServiceProvider? If so, is there an appropriate point in your application where you could create a DI Scope and resolve from that instead, and then dispose of it when the work is done?

Got it! Some more context: The project is an Azure Functions app, but using fairly straightforward .NET dependency injection. My (irrational) thinking was that "if I don't have a reason for my repositories to be scoped, then they should of course be singletons", which was kind of borrowed from the best practice of using the most constrained public/private/protected/... modifiers as possible. But contrary to my thinking I suppose there is no measurable overhead to making objects scoped and having them be instantiated on every incoming request as opposed to defining them as singletons so they live in memory without re-instantiation, is there? That was another reason why I thought singletons would be better here.

In any case, I guess you taught me one or two things about good practices in DI by enforcing the use of scopes here ;) I suggest that if another person runs into this problem and opens an issue, you might wanna think about some documentation in this regard 😃 Thanks

dazinator commented 4 years ago

Aha, I can see you're actively working on it as we speak :) So I'll hold off for now and test your most recent alpha, seems things got added 👍

In the latest alpha the package name has changed from Dazinator.Extensions.DependencyInjection to Dazinator.Extensions.DependencyInjection.NamedServices - sorry, was a necessary change as I've now also added a child containers feature so decided to split out the packages (Dazinator.Extensions.DependencyInjection.ChildContainers)

The namespace should be corrected now! Thanks for reporting

dazinator commented 4 years ago

I suggest that if another person runs into this problem and opens an issue, you might wanna think about some documentation in this regard

Yeah good shout. If this problem is a common one perhaps there is something I can improve in the design also. Let's see!