dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.47k stars 4.76k forks source link

[API Proposal]: Allow opt-out of automatic disposal in Microsoft.Extensions.DependencyInjection #109492

Closed feO2x closed 6 hours ago

feO2x commented 4 weeks ago

Background and motivation

In certain programming scenarios, I need to prevent the automatic disposal of instances resolved by the ServiceProvider in Microsoft.Extensions.DependencyInjection. Currently, the implementation always disposes of resolved instances if they implement IDisposable (or IAsyncDisposable when using AsyncServiceScopes or invoking ServiceProvider.DisposeAsync). There is no built-in way to configure this behavior. This automatic disposal also applies to transient instances resolved by the DI container, where the suggested workaround is to use factories to avoid disposal of such instances. This proposal suggests introducing a parameter during service registration that would allow callers to control whether the ServiceProvider should invoke Dispose or DisposeAsync.

An illustrative example of this need is a long-running ETL process executed nightly, which involves aggregating data from multiple third-party systems. While a DI container scope might remain open throughout the batch process, the calling code may need to close specific connections preemptively. Although it is technically feasible to do so today, the DI container retains references to these instances and will still call Dispose on them. While Dispose methods are ideally idempotent, this expectation is not always met, which can lead to subtle bugs, especially when working with third-party code not under your control. A more robust solution would be for the DI container to exclude such instances from its disposal tracking, thereby avoiding Dispose calls at the end of a service scope or when the ServiceProvider itself is disposed.

API Proposal

To achieve this, we could add a new property to the ServiceDescriptor class called DoNotDispose. This property is immutable and set by the private constructor which receives an additional doNotDispose parameter. The other constructors need to be extended with this parameter, too, and could set it to the default value of false (or, if we want the ABI to be compatible, we would need to create new constructor overloads which have the doNotDispose parameter).

namespace Microsoft.Extensions.DependencyInjection;

public class ServiceDescriptor
{
    public ServiceDescriptor(
            Type serviceType,
            Func<IServiceProvider, object> factory,
            ServiceLifetime lifetime,
            // new optional parameter here - we could also introduce new constructor overloads
            bool doNotDispose = false)
            : this(serviceType, serviceKey: null, lifetime, doNotDispose)
        {
            ThrowHelper.ThrowIfNull(serviceType);
            ThrowHelper.ThrowIfNull(factory);

            _implementationFactory = factory;
        }

    private ServiceDescriptor(Type serviceType, object? serviceKey, ServiceLifetime lifetime, bool doNotDispose)
    {
        Lifetime = lifetime;
        ServiceType = serviceType;
        ServiceKey = serviceKey;
        // The private constructor sets the immutable DoNotDispose property
        DoNotDispose = doNotDispose;
    }

    public bool DoNotDispose { get; }
    // Other members omitted for brevity's sake
}

Additionally, all the AddTransient, AddScoped, and AddSingleton extension methods would need to be updated to include the doNotDispose parameter (or new overloads could be created to maintain ABI compatibility). Below is an example:

namespace Microsoft.Extensions.DependencyInjection;

public class ServiceCollectionServiceExtensions
{
    public static IServiceCollection AddTransient(
        this IServiceCollection services,
        Type serviceType,
        [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type implementationType,
        bool doNotDispose = false);

      // Other members omitted for brevity's sake
}

The DoNotDispose property value could then be passed to the internal ServiceCallSite class and used in combination with the existing CaptureDisposable property to determine whether the DI container should track an instance for disposal. A proof-of-concept implementation can be found here as a Proof of Concept: https://github.com/feO2x/dotnet-runtime/tree/features/optional-disposal

API Usage

When used directly with the service provider:

await using var serviceProvider = new ServiceCollection()
    .AddTransient<IDisposableService, DisposableServiceImplementation>(doNotDispose: true)
    .BuildServiceProvider();

using (var disposableService = serviceProvider.GetRequiredService<IDisposableService>())
{
    disposableService.DoSomething();
} // service is disposed here by the caller and can be garbage-collected because serviceProvider won't track the instance as a disposable

Or with a scope:

await using var serviceProvider = new ServiceCollection()
    .AddTransient<IDisposableService, DisposableServiceImplementation>(doNotDispose: true)
    .BuildServiceProvider();
await using var scope = serviceProvider.CreateAsyncScope();

using (var disposableService = scope.ServiceProvider.GetRequiredService<IDisposableService>())
{
    disposableService.DoSomething();
} // service is disposed here by the caller and can be garbage-collected because scope won't track the instance as a disposable

These examples indicate the usage with AddTransient, but the doNotDispose parameter can also be passed to AddScoped and AddSingleton extension method (there is a special case for AddSingleton where the instance is directly passed to the extension method. In this case, the DI container did not track the instance because it was not created by it. To be backwards-compatible, we should set the doNotDispose parameter to true in this case).

Alternative Designs

Instead of using a simple Boolean value, we could introduce a class which allows us to add more properties in the future while keeping the amount of paramters stable. For example, we could call this class ServiceSettings:

public class ServiceSettings
{
    public bool DoNotDispose { get; set; }
}

This is then used instead of the doNotDispose parameter for the AddTransient, AddScoped and AddSingleton extension methods as well as for the constructors of ServiceDescriptor.

public class ServiceCollectionServiceExtensions
{
    public static IServiceCollection AddTransient(
        this IServiceCollection services,
        Type serviceType,
        [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] Type implementationType,
        ServiceSettings? serviceSettings = null);

      // Other members omitted for brevity's sake
}

Risks

Since this feature can be implemented in a way that maintains complete backwards compatibility, the risk associated with introducing it is relatively low. Additionally, because callers must explicitly opt-in by changing the default setting, they will be aware of the implications—namely, that the calling code is now responsible for dispoal. In my humble opinion, the likelihood of confusion or frustration among users due to unexpected behavior (e.g., instances not being disposed) is minimal.

dotnet-policy-service[bot] commented 4 weeks ago

Tagging subscribers to this area: @dotnet/area-extensions-dependencyinjection See info in area-owners.md if you want to be subscribed.

wvpm commented 4 weeks ago

Working with unreliable third party code can be challenging.

Would it be practical to wrap the troublesome third party classes in a wrapper class? The wrapper ensures disposing is only done once (typically with an _isDisposed field). Alternatively the classes could be registered as a type that does not implement IDisposable.

I wonder if the connections that are closed directly have to be managed by the DI container. It sounds to me like your code manages them directly. Perhaps registering them in DI is not appropiate in your scenario.

If you do not want wrappers and want to use DI, perhaps you can register the service in question as a singleton instance. If you register an instance instead of a type, the DI container will not dispose it as it's treated as externally managed.

Changing the Microsoft.Extensions.DependencyInjection interface is unlikely to happen as implementing libraries would also have to update. See also https://github.com/dotnet/runtime/issues/41050#issuecomment-677785224 and similar comments.

feO2x commented 4 weeks ago

Thanks for pointing out that other DI containers implementing Microsoft.Extensions.DendencyInjection.Abstractions would have to adopt this new feature, too. I also want to note that the scope of the work involved is restricted - I could easily implement a PoC in a few hours for Microsoft.Extensions.DependencyInjection - I expect a similar amount of work for other DI frameworks.

Regarding the comments about the motivational example: sure, I could wrap third-party types but this can be a lot of work - I think solving this via DI container configuration is a simpler solution. And while not implementing IDisposable on these types might solve the issue described, I would argue that this is bad from a design perspective: code should be reusable and might be used in contexts where no DI container is involved. Not having the benefits of implementing IDisposable (like being unable to incorporate using blocks) is an unnecessary restriction.

julealgon commented 4 weeks ago

@feO2x

And while not implementing IDisposable on these types might solve the issue described, I would argue that this is bad from a design perspective: code should be reusable and might be used in contexts where no DI container is involved. Not having the benefits of implementing IDisposable (like being unable to incorporate using blocks) is an unnecessary restriction.

Not implementing IDisposable on a wrapper object was just one of @wvpm 's suggestions, but not the main one. The main one is to create a wrapper object that does implement IDisposable and adds an additional check on the Dispose call to prevent running the underlying disposal multiple times. That would still allow you to dispose of the instances normally with using blocks etc.

Considering how nice this issue is, and considering that guidelines on implementing IDisposable explicitly recommend it being an idempotent operation, I don't think this should be implemented.

Instead, work with the vendor for the library you are using to fix the underlying problem in their IDisposable implementation, and add a simple wrapper like @wvpm suggested in the meantime.

feO2x commented 3 weeks ago

@julealgon Thank you for your feedback. I want to note that this is a motivational example. I agree that implementing wrappers is the best thing as both of you proposed. However, I worked with libraries that implemented low-level access layers to hardware instruments built on top of socket connections, implementing costum filtering of messages via channels. There werde dozens of objects implementing IDisposable, forming a complex object graph. Of course, one would build facades for the resulting complex objects graph - but this can still take up quite some time. Having the option to configure the DI container as described in this proposal would speed things up.

I want to emphasize that this is a motivating example. If we step back and think about this topic from a design perspective, I'd argue that having automatic disposal hardwired on the fact that a type implements I(Async)Disposable and that an instance was not passed to the service collection as a singleton is very strict behavior - why not make this configurable so that the caller can decide? We can implement this in a completely backwards-compatible way. If you think that extending all DI container frameworks that adhere to Microsoft.Extensions.DependencyInjection.Abstractions is too much of a burden, please let me know.

a10r commented 3 weeks ago

I've previously come across situations were such a feature would've been useful.

If you want to register a service with more than one service type, there is no native support for this currently (#70004 tracks this use case specifically) and you'd have to do something like this instead:

services.AddSingleton<MyService>();
services.AddSingleton<ISomeInterface>(sp => sp.GetRequiredService<MyService>());

If the service is disposable this has the side effect that the container will dispose it twice. This is similar to OP's case and only really a problem if the Dispose implementation can't cope with that. I would still prefer being able to control it though.

The second case is when trying to forward requests to another service provider (basically child containers).

IServiceProvider parent = ...;
IServiceCollection child = new ServiceCollection();
child.AddSingleton<ISomeInterface>(_ => parent.GetRequiredService<ISomeInterface>());

The above has the problem that both containers will keep a reference to the service and dispose it when the container is disposed, so the child container can't be disposed before the parent container. So this approach won't really work as intended.

wvpm commented 3 weeks ago

[...] I want to emphasize that this is a motivating example. If we step back and think about this topic from a design perspective, I'd argue that having automatic disposal hardwired on the fact that a type implements I(Async)Disposable and that an instance was not passed to the service collection as a singleton is very strict behavior - why not make this configurable so that the caller can decide? We can implement this in a completely backwards-compatible way. If you think that extending all DI container frameworks that adhere to Microsoft.Extensions.DependencyInjection.Abstractions is too much of a burden, please let me know.

Automatic disposal is 'hardwired' because you use DI to manage the lifetime of those services. Part of managing the lifetime of services is proper disposal.

If you want to keep control over disposal, you should take control over the service lifetime. You can register a singleton instance. This tells the DI that you manage the lifetime. The singleton instance will never be disposed (even if the service provider is).

Note that some third party implementation have options to disable disposal. See Autofac for example.

feO2x commented 3 weeks ago

@wvpm Your summary is accurate, and there is a different way to look at this. In their book Dependency Injection Principles, Practices, and Patterns, authors Steven van Deursen and Mark Seemann outline distinct phases of interaction between calling code and a DI container: the Register, Resolve, and Release phases. If I understand your point correctly, you suggest treating the Resolve and Release phases as a combined unit for lifetime management. I’d propose an alternative: keeping these phases separate and independently configurable. This way, we can fully leverage the DI container's capabilities, such as resolving complex object graphs and reusing singleton or scoped instances, while still taking control of disposal ourselves. What are your thoughts?

julealgon commented 3 weeks ago

@feO2x

... there is a different way to look at this. In their book Dependency Injection Principles, Practices, and Patterns, authors Steven van Deursen and Mark Seemann outline distinct phases of interaction between calling code and a DI container: the Register, Resolve, and Release phases. If I understand your point correctly, you suggest treating the Resolve and Release phases as a combined unit for lifetime management.

I'm not sure the "Release" mechanism Steven talks about and actual Dispose are necessarily the same thing. I see the "Release" method as a way to "signal back to the container" that the object is "not needed anymore", kinda like what happens with the various Pool APIs in .NET where you have a method to give the instance back to the pool. You could of course decide to call Dispose on the instance at that point, but not necessarily.

Perhaps @dotnetjunkie wants to chime in on this topic here?

dotnetjunkie commented 3 weeks ago

I'm not sure the "Release" mechanism Steven talks about and actual Dispose are necessarily the same thing.

On paragraph 8.2.2, page 250, Mark and I state:

Releasing an object graph isn't the same as disposing of it. As we stated in the introduction, releasing is the process of determining which dependencies can be dereferenced and possibly disposed of, and which dependencies should be kept alive to be reused.

feO2x commented 3 weeks ago

I agree with the both of you that the Release phase and disposing an object are not the same thing. Disposal can occur during the Release phase. Yet this is not what I'm getting at. I simply want to be able to configure how the DI container acts on an instance during the Release phase, which should be independent of how it obtains a reference to the instance in the Resolve phase.

wvpm commented 3 weeks ago

@feO2x I see where you're coming from. As julealgon pointed out, pool APIs have a Release method for this reason.

In the end it comes down to responsibilities. When you create an instance, you should manage the lifetime and dispose when no longer using the instance. You can pass the instance and the responsibility along. The StreamReader constructor and the HttpClient constructor do this for example with a leaveOpen parameter. That indicates whether the disposable instance is to be disposed by the HttpClient/StreamReader or the code constructing them.

When you use Microsoft.Extensions.DependencyInjection to handle lifetime for you, it is the responsibility of the serviceprovider to dispose at the right time. If consuming services dispose injected services the next consuming service might get a service that is already disposed.

If this does not suit your scenario, employ an alternative approach. You could register a pooling service. Consuming services would inject the pool and rent and release when they see fit.

dotnetjunkie commented 3 weeks ago

Here are my two cents.

I think you should generally strive to prevent to get yourself into a situation where you need to suppress disposal of dependencies by the DI Container. This can be done by changing the design and introducing wrapper classes that act like a pool and allow suppressing disposing of its dependency (as already mentioned by @wvpm).

However, the existence of this feature in DI Containers like Autofac (which was already mentioned) and Simple Injector (which I maintain), and possibly others, gives us an indication that there's a use for this feature. In some situations, it can be really hard to work around this.

While I think this feature is useful, the question now becomes whether it is a good fit to be added to the built-in DI Container. Even though adding it to the built-in container might be rather straightforward, it's important to realize that the built-in DI Container was always meant to be a Lowest Common Denominator (LCD) with a minimal feature set that other -more feature-rich- DI Containers could extend upon.

Because it is not meant to be a full-fledged DI Container, but rather an LCD, it must take special care in which features it implements, because any feature that is added down the line (such as this proposal) could mean breaking existing third-party DI Container adapters. And that's certainly the case with this proposal. All DI container adapters must be updated to support this feature, because the adapter must be considered broken or incompatible, until the third-party adapter is updated. While this might be an easy fix for Autofac, which already supports suppressing of disposing, this might not be the case for other DI Containers that might not have native support for suppression.

What we've seen in the last couple of years is that any proposal for a new feature to the built-in container leads to a long and vivid discussion between Microsoft and maintainers of the many DI Containers in the .NET space. There are several open discussions in this space, for instance around adding decorator support that still haven't been settled.

It's, therefore, my opinion, that Microsoft should add as little features as possible to the container that force third-party DI Containers to update their adapter. Instead, if suppression of disposal is a requirement, pick the third-party DI Container that better suits your needs.

steveharter commented 5 days ago

While I think this feature is useful, the question now becomes whether it is a good fit to be added to the built-in DI Container. Even though adding it to the built-in container might be rather straightforward, it's important to realize that the built-in DI Container was always meant to be a Lowest Common Denominator (LCD) with a minimal feature set that other -more feature-rich- DI Containers could extend upon.

Correct; new features like this have to be carefully considered not only for breaking existing users, but for the layering that exists in DI.