Revisit tracking transient services for disposal #456
Comments
|
Oh, I just noticed this is being "thought about" for a future version. 👍 (#433 (comment)) I'll keep this open for tracking this specific issue. |
|
Watching this thread with interest - spent a long time on this topic over the years :-)
This statement may not be the best starting point. It's one of those cases where "the way you've always done it" seems like the most logical way, whichever camp you're in. The alternative point of view goes something like - how do you do release resources deterministically in a loosely-coupled system if your IoC container can't/won't do it? There are good arguments for either position; I can't see any value coming from a debate on the merits of one tactic or the other. I know that's not the intention of the ticket, but perhaps this will be easier to work through if we frame the discussion more carefully as:
Or something along those lines. If that sounds close to the mark, getting a clear write-up of what the framework needs in terms of resource management, and what the desired user experience is, would be great next-steps. |
I'm not sure I see how you could properly resolve transient instances without doing a call to When you do the call to |
|
A compromise might be to only track transient disposable objects in scopes outside of the root scope or make it a setting like scope validation. |
|
@davidfowl Are you suggesting that the child scopes should act differently from parent scope? I happen to fall in line with how Autofac handles this by default transients are tracked unless otherwise configured. This seems to give the best of both worlds auto disposal if you want it, turn it off when you don't. One of the upsides for transient tracking by the container is that the interface you request doesn't have to implement IDisposable. For example the interface generated for WCF proxies don't implement IDisposable even though the generated proxy class implements the IDisposable interface. |
|
Commenting here just to say I'm also affected by memory leaks due to this issue, where EF contexts are never released from memory when added using AddTransient. Preferences aside, I'm not sure how the provider would track any disposables without a scope. It simply holds the instances forever and never knows when to dispose them. |
|
@nvivo Side question - why are you registering EF contexts as transient? Shouldn't they be scoped? |
|
@nvivo not just that but why not just make a scoped service provider and resolve from that. If we do anything here it would be adding an options to turn the tracking off. There's a question as to whether we should turn it off just for scopes or all up. We haven't made a decision to do anything just yet though as turning it off by default would be a massive breaking change. |
|
@davidfowl I can see where you are coming from with the idea of having the root scope act differently from a child scope but that to seems more confusing. Maybe it's just me but some classes dispose their transient dependencies while others don't all because they are resolved from a different scope sounds scary (what if you decided to start resolving it in a different scope then you have to change code?) Turning it on/off for a whole container instance seems like a reasonable compromise between the two sides as @nblumhardt said there are developers that want transient track and some that don't. Maybe to help address these "Memory leaks" it's a matter of disseminating the pattern to create a scoped service provider and resolve from it (maybe it's out there and I haven't run into it as I haven't looked). |
|
@jbogard I'm using the DI container with akka actors inside a windows service. There is no natural scope to tie it to. Some actors live forever, and I sometimes want to release the entire set I was working with and get a clean context inside the same actor. For long running services, it's much simpler to have transient scopes. @davidfowl I guess I can workaround with scopes. It's not as clean though. Correct me if I'm wrong, but I'd have to change from: using (var ctx = provider.GetRequiredService<DataContext>()) {
...
}to: using (var scope = rootProvider.CreateScope()) {
var ctx = scope.ServiceProvider.GetRequiredService<DataContext>();
...
}As for the breaking change, unless I'm missing something, if you just stop tracking disposables at the root level, the worst thing that could happen is that sometimes resources won't be freed. Currently what happens is that resources are never freed, doesn't matter if we call If changes were made to stop tracking disposables at the root level, ASP.NET shouldn't be affected as it's always scoped to requests. Or am I missing something? Anyway, I strongly believe that the leak shouldn't happen. Either it stops tracking the disposable at the root level or it prevents us from instantiating transient disposables at the root level. The first option being much simpler and cleaner. |
|
@ipjohnson I agree a documented pattern would be a good idea, but I don't think it would solve the actual problem. It's not like the leak happens on a very obscure case. |
|
@nvivo I'm confused how exactly does david's suggestion not solve the problem? To david's point if you stop disposing transients then existing code that depends on this really will have a memory leak not just trapped transients like what you have now. |
|
@ipjohnson I guess we're discussing different things or you didn't notice the current problem. First: scopes actually solve the problem, but create another. My code suddenly needs to be aware it needs a different scope and hard code it. The issue IMO is that scopes should be something handled by the infrastructure, not hardcoded just to make disposables work. Disposables already have an interface for that. In my case, a windows service with long lived objects doesn't have any natural scope. Second, you said:
This is exactly what is happening today. It's not a matter if I want the framework to handle it or not. Currently, using the container as intended, you can get instances that will never be released, by design. This shouldn't happen. We cannot say the root provider is exactly the same as a scoped provider because scoped providers can be disposed, but the root provider is a singleton, it makes no sense to destroy and create another. So, today the same interface provides different behavior. An alternative to all of this is a |
|
@nvivo I guess I look at it as multiple DI containers (autofac, this project, and grace to name three) do transient tracking and consider it a feature. The container was designed to operate this way and is inherently not a bug (you can disagree with the feature but it's not a bug). "Currently, using the container as intended, you can get instances that will never be released, by design. " Technically this is not true, the instance will most certainly be disposed when the scope that it's resolved from disposes. You are just choosing to resolve from the root scope and getting a behavior you didn't expect. Also it seems like one of the creators of the container is specifically telling you in this thread it was never intended for you to resolve disposable transients from the root scope. To your point today you are correct to do what you want to do you have to make your class a bit smarter but is this really the common use case for this container? It seems to me the common use case is in a scope per request situation in AspNetCore, not calling GetService directly from the root scope to resolve disposable transients. I'm all for making it a feature that can be turned on/off but the idea that transient tracking is completely wrong and needs to be removed completely seems to ignore a number of other very valid opinions and use cases. |
|
@ipjohnson Just to clarify, I didn't said the idea of transient tracking is wrong. What is wrong is to track the transient disposables in the root container and not allow me to release them. Since it's not possible to dispose the container, it's just holding references forever.
This is unrealistic. In real world, you cannot destroy the root container without restarting your application. And even in ASP.NET you can access the root container and create memory leaks. Honestly, I think creating a scope in code just to dispose a service is a bad solution. This is not making the class smarter, it's just adding boilerplace. If I need to remember to create and dispose a new scope, I can as well remember to dispose the instance directly. Now, again, I'm not against tracking disposables or scopes. I'm just saying it's not enough. Those are two ways to handle lifecycles. Scopes work well with requests and such, and tracking works there as well. But another simple and more natural way is to call So, after considerations, I vote to make IServiceProvider as: public interface IServiceProvider
{
object GetService(Type serviceType);
void DisposeService(object service);
}
@davidfowl was this considered? |
That's not correct. The root container does get disposed, it just gets disposed when the application is being torn down or if somebody disposes the host manually.
Only a single piece of code has to be aware of it (like all code directly calling GetService) the composition root. If you have it sprinkled all over then something else is wrong.
They are freed, on dispose.
No and we wouldn't make a breaking API change like the one in the sample to do a feature like this. You did give me an idea though, what if we just have a marker interface that you can implement on your type to signal that it shouldn't be tracked for disposal. Basically a statement that you will take ownership of the lifetime of this object even though it is disposable. In this case, the container won't track it and you can dispose to your heart's content 😄 . One downside of this approach is that this will only work for our container, others won't respect this marker as it isn't expressed in the contracts we expose for 3rd party containers. |
|
@davidfowl adding an interface to every IDisposable is not possible. Sometimes disposables are not subclasses. The only thing that would really work is to somehow signal the framework that an object may be disposed at a specific time. Scopes work well if they exist (like requests, parent object lifecycles, etc). If there is no scopes, adding scopes manually is a really dirty workaround. In any case, my I understand from this is that the container is not supposed or not made to be used outside of asp.net, as it depends on scopes to prevent leaks. This is confusing, as the package is published under "Microsoft.Extensions", and not "AspNet.Extensions". It would be nice to signal somehow this package is made to work only with ASP.NET. I still think this is a HUGE problem to allow people to fall into this trap without warning. I'd rather have an exception thrown trying to get a disposable from the root container then just having services blowing up in production. This is not something that can be prevented with a comment hidden in a github issue. For now, my workaround with this component is to never get disposables from the service provider directly. Instead, I'd create a factory to be injected and then I can control the lifecycle myself. |
If akka has no natural scopes then you're kinda SOL in general no? You can't constructor inject anything as it'll be a singleton or live for however long an akka actor lives for. I'd imagine they have a factory that you can replace to take over actor construction and disposal? That's the sorta place where you'd make a scope.
I think having an option to turn it off would be a fine solution. As for the behavior in ASP.NET, we probably wouldn't change the default but would make it easier to configure the default container options within ASP.NET itself. |
|
Just on the Akka.NET situation; Akka.NET + Autofac does handle scoping as far as I'm aware: There was a discussion about this on the Akka.NET repository in 2015 that led to a refactor and some improved implementation. Is it possible the Microsoft.Extensions.DependencyInjection/Akka.NET integration just needs some tweaks to properly release scopes? (I haven't used Akka.NET myself, so apologies if I'm overlooking something here.) |
|
@davidfowl, @nblumhardt Yes, there is an interface that acts as a factory for actors when using DI. After this discussion, I was able to create an scope for each actor and release when the actor is destroyed. This helped with half of the problems. But there I cases where I need multiple instances of the object over the lifecycle of the actor, as the actor may remain there forever. In these cases, instead of injecting the service, I injected the using (var ctx = provider.GetService<DbContext>()) { ... }which I thought should free the resources but wasn't because the provider kept that instance forever. So maybe it was my mistake to inject IServiceProvider this way. In the end, instead of creating a new scope there, I found it simpler to inject my own factory in the constructor, so I can control the lifecycle. |
|
@nblumhardt does autofac basically give you control over this at the callsite with |
|
You can resolve an |
|
Just to make explicit what @tillig implies - it's for the whole sub-graph behind the So yes, |
|
Closing because the trade-offs of implementing this don't seem worthwhile. |
|
@Eilon What are those trade-offs? |
|
|
As I read @davidfowl's response, all three trade-offs are breaking changes. They break all adapters of the Conforming Container. |
|
They actually don't. If we added a separate registration API for the build in container separate from |
|
@davidfowl from a client recommendation perspective, do you look at users going with the built-in container until they meet limitations like the above, then switch over to a 3rd party container? Or just go with 3rd-party from the get-go? |
@Dima4ka If you're building a service that remains in memory forever and you want to create and destroy many contexts during this period, scopes are not necessary. The rule is: if you use the DI to create the object, then you MUST rely on DI to release it when the root provider or associated scope is released. If you want to release any object yourself, don't let the DI create that object. Then, I found by experience that if you're letting A simpler solution can be achieved by registering a factory instead: // this can be used inside asp.net, whatever
services.AddScoped(ctx => new DataContext(connectionString));
// this can be used everywhere else
services.AddSingleton(ctx => new Func<DataContext>(() => new DataContext(connectionString)));Then, if you want to create and dispose your own contexts, you can just request the factory instead: class SomeService {
Func<DataContext> contextFactory;
public SomeService(Func<DataContext> contextFactory) {
this.contextFactory = contextFactory;
}
public void DoSomethind() {
using (var ctx = contextFactory()) {
// doesn't matter what DI wants, your object will be disposed here
}
}
}Alternatively, create your own factory class that can build contexts, connections for dapper, etc, and register it as a singleton. I think it's cleaner than requesting public class DbFactory {
string _cn;
public DbFactory(string connectionString) {
_cn = connectionString;
}
public DataContext CreateContext() {
return new DataConext(_cn);
}
public DbConnection CreateConnection() {
return new SqlConnection(_cn);
}
}This makes it easier to add additional methods for other libraries like Dapper for example. The factory pattern works well for any other disposables you may want to control. IMHO, the team is still overlooking this problem because they're focused on ASP.NET. But in the age of docker, people are building more and more services with this DI component, because it's there. I don't think there should be any change to the component anymore, but there should be a recognition that this is a very common use case, there should be a clear section in the docs for "Using DI in long running services/outside of asp.net" with some patterns like this and also tips for where to create scopes when actually necessary. |
|
I about agree and disagree with that assessment. I think scopes are basically what you want. In the background worker cases, your code is the compostion root so it needs to get its hands dirty with IServiceProvider if you want to get services. There's no intrinsic scope or request so you have to make it up and scopes are perfect for this. |
|
@davidfowl I don't intend to create a long discussion about this, but with all due respect, I believe you and people building containers are biased on this issue. You built a library, you decided the features. It's easy to say "it works, it's what you should be doing" when you're not using it daily for that purpose. You say:
I disagree completely. If there is no intrinsic scope, you don't have to make anything up. The goal is to dispose an object, not to make your code use a new convention just for the sake of it. The feature to dispose objects is already in the language. There is zero reason to invent one just to adhere to arbitrary principles when there is zero gains. We're talking going from: using (var context = createContext())
{ ... }to using (var scope = serviceProvider.CreateScope())
using (var context = scope.ServiceProvider.GetRequiredService<SomeContext>())
{ ... }Just to put scopes to use. If this was some hidden code you'd type once, I'd agree. But we're talking about adding this complexity hundreds of times over and over in the middle of business logic for no good reason. |
|
The golden rule about creating object instances is "If you create it, you destroy it" If you need to need to control the lifetime, why just don't inject a Func to create the instance? We could of course discuss whether IDisposable is a leaky abstraction, but we see this all over the place in the BCL. I really don't see why this has to be so complicated. |
|
What if you wrap the logic up into an extension method and generic wrapper class, something like this. |
|
@ipjohnson How is this better than just having a factory? |
|
@seesharper exactly my point. |
|
@nvivo it's generic so you don't need to implement a factory per type. That said factories are perfectly acceptable I was just suggesting something that uses the DI container. |
|
@ipjohnson |
|
@khellang I was responding to nivivo question is relation to Factory like the example he was showing here making no comparison to the |
|
@khellang Also if we're getting technical in both factory examples the DataContext is instantiated by hand rather than resolving it from the container definition That said the factory example works great for scenarios like this and will be more performant than going the scope route. I was just throwing out an option if people wanted to go the scoped route. |
|
@ipjohnson It was just an alternative for his specific case. I referred to |
|
@nvivo I retract my suggestion, have at it 👍 |
|
@ipjohnson for some reason people think I'm angry when I say things. I'm not. I'm not dismissing your suggestion either, I'm just explaining my point, which seems to be misunderstood as @davidfowl disagreed with me, but agreed with @seesharper that said exactly the same thing. So, I'm probably not explaining my ideas right. |
|
As with most online discussions, I'm not sure there's as much disagreement as there seems :-)
There's not really much contradiction here. The func does work fine in the example. As soon as we move from the example to something general, e.g. instead of As interesting as this stuff is, we should probably let this closed thread close ;-) |
|
@nvivo no worries I didn't take it that way. There are many ways to solve this issue and I would be the last to suggest that mine is the only way or even the best way, merely a way. |
|
I also got bitten by this. Was happy enough to have DbContext created 10 times a second so found out very fast but it really feels dangerous. Bug or not it doesn't seem to be covered well by documentation atm and the default approach to transients isn't logically expected IMO. |
Tracking transient objects for disposal is a pretty bad practice and will almost always lead to memory leaks. Even if it's tracked in a scoped provider, it'll be kept around for longer than it needs to, at a minimum.
This is why containers like SimpleInjector and StructureMap (and probably others as well) don't track transient disposables. SimpleInjector even has a diagnostic warning about this.
In order to comply with the MS.Ext.DI specs, StructureMap had to introduce a way to opt-in to track transients for disposal, which sucks.
I've already seen several reports on severe memory leaks because of this in the wild. Here's the latest example: https://gitter.im/aspnet/Home?at=57ff6e91891a530163074f94
I also suspect there are other, less severe memory leaks around, that people haven't noticed yet or worked around.
What can be done about this? The least breaking change I can see is add a
Releasemethod somewhere that can be used to explicitly evict specific instances from tracking, but ideally, this behavior should be removed altogether and throw if a transient disposable is registered.// @davidfowl @pakrym @tillig @alexmg @nblumhardt @seesharper @jeremydmiller @dotnetjunkie
The text was updated successfully, but these errors were encountered: