Make CreateScope return ServiceScope that also implements IAsyncDisposable

[bjorkstromm]

Background and Motivation

IAsyncDisposable support in DI was added a while ago (see dotnet/extensions#426), but then it was agreed to only add interface implementation to ServiceProvider. I didn't find any tracking issue on implementing it also on scopes.

Currently, the following code will throw exception:

using System;
using System.Threading.Tasks;
using Microsoft.Extensions.DependencyInjection;

await using var provider = new ServiceCollection()
        .AddScoped<Foo>()
        .BuildServiceProvider();

using (var scope = provider.CreateScope())
{
    var foo = scope.ServiceProvider.GetRequiredService<Foo>();
}

class Foo : IAsyncDisposable
{
    public ValueTask DisposeAsync() => default;
}

Unhandled exception. System.InvalidOperationException: 'Foo' type only implements IAsyncDisposable. Use DisposeAsync to dispose the container.
   at Microsoft.Extensions.DependencyInjection.ServiceLookup.ServiceProviderEngineScope.Dispose()
   at <Program>$.<<Main>$>d__0.MoveNext() in C:\src\tmp\asyncdisposablescope\Program.cs:line 12
--- End of stack trace from previous location ---
   at <Program>$.<<Main>$>d__0.MoveNext() in C:\src\tmp\asyncdisposablescope\Program.cs:line 12
--- End of stack trace from previous location ---
   at <Program>$.<Main>(String[] args)

Workaround today is to cast the returned scope to IAsyncDisposable, because the concrete implementation in DI implements that.

using System;
using System.Threading.Tasks;
using Microsoft.Extensions.DependencyInjection;

await using var provider = new ServiceCollection()
        .AddScoped<Foo>()
        .BuildServiceProvider();

{
    var scope = provider.CreateScope();

    var foo = scope.ServiceProvider.GetRequiredService<Foo>();

    await ((IAsyncDisposable)scope).DisposeAsync();
}

class Foo : IAsyncDisposable
{
    public ValueTask DisposeAsync() => default;
}

Proposed API

IServiceScope implements IAsyncDisposable

 namespace Microsoft.Extensions.DependencyInjection
 {
-    public interface IServiceScope : IDisposable
+    public interface IServiceScope : IDisposable, IAsyncDisposable
 }

Usage Examples

Changing this would allow us to use DisposeAsync on the returned scope.

using System;
using System.Threading.Tasks;
using Microsoft.Extensions.DependencyInjection;

await using var provider = new ServiceCollection()
        .AddScoped<Foo>()
        .BuildServiceProvider();

await using (var scope = provider.CreateScope())
{
    var foo = scope.ServiceProvider.GetRequiredService<Foo>();
}

class Foo : IAsyncDisposable
{
    public ValueTask DisposeAsync() => default;
}

Alternative Designs

Create a new interface IAsyncDisposableServiceScope (needs better naming), which implements IServiceScope and IAsyncDisposable and let IServiceScopeFactory.CreateScope() return that.

 namespace Microsoft.Extensions.DependencyInjection
 {
+    public interface IAsyncDisposableServiceScope : IServiceScope, IAsyncDisposable { }

     public interface IServiceScopeFactory
     {
-        IServiceScope CreateScope();
+        IAsyncDisposableServiceScope CreateScope();
     }

    public static class ServiceProviderServiceExtensions
    {
-        public static IServiceScope CreateScope(this IServiceProvider provider)
+        public static IAsyncDisposableServiceScope CreateScope(this IServiceProvider provider)
    }
}

Risks

Will probably break some 3rd party implementations of Microsoft.Extensions.DependencyInjection.Abstractions and consumers.

[ghost]

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

[eerhardt]

Will probably break some 3rd party implementations of Microsoft.Extensions.DependencyInjection.Abstractions and consumers.

Is this a valuable enough change to break these implementations?

cc @davidfowl

[davidfowl]

Is this a valuable enough change to break these implementations?

It is not.

[bjorkstromm]

As Microsoft.Extensions.DependencyInjection is basically broken when it comes to using IAsyncDisposable in scopes, would you consider adding a new extension method for IServiceProvider in Microsoft.Extensions.DependencyInjection that would return a type that implements both IAsyncDisposable and IServiceScope? That way we won't break Microsoft.Extensions.DependencyInjection.Abstractions.

[davidfowl]

Can you propose the API? It would need to be an extension method (with a different name than CreateScope) that wraps the existing scope and does the test for IAsyncDisposable.

[bjorkstromm]

Can you propose the API?

Sure, naming is the hardest 😄. Sometimes I wish one could just slam on the good old Ex suffix from Win32.

Alternative 1. Introduce new interface

namespace Microsoft.Extensions.DependencyInjection
{
    public interface IAsyncDisposableServiceScope : IServiceScope, IAsyncDisposable { }

    public static class ServiceProviderScopeExtensions
    {
        public static IAsyncDisposableServiceScope CreateServiceScope(this IServiceProvider provider) { }
    }
}

It would need to be an extension method (with a different name than CreateScope) that wraps the existing scope and does the test for IAsyncDisposable.

No need to wrap in most cases if we let ServiceProviderEngineScope implement the new interface. We only need to wrap in an internal class if the returned scope from IServiceScopeFactory isn't of type ServiceProviderEngineScope.

runtime/src/libraries/Microsoft.Extensions.DependencyInjection/src/ServiceLookup/ServiceProviderEngineScope.cs

Line 12 in cb873bc

internal class ServiceProviderEngineScope : IServiceScope, IServiceProvider, IAsyncDisposable

Alternative 2. Introduce new class

This is similar to what's happening when creating ServiceProvider from IServiceCollection.

namespace Microsoft.Extensions.DependencyInjection
{
    public abstract class ServiceScope : IServiceScope, IAsyncDisposable
    {
        public abstract IServiceProvider ServiceProvider { get; }
        public abstract void Dispose();
        public abstract ValueTask DisposeAsync();
    }

    public static class ServiceProviderScopeExtensions
    {
        public static ServiceScope CreateServiceScope(this IServiceProvider provider) { }
    }
}

Same here, no need to wrap in most cases if we let ServiceProviderEngineScope inherit from ServiceScope.

[davidfowl]

Alternative 1 would return null if the underlying scope doesn't implement the interface? Remember our container is a single implementation, there are many others that would need to implement this new interface. That means it would return null if the implementation doesn't?

The latter is fine but requires an additional allocation for those providers that don't derive from our new base class.

[bjorkstromm]

Alternative 1 would wrap the scope in an internal class (implenting the interface) instead of returning null, thus would also require an additional allocation. So both alternatives are similar.

Question. If the new interface (or class) and extension method would reside in Microsoft.Extensions.DependencyInjection, would it affect other implementations? Quickly checking, the other implementations depend on Microsoft.Extensions.DependencyInjection.Abstractions. How is the root container for e.g. Autofac implementation created when using with ASP.NET Core? Does it still rely on Microsoft.Extensions.DependencyInjection when creating the container (service provider)?

Would it make sense to add the new interface and extensions method to Microsoft.Extensions.DependencyInjection.Abstractions instead and obsolete the the old extension instead? This would also require a new interface for replacing IServiceScopeFactory.

[davidfowl]

This keeps coming up so I think we should go with a modified version of alternative 2:

public static class ServiceScopeExtensions
{
    public static AsyncServiceScope CreateAsyncServiceScope(this IServiceProvider serviceProvider)
    {
        return new AsyncServiceScope(serviceProvider.CreateScope());
    }
}

public struct AsyncServiceScope : IServiceScope, IAsyncDisposable
{
    private readonly IServiceScope _serviceScope;

    public AsyncServiceScope(IServiceScope serviceScope)
    {
        _serviceScope = serviceScope;
    }

    public IServiceProvider ServiceProvider => _serviceScope.ServiceProvider;

    public void Dispose()
    {
        _serviceScope.Dispose();
    }

    public ValueTask DisposeAsync()
    {
        if (_serviceScope is IAsyncDisposable ad)
        {
            return ad.DisposeAsync();
        }
        _serviceScope.Dispose();
        return ValueTask.CompletedTask;
    }
}

@maryamariyan we should take this to one API review. The change is tiny but we need to agree on the name.

[bartonjs]

Video

• We renamed CreateAsyncServiceScope to CreateAsyncScope to better match the existing name
• We moved it into the extension type for the existing method to avoid making a new type

public partial class ServiceProviderServiceExtensions
{
    public static AsyncServiceScope CreateAsyncScope(this IServiceProvider serviceProvider)
    {
        return new AsyncServiceScope(serviceProvider.CreateScope());
    }
}

public struct AsyncServiceScope : IServiceScope, IAsyncDisposable
{
    private readonly IServiceScope _serviceScope;

    public AsyncServiceScope(IServiceScope serviceScope)
    {
        _serviceScope = serviceScope;
    }

    public IServiceProvider ServiceProvider => _serviceScope.ServiceProvider;

    public void Dispose()
    {
        _serviceScope.Dispose();
    }

    public ValueTask DisposeAsync()
    {
        if (_serviceScope is IAsyncDisposable ad)
        {
            return ad.DisposeAsync();
        }
        _serviceScope.Dispose();
        return ValueTask.CompletedTask;
    }
}

[davidfowl]

@bjorkstromm Are you interested in sending a pull request for this API?

[bjorkstromm]

@bjorkstromm Are you interested in sending a pull request for this API?

Sure! It would be my pleasure.