Issue with disposable singletons

[peter-perot]

Let's assume we have the following types:

public interface IMyInterface
{
}

public sealed class MyClass : IMyInterface, IDisposable
{
  public void Dispose()
  {
    Console.WriteLine("Disposing");
  }
}

When using Microsoft ASP.NET DependencyInjection the following code works correctly:

private static void MicrosoftDI_1()
{
  var sc = new ServiceCollection();
  var externallyOwned = new MyClass();

  sc.AddSingleton<IMyInterface>(externallyOwned);

  var sp = sc.BuildServiceProvider();

  var myObj = sp.GetRequiredService<IMyInterface>();

  ((IDisposable)sp).Dispose(); // myObj is _not_ disposed
}

The externally owned singleton is not disposed - that's correct.

The following code works correctly, too:

private static void MicrosoftDI_2()
{
  var sc = new ServiceCollection();

  sc.AddSingleton<IMyInterface>(_ => new MyClass());

  var sp = sc.BuildServiceProvider();

  var myObj = sp.GetRequiredService<IMyInterface>();

  ((IDisposable)sp).Dispose(); // myObj is disposed
}

Here the singleton myObj is disposed, because it is created inside of a lamdba factory function -
correct so far.

Now let's see what Autofac using the Microsoft ASP.NET DependencyInjection adapter is doing:

private static void AutofacDI_1()
{
  var sc = new ServiceCollection();
  var externallyOwned = new MyClass();

  sc.AddSingleton<IMyInterface>(externallyOwned);

  var cb = new ContainerBuilder();
  cb.Populate(sc);
  var ctnr = cb.Build();
  var sp = new AutofacServiceProvider(ctnr);

  var myObj = sp.GetRequiredService<IMyInterface>();

  ctnr.Dispose(); // myObj is disposed - this is not correct!
}

The externally owned singleton is disposed on container disposal - this is not correct.

When using a lambda factory function, Autofac is working correctly:

private static void AutofacDI_2()
{
  var sc = new ServiceCollection();

  sc.AddSingleton<IMyInterface>(_ => new MyClass());

  var cb = new ContainerBuilder();
  cb.Populate(sc);
  var ctnr = cb.Build();
  var sp = new AutofacServiceProvider(ctnr);

  var myObj = sp.GetRequiredService<IMyInterface>();

  ctnr.Dispose(); // myObj is disposed
}

The singleton is disposed as expected.

Used versions:

<?xml version="1.0" encoding="utf-8"?>
<packages>
  <package id="Autofac" version="4.6.0" targetFramework="net46" />
  <package id="Autofac.Extensions.DependencyInjection" version="4.1.0" targetFramework="net46" />
  <package id="Microsoft.Extensions.DependencyInjection" version="1.1.1" targetFramework="net46" />
  <package id="Microsoft.Extensions.DependencyInjection.Abstractions" version="1.1.1" targetFramework="net46" />
</packages>

[tillig]

Duplicate of autofac/Autofac#780

[peter-perot]

@tillig I don't think this is a duplicate of autofac/Autofac#780

Autofac.Extensions.DependencyInjection is an adapter for Autofac in order to act as a substitute for Microsoft ASP.NET DependencyInjection when using the conforming container pattern.

The problem here is: when you are using the original DI container from Microsoft the behavior is different compared to the implementation of the adapter.

I think the issue can simply be resolved by checking if the descriptor of the singleton service contains a ImplementationInstance. If this is the case the singleton should be registered as externally owned at the Autofac container builder.

[tillig]

It's a duplicate because when you're using the Autofac adapter the thing fulfilling requests is not the Autofac adapter it's the Autofac container. The shell implementations of IServiceProvider and so on in the Autofac.Extensions.DepdencyInjection are just proxies to Autofac functions. The issue you're pointing out is an issue of whether we're properly handling disposal of provided instances in core Autofac (which is issue autofac/Autofac#780) - it really doesn't have anything to do with Autofac.Extensions.DependencyInjection or Microsoft.Extensions.DependencyInjection.

To be clear, there will definitely be behavior differences between the Autofac DI container and the Microsoft DI container. We're not trying to behave identically - if you choose to use Autofac as your backing container, you're also choosing the Autofac behaviors. The difference in behavior you found is one of probably many; it just happens that we've been having a discussion about how Autofac should handle it (and, by virtue of that, how the Autofac.Extensions.DependencyInjection adapter will behave).

What you've stumbled into inadvertently is a fairly huge discussion where Microsoft has created a container (rather than just abstractions/interfaces) and developers somewhat assume if they switch backing containers that behavior will be identical - that there's a conforming container here. You can see it in issues and blog entries like this where many different container owners are concerned about that:

• Spec out implicit behaviors of the DI container and service collection aspnet/DependencyInjection#379
• Ambient scopes not supported by all DI's aspnet/DependencyInjection#334
• https://simpleinjector.org/blog/2016/06/whats-wrong-with-the-asp-net-core-di-abstraction/

[peter-perot]

Interesting - the behaviour described here is really quirky (the difference between registration at container level as opposed to registration at scope level with a different behavior on disposal).

So I understand your point.

But do you have an idea how to keep the externally owned instance (see my initial example) from getting disposed when the container is disposed? In Autofac I could use ExternallyOwned() on registration but what will I do when using the adapter?

[tillig]

The workaround now is to register directly with Autofac instead of the adapter.

For the adapter, we need to solve the behavior in the core container. We are using RegisterInstance in the adapter (which is correct) but it's not behaving consistently. Once figure out what core Autofac should do, that's what the adapter will also do. The adapter will behave like Autofac.

If you want to follow autofac/Autofac#780, that's where this is being discussed.

[HeikoOehme69]

From my perspective the issue comes from a gap in the overall design of dependency injection containers. That is not only an autofac issue. Often dependency containers work as a service registries and service factories at once. From the client side it is not possible to decide who is responsible for disposing an object. This detail is provided by the registration but not forwarded appropriately to the client. Or did I oversee something?

[peter-perot]

From my perspective the issue comes from a gap in the overall design of dependency injection containers. That is not only an autofac issue. Often dependency containers work as a service registries and service factories at once. From the client side it is not possible to decide who is responsible for disposing an object. This detail is provided by the registration but not forwarded appropriately to the client. Or did I oversee something?

Disposable services are typically registered as scoped services, so the container is responsible for disposing them. Therefore I think an injected service should never be disposed by the instance that get the service injected. Only services that are created by an (injected) service factory should be disposed by the code that uses the factory.

[julealgon]

The workaround now is to register directly with Autofac instead of the adapter.

This looks like a terrible workaround.

Why can't Autofac's implementation just check for the ImplementationInstance property in the ServiceDescriptor and if that is set, mark the object as "externally owned"? That would preserve the original behavior I'm pretty sure.

The ServiceDescriptor class has 3 distinct properties for the 3 "types" of injections supported by MEDI:
https://learn.microsoft.com/en-us/dotnet/api/microsoft.extensions.dependencyinjection.servicedescriptor?view=net-8.0#properties

• Factory registrations (ImplementationFactory)
• Type-based registrations (ImplementationType)
• "provided instance" registrations (ImplementationInstance)

The latter is clearly externally owned.

[alistairjevans]

We've had some discussion today over the contract of the Populate method. Part of the issue here is that there are a set of tests defined by the dotnet team that define what a "conforming container" is. That is to say, what behaviours/rules must a given DI provider have to ensure compatibility with the ASP.NET Core framework.

None of those conforming container tests currently specify/enforce that singleton instances registered by RegisterSingleton(instance) are or are not disposed by the container.

This behaviour of not disposing instances is therefore implicit (albeit documented) behaviour of MSDI. Note that this is the opposite of the Autofac defaults. There are other implicit behaviours as well that aren't in the conforming container tests.

All that being said, the contract of the Populate method really is that Populate should attempt to map, whereever possible, between the user expectation of the various MSDI Register* methods, and Autofac methods.

The user expectation here is that calling RegisterSingleton(instance) shouldn't result in the instance being disposed; in other words, the behaviour of RegisterSingleton(instance) maps to Register(instance).ExternallyOwned() in the Autofac world.

So, we now plan to make a change in this Autofac integration to map in that manner and make singleton provided instances ExternallyOwned.

Here's what we're going to do:

• Leave this issue open for feedback for a while, to elicit feedback and give a chance for people to counter what I've said above.
• Raise an issue in the dotnet runtime asking for the conforming container tests to get disposal behaviour tests.
• Finally, make the change and increment the major (breaking) change semver. This is fundamentally breaking since some users might actually rely on the current behaviour.

[julealgon]

Here's what we're going to do:

• Leave this issue open for feedback for a while, to elicit feedback and give a chance for people to counter what I've said above.
• Raise an issue in the dotnet runtime asking for the conforming container tests to get disposal behaviour tests.
• Finally, make the change and increment the major (breaking) change semver. This is fundamentally breaking since some users might actually rely on the current behaviour.

That's a really nice plan Alistair, agreed on all fronts. In particular, I'm surprised this behavior wasn't part of that set of tests on MS' side: it should definitely be there and I'm glad you are making a proposal to include it.

On a side note, you might want to remove the duplicate label on this one as it gets reopened.