DI: Allow IDisposable services to get garbage collected as soon as they are disposed

[divega]

This idea came up in a discussion with @danroth27, @rynowak and @ajcvickers about using EF Core in Server-side Blazor applications, and specifically about how creating a large (theoretically unbounded) number of transient DbContext instances from DI would cause memory leaks with long living DI scopes.

DI scopes maintain a list of disposable service instances resolved so that they can be safely disposed when the scope is disposed. These references cause all disposable services to stay in memory for as long as their owner scope is in memory.

As a possible optimization, we could define a DI intrinsic service interface that any IDisposable service could depend on, and through which it could remove itself from the list of disposables that it’s owner scope maintains, akin to how it would call GC.SuppressFinalize(this) to remove itself from the finalizer queue as part of the Dispose implementation.

[rynowak]

Related: dotnet/aspnetcore#5496 dotnet/aspnetcore#10448

[analogrelay]

Let me make sure I understand this. The idea is that services like DbContext could be disposed "early" by user-code but because the DI scope is holding a reference in order to dispose it, they will be kept alive? And the proposed remedy is to allow such a service to call some method provided by DI to "remove" itself from this dispose list? So the pattern is something like:

public class DbContext
{
	public void Dispose()
	{
		// Do dispose things

		// Remove ourselves from the containing lifetime's list of things to dispose.
		SomeDIApi.ComponentWasDisposed(this);
	}
}

(Note: API name and shape is a strawman)

[davidfowl]

I don't think it makes sense to couple arbitrary types to the container, this sort of design flies in the face of having services decoupled from the DI system. Something on the registration is much more reasonable (or managing scopes yourself which lets you control the lifetime of objects in that scope)

[divega]

@anurse, not exactly. The API to call would be a method in another service that DbContext would depend on.

@davidfowl I would like to understand what kind of coupling we are trying to avoid and why exactly. For example, this optimization would be directly applicable to all DI systems we can wrap, but also the service interface could be implemented directly by any DI system that keeps track of disposables in a similar way. Also, curious what alternative you have in mind that would allow something in the registration to help?

[analogrelay]

The API to call would be a method in another service that DbContext would depend on.

Well, yes that's what I meant by the shape not being super clear, but the idea is that you (the service in question) are trying to opt out of DI disposal because you've already been disposed.

[divega]

Yes, that is the general idea. I just wanted to emphasize that there is no magic or static method and you do it through a service interface because of the point about coupling.

[davidfowl]

@davidfowl I would like to understand what kind of coupling we are trying to avoid and why exactly. For example, this optimization would be directly applicable to all DI systems we can wrap, but also the service interface could be implemented directly by any DI system that keeps track of disposables in a similar way.

Adding an interface doesn't mean things aren't coupled. When your objects need to rely on services from DI, that's coupling and code smell. The ideal state is to resolve a clean object graph (without dependencies on any infrastructure from the container).

We've had similar ideas for things in the past based on a generic type: Owned<T>. If you resolve an Owned<T> then you're signaling to the DI container that you don't want it to track the object lifetime. Having that Owned<T> show up in your ctor arguments is as bad as injecting an interface for the container to "forget" the registration (ok, maybe it's a bit worse). It would also need to work with all the 3rd party containers.

The mechanism we have for managing the lifetime of objects today is scopes, and this feature will need to be tied to that.

Also, curious what alternative you have in mind that would allow something in the registration to help?

You could imagine extra metadata saying that this type of object shouldn't be tracked. The problem is that decision is made at declaration time rather than at the consuming call site.

[divega]

Adding an interface doesn't mean things aren't coupled.

Yup, not saying they aren’t, though I would argue it is a slightly more “benign” case, especially for an optional optimization.

You could imagine extra metadata saying that this type of object shouldn't be tracked. The problem is that decision is made at declaration time rather than at the consuming call site.

And personally I am not sure it is a good idea to remove the ability from the scope to ultimately dispose any disposables that weren’t disposed early. Especially because you need to know exactly how the service was registered to know if it is safe not to dispose.

[ajcvickers]

Would really like @rynowak's opinion here. The things @davidfowl are saying don't jive (in my mind) with what was discussed with Ryan in the meeting about this.

[rynowak]

I wrote a sample of Owned<> and showed @davidfowl in our 1:1 https://github.com/rynowak/BlazorEFFriendshipParty - I would have booked time to show this to the two of you but I literally didn't have time in the last two days. I'm travelling now and everyone wanted to see me before I left 😎

The highlights of this:

• Owned<> as a pattern isn't baked into DI
• Component base class that resolved Owned<MyDbContext> for you

So far this sounds really good right? This doesn't require a new DI feature, and it hides most of the complexity for most users.

The thing that blows up all of the ideas we discussed is that they break as soon as you define a repository abstraction. Does your repository abstraction also pull in Owned<MyDbContext>?

The pattern either becomes viral (all dependencies have to be owned), or it has to be recursive (new and complex DI feature). This isn't a problem when DbContext is the composition root, because DbContext doesn't have dependencies in the app service provider - however we can't really tell people to use Owned<> if they write their own stuff, because it's not powerful enough.

---

So the next steps for Blazor.... Steve and I are going to discuss further the idea of having a way to declare a component that owns a scope. Blazor has other ways of passing data around (cascading parameters) and we might be OK. This then becomes another thing users have to learn about. Ultimately that was going to happen, even if we did Owned<>.

The fallback option is that we provide a special integration point for EF and don't try to solve the problem generally. That's probably acceptable - it will require us to write an EF integration package - but I ultimately see that as a good thing. I think we should we be willing to create integrated experiences as long as they are optional.

What I'm worried about is us not providing guidance about this leads to users writing bad app code. Suppose we did something sketchy like write a base class that fetches the DbContextOptions<> from DI and uses the constructor to create your context. This will allow users to write good app code, even though it might not be pure or support every configuration pattern that exists. There's always the opportunity to make our built-in support work better, the more urgent thing is to avoid footguns for users.

[ajcvickers]

Thanks @rynowak. That makes things much clearer.

[analogrelay]

The sample looks good. The idea here is that because GetOwnedInstance uses Activator.CreateInstance it's tracked "outside" of DI, by the owner of the handle, am I correct? What if the DbContext (or other service in question) is registered with a factory method? Would DI auto-register an IOwningHandleFactory in that case?

[dazinator]

The mechanism we have for managing the lifetime of objects today is scopes, and this feature will need to be tied to that.

+1 to what @davidfowl said. IServiceScope is the framework provided DI mechanism for partitioning lifetime, and anyone with any experience of Microsoft.Extensions.DependencyInjection will be familiar with them. Every http Request that flows through an asp.net core middleware pipeline today is currently serviced by one. EF core works fine in an IServiceScope because it has a (relatively) limited lifetime (singleton usage was never recommended). I'm not on the team so from an outsiders perspective the problem seems to me that Blazor doesn't currently use IServiceScopes to scope operations. It has a single IServiceProvider (corresponding to ApplicationServices) which it uses throughout the lifetime of the app - having everything live for as long as the application is not optimal. Yet it does have smaller lifetimes - for its pages / components. It creates them and disposes of them. So conceptually I would think it should create / dispose service scopes in order to formalise these smaller lifetimes for its pages / components. In other client UI stacks (WinForms, WPF, Xamarin Forms etc) - you can also create a scope when navigating to each new page / window. When the page / window is disposed you also dispose that scope. In this manner EF also works fine in those stacks - its registered exactly as it currently is in asp.net core mvc apps today (scoped). Consistency for the win! I'm guessing you must have thought of this already and perhaps there is something with Blazor that makes this too difficult - not sure. But I have used this concept successfully across wpf, winforms, xamarin etc - and hopefully will do so in blazor - please don't make us learn Owned just so blazor doesn't have to create an IServiceScope to resolve its pages in (for objections as yet undefined) - this stuff is onerous enough to learn as it is, we don't want more ways of solving a problem already solved
End of one guys opinion, now I'll butt out ;-)

[dazinator]

One last point: Also please consider that transient lifetime for dbcontext is not optimal. If you navigate to a blazor page which has 3 services injected, themselves accepting a dbcontext in their constructor, you'll end up resolving 3 seperate dbcontext instances. This will yield a mess. Compare this to the behavior today in regular asp.net core mvc, when injecting those three services into an mvc controller, they would each get passed the same DbContext instance from the RequestServices (scope). This behavioral difference will result in subtle bugs when people attempt to reuse their services in blazor. IServiceScope is the only option that makes any sense imho.

[davidfowl]

It's broken because it's not transitive, it only handles giving up ownership of the thing marked Owned<T> and that's how it fell apart.

[divega]

@davidfowl @rynowak thanks for the summary. Re Owned<T> only working for a single IDisposable service, yes, that is what I was referring to in my comments at dotnet/aspnetcore#5496 (comment).

I get the concerns with having services coupled to the DI system, but it is hard for me to understand how starting to use IServiceScope pervasively in application code (and potentially inside other services) would be better. Is it just because IServiceScope is an already established DI interface vs. a novel one that it doesn't smell? (Edit) I.e. how would you use IServiceScope without falling into service locator?

Anyway, another way I started to think about this is that if you squint, "Disposed" is just an event (somewhat interestingly, there is precedent, but I wouldn't want to carry all the baggage that comes with it).

[davidfowl]

I get the concerns with having services coupled to the DI system, but it is hard for me to understand how starting to use IServiceScope pervasively in application code (and potentially inside other services) would be better. Is it just because IServiceScope is an already established DI interface vs. a a novel one that it doesn't smell? (Edit) I.e. how would you use IServiceScope without falling into service locator?

@rynowak is looking at having Blazor create a scope for specific components, not application code. It's also better than Owned<T> because it's the composition root that's doing the resolution of the dependency graph so the services themselves don't need to be aware of which scope they reside in.

[danroth27]

@rynowak is looking at having Blazor create a scope for specific components

Maybe just for routable components?

[analogrelay]

Are we still moving towards a solution inside DI or outside DI here? If we're going to do something in DI, the time window for that change is getting narrower and narrower :).

[davidfowl]

I'm really hoping we don't have to do anything inside of DI for 3.0, it feels far too late to vet ideas against other containers.

[analogrelay]

Agreed, that's my main concern is that it's very late to make a DI change here, especially given the ecosystem.

[khellang]

We've been through this quite a few times already. See aspnet/DependencyInjection#456 (and the myriad of linked issues).

[rynowak]

For Blazor's usage we're going to provide some samples and maybe a base-class that creates its own scope. We're not expecting DI to unblock us.

[djj0809]

Well I was caught by this "issue" as well.
I don't think using scope everywhere is a good idea. Request scope for web request is a good idea, but not for other things. If you write a simple batch or worker program, things do not naturally have a "scope" concept.
Now is 2019.06, and EF Core documentation (https://docs.microsoft.com/en-us/ef/core/miscellaneous/configuring-dbcontext) still suggests using transient DBContext without warning.
Our objective is preventing people from trapped by this issue, no matter by good documentation or changing design.

[davidfowl]

What's the issue with having a scope in your worker program?

[djj0809]

What's the issue with having a scope in your worker program?

To fix this issue, I have to create extra scope when I need a DBContext, this works fine now. But it is extra work right? All I need is a transient object, but it tells me that you need to create a scope first, or we won't release it, and you will get a OOM finally.

At least please update the documentation I mentioned above, and perhaps also write a guide about using DI in DotNet Core console application.

[brandondahler]

Going through all of the individual discussions, I wanted to try to summarize all of the proposed solutions and why they aren't favorable enough to implement.

I've also added some situations and solutions that I believe fit the general discussions' recommendations. Finally, I've made some general guidelines from those.

Proposed Solutions

Disposed Event

Summary

Have an interface that ensures a Disposed event is exposed, implementers are expected to call the Disposed event when Dispose is called on it. Scopes would register for the event after constructing the service instance, would remove item from _disposables if the service gets disposed ad-hoc.

Problems

• Couples types to DI, which is generally undesirable
• Requires services to participate in order for it to work
• Not backwards compatible

Owned<T> aka INotifyWhenDisposed

Summary

A type that owns the actual disposable item. Allows resolution of T's dependencies to be done by and registered with the service provider while allowing T to be disposed manually through Owned<T>.Dispose(), which will also release T's reference.

Problems

• Becomes viral in nature or requires some recursive implementation.
• Doesn't really resolve the problem in an elegant manner, probably makes more problems than it solves.

WeakReferences

Summary

Instead of holding normal references to the disposable items, use WeakReference instances to Dispose the instances if they haven't already been GC'd.

Problems

• GC timing is not reliable and may result in the same code sometimes calling .Dispose and other times being GC'd without disposal.
• If only done in the root scope, that would makes it weirdly special and may lead to just as much confusion because of the non-uniform behavior.

Make auto-disposal a registration option

Summary

Add some way to opt-out of auto-disposal of instances during registration.

Problems

• Allows for non-uniformity across libraries, some may register with and others without by default.
• Decision is made at registration instead of at the call site.

Just don't track or dispose transients

Summary

Don't add transients to _disposables at all or maybe add transients with a WeakReference.

Problems

• Breaking change, we currently call Dispose on transients.
• Using WeakReference on transients is still a breaking change and also has the same problem as WeakReferences above.

Situations and Solutions

Transient, limited lifetime

Situation

You need an IDisposable instance with a transient lifetime that you either are resolving in the root scope or would like it to be disposed before the scope ends.

Solution

Use the Factory pattern to create an instance outside the parent scope. In this situation, you would generally have a Create() method that calls the final type's constructor directly. If the final type has other dependencies, the factory can receive an IServiceProvider in its constructor and then use ActivatorUtilities.CreateInstance<T>(IServiceProvider) to instantiate the instance outside the container while using the container for its dependencies.

Shared Instance, limited lifetime

Situation

You need to share an IDisposable instance across multiple services but you need the IDisposable to have a limited lifetime.

Solution

Register the instance with a Scoped lifetime. Use IServiceProvider's CreateScope() to start a create a new IServiceScope, then use that scope's ServiceProvider to get your service(s). Dispose the scope when you want the lifetime to end.

General Guidelines

1. You probably shouldn't register IDisposable instances in Transient scope, use the factory pattern instead.
2. Don't resolve Transient or Scoped IDisposable instances in the root scope unless you're (re-)creating and disposing the IServiceProvider (which you also probably don't want to be doing).
3. Receiving an IDisposable dependency via DI does not infect the receiver with the need to implement IDisposable itself. The receiver of the IDisposable dependency should not call Dispose() on that dependency.
4. Scopes should be used to control lifetimes of services. Scopes are not hierarchical and there is no special connection among scopes.

[akovac35]

Hi Team, I would like to point out that users really don't expect the described behavior from the DI framework. It might be better to just introduce a braking change and make it perform as expected instead of documenting it - this is something EF Core team is doing regularly and it shows because the framework is getting better and better. Otherwise, how exactly are you planning to document this? Don't use transients in case of ... ? It just doesn't seem right :)

[dazinator]

@davidfowl @brandondahler

Have you considered the following worryingly (for me) simplistic solution.. when a transient disposable TService is registered using AddTransient, set up an additional registration to also allow a Func<TService> to be injected. You could have that Func remove the transient instance from the tracking list as soon as it is invoked, before returning it - leaving responsibility for disposal now entirely at the call site?

So now if I inject Func<TService> I can get call that to get the transient disposable instance which I must dispose of myself - it will be removed (or never added in first place?) from tracking list. If I inject TService directly then I am dependent on an owning scope to dispose of that service (I do not own it) so its going to be in some tracking list for the duration of the owning scope.

I think this would work. Personally I don't like the idea of tracking transients for disposal and would advocate for the breaking changes to remove that- I left a long winded comment to that effect but deleted it - was too long.

[analogrelay]

Triage: The original motivating scenario (Blazor Server) no longer needs this change.

Not tracking transient services for disposal may well have been a better choice but it's significantly breaking and not really a viable option for us here. Closing as we don't plan to take further action at this time.

[akovac35]

So not using transients with blazor is the recommended solution?

[dazinator]

Just curious if you considered anything similar to:

1. When transient service is registered with AddTransient<TService> - create a Func<TService> registration and allow that to be resolved, in addition to just TService.
2. The Func<TService> implementation would remove the transient instance from the IDisposable tracking list, as soon as it was invoked.
3. Users can still inject TService as before - and would have to wait for services resolved that way to be disposed as per normal. But for those occasions where users want to take responsibility themselves for disposal, they can inject Func<TService> and invoke that to obtain their instance.

I appreciate your immediate need for this feature may have diminished, but perhaps being able to take ownership of transient disposal is still a feature worth considering for the future?

[rynowak]

So not using transients with blazor is the recommended solution?

You can use OwningComponentBase if you need to control the lifetime of a service. We'd recommend that if you like using transient or scoped things as a "unit of work".