Open SteveSandersonMS opened 4 years ago
ghost
commented
4 years ago Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.
ghost
commented
4 years ago We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.
davidfowl
commented
4 years ago Can I assume backlog is another way to say, we're going to do it for 6.0 😬
mrpmorris
commented
2 years ago Have there been more discussions about this since?
Could someone share the current thoughts so we can talk through it?
TanayParikh
commented
2 years ago Allow for more granular control over the lifetime of transient services implementing IDisposable within Blazor Server.
Transient services which implement IDisposable currently leak memory. The behavior is as follows:
Transient Service which do NOT implement IDisposable |
Transient services which implement IDisposable |
|---|---|
| Are garbage collected when the component in which the service was injected is disposed. | A reference to the service is maintained by the DI container so it can handle service disposal. This prevents garbage collection of the service upon component disposal. The service is kept in memory till the DI container itself can be garbage collected. |
The service is kept in memory till the DI container itself can be garbage collected.
The DI container for Blazor Server cannot be garbage collected until the circuit is terminated. Consequently, transient services implementing IDisposable are kept in memory till circuit termination.
Given the lifetime of the circuit may be much longer than the lifetime of the component, we're unduly delaying the disposal of transient services implementing IDisposable, thus leaking memory.
IDisposable within Blazor Server.We want to ensure that transient services implementing IDisposable can be disposed when the component in which the service was injected, is disposed.
Depending on the solution chosen, it may be possible for users to purposely not dispose of the transient service upon component disposal. This would be discouraged, but it may not be possible to entirely prevent.
Have the Blazor framework handle the lifetime of IDisposable transient services.
The Blazor framework requests the DI container (Service Provider) for a tracker. A tracker is maintained for each component created. All transient services created for a specific component are associated with the tracker. When the component is disposed, the tracker is disposed, thereby disposing the IDisposable transient services created using the tracker.
Framework code:
IServiceProvider provider;
// Create a tracker for the component.
var tracker = provider.CreateTracker(); // [NEW]
// `IDisposable` transient services injected into the component are created
// using the tracker.
tracker.GetRequireService<ITransientDisposableService>();
// The tracker is disposed when the component is disposed.
// The `IDisposable` transient services created using the tracker are disposed via the tracker disposal.
await tracker.DisposeAsync();IDisposable when the component is disposed.IDisposable transient services.
IDisposable within Blazor Server.IDisposable transient services.Allow users to manage the lifetime of IDisposable transient services.
Users wrap the IDisposable transient service with an additional generic interface which facilitates user disposal of the IDisposable transient service.
User code:
@inject ITracked<ITransientDisposableService> trackedTransientDisposableService
@{
var service = trackedTransientDisposableService.Value;
trackedTransientDisposableService.Dispose();
}Framework code:
IServiceProvider provider;
return provider.GetRequiredService<ITracked<ITransientDisposableService>>();Definition
interface ITracked<T> : IDisposable
{
T Value { get; }
}Advantages / Disadvantages are essentially the inverse of the explicit approach.
IDisposable within Blazor Server.ITracked wrapper would handle much of the functionality.ITracked wrapper.IDisposable when the component is disposed.IDisposable transient services.Allow both implicit and explicit management. Implicitly manage IDisposable transient services by default, however allow explicit management of transient disposable services wrapped with (ITracked). Thus we get the advantages of implicit handling without losing the ability to granularly control the lifetime of IDisposable transient services should that be desired.
IDisposable transient services.ITracked wrapper.OwningComponentBaseBy leveraging OwningComponentBase we can manage the lifetime of IDisposable transient services within Blazor Server presently.
By having a component extend OwningComponentBase, we have access to a ScopedServices provider.
User code:
@implements OwningComponentBase
@{
var service = ScopedServices.GetService<ITransientDisposableService>();
}The disposal of the component results in the disposal of the OwningComponentBase. Thus, the ScopedServices provider is garbage collected, and the transient services implementing IDisposable are disposed.
@inject).Varies based on the approach chosen. The disadvantages sections cover the risks and unknowns of the approach.
Requires updating the Dependency Injection Container implementation which is used broadly. Different stakeholders may have differing concerns and requirements.
See above.
mrpmorris
commented
2 years ago Could this be solved with CascadingValue, perhaps?
A component like OwningComponentBase (let's call it ScopedComponent) creates a new IServiceScope and cascades that down as a CascadingValue.
Any components rendered inside this component get that new cascading value, and therefore the relevant scope. That scope is disposed along with the ScopedComponent that owns it.
This way you can have any service last as long as you like
1: App lifetime = Register as Singleton
2: Session lifetime = Register as Scoped
3: Component (and children) lifetime = Register as Transient and consume from a ScopedComponent
I think this would make sense to users because it uses an already known concept (cascading values) - and we know when we can dispose of the transient instances regardless of how long the user wants to keep them for.
Questions that occur to me are 1: Do you first search the parent scope for an instance of a transient service and use that, or only use transients your scope creates? 2: Do you throw an exception if a transient service is injected at the root or scoped level? 3: Do you add the ability to let the user specify they want the scenario in question 2 to happen on a per-registration level?
Obviously you don't need a new Blazor component (ScopedComponent) to achieve this. Anyone could use a standard ComponentBase and render child content inside a <CascadingValue> if they wished to.
javiercn
commented
2 years ago @mrpmorris I don't think what you are suggesting necessarily solves this problem. There is already OwningComponent which you could use do what you describe (pass down the IServiceProvider it exposes as a cascading value and use that in your components) however any scoped service wouldn't be initialized in that context (like AuthenticationStateProvider) and it would not address services resolved outside of the scope of a component, like those in a circuithandler.
In addition to that, it would create hard to debug scenarios like errors that happen when a children component is instantiated in this new special way vs in the "regular way".
@mrpmorris I don't think we would want to couple this behavior to CascadingValue. We don't want to invent any new idiom for resolving dependencies that is Blazor specific and doesn't align with the core existing DI concepts and it would have a non-trivial cost for the renderer both in terms of complexity and performance.
We are looking for a general solution in the DI space here and we are hoping to improve this scenario in the DI container itself, not just in Blazor, since other app models like Desktop apps, likely also suffer from this problem.
mrpmorris
commented
2 years ago @javiercn I think the problem is that Blazor already has introduced a new idiom. On an API server a Singleton is shared by all users (as with Blazor Server), but a Scoped is very short lived as opposed to Blazor which keeps it alive for the duration of a user session rather than a user action.
I think what a developer needs to do is to be able to define the scope. They know what the "action" equivalent is for their app, for example a "CreateUser" page. That component + embedded components share the same scope, the owner of the scope destroys it when it knows the task is over (i.e. when the page is disposed).
CascadingValues was just a suggestion. No matter what the solution is, I think it should be based around letting programmers define where a new scope starts / ends - and not just giving us tools to track everything manually.
Requested Scoped services would be requested from the parent scope (as long as it's not the root) and if they don't exist then are created inside the scope specified by the programmer. This would give the same AuthenticationStateProvider throughout the app.
Have a new option on the service container that specifies IDisposable cannot be injected into the root container or a first-child container (first scoped off the root).
javiercn
commented
2 years ago @javiercn I think the problem is that Blazor already has introduced a new idiom. On an API server a Singleton is shared by all users (as with Blazor Server), but a Scoped is very short lived as opposed to Blazor which keeps it alive for the duration of a user session rather than a user action.
I don't think that is a new idiom. The scope is defined by the app model, and nothing says how long it should last. The container has a behavior that favors one app model over the other without giving developers/frameworks choice. The fact that the container keeps a reference to transient disposables works well with web apps where the resolutions normally happen in the context of a request which only last a few milliseconds/seconds, but that doesn't work for desktop apps or background services in a web app (for example).
Containers keep track of transient disposables because otherwise the "implemementation would leak" but that also have the side effect of holding a reference to the instance for the lifetime of the scope (and doesn't matter how long your scope lasts). It essentially makes any transient disposable into a scoped service for garbage collection purposes, without the framework/user having any control over it. It shouldn't be all or nothing, there should be a way for frameworks/users to control this behavior in a way that works with their app model. That should be orthogonal from what service gets resolved from where.
mrpmorris
commented
2 years ago What I suggest would solve all of that.
1: The user specifies in their component that this is a new scope 2: All embedded components get the same scope when injecting dependencies 3: The scope checks the parent (but not as far as the root) for an instance before creating it and managing it for itself 4: That component disposes of the scope
You are just giving the user the ability to define where their scopes are (like a request in MVC). WinForms / MAUI etc would need to do the same - as it is up to the framework consuming the DI to understand its own specific requirements, the DI is just there to provide things, not understand the workings of different consumers.
Obviously the user would need to be able a navigation from a component back to itself with different parameters also counts as a new scope.
I think if you implement this in the DI itself you are making things messy. DI does one thing only, provides services in scoped containers - the consumer should manage the scopes and thus the lifetimes of everything within them.
Sebazzz
commented
2 years ago Scoping DI on a per component basis is imho a case of "barking up the wrong tree".
If you look at something practical like a component with a few buttons that is used for minutes and makes some database calls via Entity Framework. The component is visible for the duration the user uses the page and during this time the database connection is kept occupied and the DbContext and everything that comes with it is kept in memory, including any (eventual) stale data.
What you'd rather want is that you use these scoped services only when you need it, and otherwise dispose of it: during loading of data and when an event handler is called (for instance opening a temporary scope via a IServiceScopeFactory). This will cause higher GC pressure because you have a bunch of temporary objects that are only used when the event runs - but you're not keeping it around in memory.
mrpmorris
commented
2 years ago @Sebazzz The Db connection on a DbContext is not kept open for its lifetime. It is retrieved from a pool and returned whenever you perform a DB operation.
You do want the DbContext held for the lifetime of the component at the heart of the action (e.g. maintain purchase order) because you'll have change tracking turned on, sub-components will add new child associations to a 1..* owned by the parent, child associations will be removed. All of that will be tracked until the developer says "now save it or discard it" and EF will do its magic and add/update/delete whatever is necessary to reflect the user's changes.
If you create a DbContext, then fetch, then dispose, then allow editing of detached objects then you are going to have to manually track which objects were altered / added / deleted and that is a lot of work. Or you will be forced into the existing pattern websites currently have where every object edit is an atomic operation (e.g change order line, remove order line) - you remove the ability for the developer to have a "modify these complex object relationships and then save or cancel at the end".
Note: I am not saying there should only be this way of doing it. I am just saying this should be one way of dealing with the problem.
SteveSandersonMS
commented
2 years ago Thanks for the detailed design, @TanayParikh! I see this is all a matter of tradeoffs, as usual :)
Question: is this Blazor Server-specific? It seems like it would equally be a problem on WebAssembly (holding browser memory indefinitely) and on WebView for all the same long-lived-scope reasons.
One challenge for us here, which you've identified, is trying to make this a near-zero-perf-cost addition in the case where people aren't even using transient disposables. Creating a special new scope for every component, when we don't know if it uses transient disposables, could be problematic (though TBH I don't know how much overhead there is in it).
If we are adding a new feature to the DI container to support this, I wonder if it's enough just to have a new API on the container like scope.DisposeIfTransient(instance). We could change Blazor's component teardown logic to walk through all the [Inject]-annotated properties on the component instance and call scope.DisposeIfTransient(thatPropertyValue) (caching the list of such properties on a per-type basis). Then the service scope could check if it was holding the instance just for the purpose of disposal, and if so, dispose it now and stop holding it. If the instance is scoped or singleton, the DI container would do nothing. And if we didn't want to use reflection to walk the properties, we could have the Razor Compiler codegen a series of calls to dispose each of the things that got @inject-ed. This sort of approach would have no perf impact for components that aren't doing any of this. Perhaps it's impractical for some reason I haven't anticipated, though!
As a drawback, this would give up on the "manual control" aspects of things that you mentioned above, so maybe the ITracked<T> concept is required if we want to dispose-by-default but also allow developers to flag that a given service injection shouldn't be auto-disposed with the component.
mrpmorris
commented
2 years ago @SteveSandersonMS To be clear, my suggestion isn't to have a new scope for every component, it was to allow the developer to specify their component should have a new scope (which is then also used by any other components it renders, unless they specify their own).
Regarding scope.DisposeIfTransient, that would work quite nicely as long as when you resolve ComponentX than contains ComponentY they don't get injected with the same instance - which I assume is the case?
SteveSandersonMS
commented
2 years ago Regarding scope.DisposeIfTransient, that would work quite nicely as long as when you resolve ComponentX than contains ComponentY they don't get injected with the same instance - which I assume is the case?
With transient, you always get a new instance each time you ask for one. https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection?view=aspnetcore-6.0
mrpmorris
commented
2 years ago With transient, you always get a new instance each time you ask for one. https://docs.microsoft.com/en-us/aspnet/core/fundamentals/dependency-injection?view=aspnetcore-6.0
Oooh, thanks for setting me straight on my misunderstanding!
I always thought it was once per build-up. I wonder if I am remembering back to the old MS Unity days? Oh well, I'm pleased I now know a new thing :)
egil
commented
2 years ago Ideally a component should not have any knowledge about the life time of services it depends on. That is an implementation detail of the individual service, and the point of the dependency inversion principle is that the dependant depends on abstractions, not the concrete implementation.
Thus I'm not a fan of ITracked or similar solutions, where we have to embed the life time information about each service in each component they are injected into.
Since it's the Blazor runtime that owns the life time of Blazor components, it can tell the service provider when a component goes out of scope and the service provider can then dispose of any services as needed (believe that's what @SteveSandersonMS suggest above).
MariovanZeist
commented
2 years ago I am in @egil corner on this one, in that the lifetime of service is an implementation detail, not something a user should need knowledge of. Therefore I am not enthusiastic about an ITracked<T> concept.
I think @SteveSandersonMS suggestion of changing Blazor's component teardown logic is good. Have all the [Inject] annotated services checked for Transient disposablilty and dispose of them if applicable. It's simple and can be implemented transparently to the user. It also makes sense logically as the user already stated his "consent" by using the [Inject] keyword, that the Blazor infrastructure besides instantiating the service is also responsible for its cleanup.
With the important addition that the references to those IDisposables are also removed from the DI Scope. All of this is not explicitly related to Blazor-Server or Blazor in general and is problematic for the DI Implementation as a whole.
In my opinion, one should never use transient IDisposables. for 2 reasons.
There is no clear lifecycle when that service will be disposed of. One could request a service from the global scope, and that transient disposable will never be disposed of until either the application ends, or it's manually disposed of. (The same arguably applies to a Singleton service as well, but in that case a developer choose for that implementation)
The scope that instantiated the service will hold on to that reference until that scope get's destroyed. (Which can be a long time in case of global scope, or for example Blazor Server SignalR scope, or Blazor Wasm scope)
Problem 2 can be even more problematic if you request multiple Transient IDisposables from any scope, even when you properly Dispose of it and clear all references, the DI Scope will hold a reference to that Service until that scope gets destroyed.
This might actually be a feature request for the DI team, in that we can remove tracked instances from a scope, as the above-mentioned solution by steve would also need that functionality, it would be great if it could be made a public function.
egil
commented
2 years ago I think @SteveSandersonMS suggestion of changing Blazor's component teardown logic is good. Have all the
[Inject]annotated services checked for Transient disposablilty and dispose of them if applicable. It's simple and can be implemented transparently to the user. It also makes sense logically as the user already stated his "consent" by using the[Inject]keyword, that the Blazor infrastructure besides instantiating the service is also responsible for its cleanup.
The problem with this approach is that an implementation of ISrv is not know at compile time. It's a runtime decisions based on what type is registered in the IoC container to fulfill ISrv.
I don't see another solution to this than to tell the service provider that the user (component) of ISrv is disposed/out of scope, and give the service provider the chance to dispose of the implementation of ISrv if it's transient.
That does unfortunately affect perf as that would have to be done for every service.
SteveSandersonMS
commented
2 years ago That does unfortunately affect perf as that would have to be done for every service.
I'm not too worried about that: we'd only codegen this call for @inject, and the DI-system can no-op the call almost for free if it knows the service type isn't transient, and it's only when a component is disposed.
javiercn
commented
2 years ago @MariovanZeist This is a proposal that we plan to take over and discuss with the runtime team, we are currently discussing the approaches we can take, but there will need to be new API surface on the DI container to support this concept, whether it is something that happens implicitly or explicitly.
As I mentioned above, this is something that not only affects Blazor, and it is a leaky aspect of DI container implementations. We are not looking for a Blazor specific solution. Containers have to choices:
The issue here is that it is all or nothing, you can't tell the container when those transient disposables are no longer "needed" and he can get rid of them, hence why we think we need a way to "notify" the DI container of such event. The key problem is that the container assumes ContainerScope = DisposeScope and that is not true for Transient services.
That can happen in several ways:
Whatever we do, the scoping rules for components can't change:
I don't want people to get fixated on ITracked
No matter what we do here, we will very much hide this detail from the API surface of Blazor consumers. I don't believe they should be annotating anything in their apps. This behavior should be "automatic" and "correct" without you having to make a change to your app.
That said, there will be situations when people need to handle the lifetime of anything potentially Transient Disposable, which is for example when they inject their own IServiceProvider into a service, so again, there needs to be public API for this.
adamhathcock
commented
2 years ago I've been tracking this and maybe I'm coming out of left field or ignorance but "Transient Disposable" effectively turns into "Scoped." A lot of the suggestions seem to be adding a new layer of scoping which doesn't feel right.
Perhaps a simpler thing is changing DI behavior in possible breaking ways:
The above or something similar would be more "correct" but undesirable. All of the other solutions are Blazor Server specific (or other longer lived type scenarios.)
DisposeIfTransient seems like the compromise move without adding a new scope system.
javiercn
commented
2 years ago
- logging warning that "Transient Disposable" means Scoped so will be handled as such- no change really
- DI container doesn't track any "Transient Disposable" as the caller object should be handling the dispose.
- disallow "Transient Disposable" in DI - I guess this means runtime exception
All these details are up to the container implementation (for which there are several) and nothing the framework has control over.
We would not do anything here that involves a breaking change, or a behavior change that can cause apps or libraries to break. This is not a Blazor specific behavior, but a behavior presents in every app except for HTTP web-based apps. Containers could give the option to turn off that behavior, but that has the cost of putting the responsibility on the developer, which can be at the very least cumbersome. (You'll end up with IDisposable implementations on every component just in case). The other option is for the container to give you control through an API to tell it when you know you are done with some objects the container is tracking.
Apps and frameworks don't have a good way of determining those things unless the container gives them APIs to do so.
A lot of the suggestions seem to be adding a new layer of scoping which doesn't feel right.
Why do you think that, could you be specific?
adamhathcock
commented
2 years ago Why do you think that, could you be specific?
There's already a scoping mechanism CreateScope etc. The ITracked suggestions are controlling the lifetime of object(s) but just in a different way. Feels like adding another layer of scoping isn't the right answer. It's redefining/reworking lifetime control.
The other option is for the container to give you control through an API to tell it when you know you are done with some objects the container is tracking.
This is a scope. I control disposables in my long lived apps (non-HTTP and Blazor server) this way. EFCore and other things demand it. Unfortunately, I have to make a scope per button click in my Blazor server apps that aren't only doing HTTP.
How do deal with "Transient Disposable" is purely up to implementation? Admittedly, I mostly use the default DI as that's all I need though I may need to rethink that.
Reminds me of another DI definition issue I followed: https://github.com/dotnet/runtime/issues/67391
alexanderfedin
commented
2 years ago Do you, guys, remember what the WeakReference class is and what is it for? If you let DI container reference transient objects through WeakReference only, then the problem solves by itself.
javiercn
commented
2 years ago @alexanderfedin that does not solve the problem.
The container holds a strong reference to the instances so that it can call Dispose on them when the scope is disposed. A weak reference does not work because if the container is the only thing referencing them, they might go away before the container being able to call Dispose on them, which makes the disposal behavior subject to GC.
alexanderfedin
commented
2 years ago As simple as following:
mrpmorris
commented
2 years ago
- when the time comes to dispose, check the weak pointer to see if it is still alive and dispose accordingly
The question in this issue though is, when does the time come to dispose?
WeakReferences will tell you when something is no longer referenced, but you obviously cannot hold a reference to it yourself.
alexanderfedin
commented
2 years ago Let's say, the item is transient and the client got it through DI. That means that there are just two references to that item: one is strong (by client) and the other is weak (by DI container). After the client is done using the item, it has the freedom to lose the strong reference to that item. When GC gets invoked, it can dispose of the item if it is referenced only by the weak reference. If the DI container was asked to give the reference to the item again, it either returns the reference that is still alive (hence making the reference strong to the GC) or instantiates the item again. You can surely count on the GC to properly dispose of the item if it has a finalizer and no one else but the DI container has a [weak] reference to it, or otherwise, there is nothing to worry about, as that probably means there are no unmanaged resources involved.
javiercn
commented
2 years ago When GC gets invoked, it can dispose of the item if it is referenced only by the weak reference.
The GC doesn't call dispose on IDisposable elements. DI containers do not operate on the way you describe and they can't.
You can surely count on the GC to properly dispose of the item if it has a finalizer and no one else but the DI container has a [weak] reference to it, or otherwise, there is nothing to worry about, as that probably means there are no unmanaged resources involved.
Finalizers are a last resort measure, no something that should be your first pick when you need to release resources. We are not talking about objects with a finalizer here, we are talking about IDisposable implementations, which require the runtime/app to participate in the release process by calling Dispose().
Finalizers have a big impact in GC, amongst other things they run in a special thread (which can be congested if you heavily rely on it) and Finalizable objects and those they reference are guaranteed to survive at least one GC generation, which means that they are going to be in memory much longer or cause higher generation collections (up to Gen 2) which are way more disruptive to the application than Gen0 collections.
In the current container implementation, the container is the one responsible for doing so. The GC is not going to take on this responsibility (the GC already deals with Finalizers), and we are not going to change neither the DI container responsibility in disposing objects nor the GC to start doing so.
Any solution we do here needs to operate within the constraints we currently have:
ghost
commented
1 year ago We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.
ghost
commented
11 months ago Thanks for contacting us.
We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.
This is something we've discussed in the past, and previously decided that we needed a notion of nested DI scopes to solve truly. If this is possibly in scope for .NET 6, we need to begin an actual design process.
cc @javiercn @davidfowl