Spec out implicit behaviors of the DI container and service collection

[davidfowl]

Here's a good starting point:

simpleinjector/SimpleInjector#41 (comment)

We should capture all of these in our specification tests.

/cc @lodejard @Eilon @pranavkm

[khellang]

That would be nice! 👍 Microsoft.Extensions.DependencyInjection.Specification.Tests is already a good starting point, but I still think there might be "undocumented" expected behaviors.

[dadhi]

May be not a right place, but it is quite explicit and exemplary how DryIoc setups DI adapter:

if (container.ScopeContext != null)
    throw new ArgumentException(
        "Adapted container uses ambient scope context which is not supported by AspNetCore DI.");

var adapter = container.With(rules => rules
    .With(FactoryMethod.ConstructorWithResolvableArguments)
    .WithFactorySelector(Rules.SelectLastRegisteredFactory())
    .WithTrackingDisposableTransients()
    .WithImplicitRootOpenScope());

[davidfowl]

@dadhi it's definitely the right place 😄

[Eilon]

Moving to backlog.

[tillig]

On the Autofac implementation we're definitely getting questions (as outlined in that SimpleInjector issue) about whether there's a guaranteed order of things coming back from enumerable/collection services. Here's the latest one.

Is there a prescribed (or required) order for enumerables returned from the service provider?

[PleasantD]

Out of curiosity, what parts of the ASP.NET Core framework explicitly depend on the DI container and registration system?

I know that the WebHostBuilder is responsible for creating the registration collection and provides services like IApplicationLifetime and IHostingEnvironment as injectable. But environment configuration is context and could be provided within the existing context objects (IApplicationBuilder and HttpContext).

MVC registers services. But does it need to? Does it still have factory classes for constructing controllers and views? It seems odd to me that to use a piece of middleware I need to remember to register it's services in a different location.

Having a single DI container does seem to provide a way to implicitly make middleware "DI friendly", but this too could be solved by providing a middleware factory and allowing middleware to be constructed via that, again bypassing the conforming container. In fact, Autofac provided this kind of middleware factory for OWIN.

[Eilon]

@halter73 can you respond to some of these questions?

[pranavkm]

AFAIK, IConfigureOptions \ OptionsManager is the only collection that we use as IEnumerable<T> but doesn't have any explicit ordering to it. Might be useful to figure out if we rely on the setups to be run in a specific order.

[tillig]

Even with IConfigureOptions we've found people expect them to run in order even if that's not specified.

[lodejard]

Yes - enumerables should be returned in the order added - so fwk services should appear before app servces in an enumerable because the fwk services are what is setting the default.

So the expectation is correct. There was a set of base test cases in the DI repo that could be run against the built-in container, and could be used by third-party container adapters to validate correctness of the assumptions. Was the project with the abstract base test cases deleted at some point? It would have been the place where the assumptions were codified.

[tillig]

The tests are still there but there is no test around the ordering of enumerables, which is where some of the challenge is arising.

[PleasantD]

IStartupFilter instances need to be ordered. Otherwise the internally registered filter that registers the per-request-scope middleware winds up out of order (and potentially last instead of first).

Actually, I have another issue with this: because the framework is registering services, how do I register services before it? Say I want to replace the per-request-scope generating filter. It is written to do nothing if a scope has already been set, but I can't register a filter before it, so it will always be the first middleware run.

[halter73]

Regarding the small set of specifications brought up in simpleinjector/SimpleInjector#41 (comment). Most of these already have specification tests:

• A registration for a closed generic type of some service will always be resolved before a registration of an open generic type for that same type. In other words, the open registration will always act as a fallback registration when there is no applicable closed registration. This fallback behavior will occur regardless of the order the two registrations appear in the list.
  • ClosedServicesPreferredOverOpenGenericServices
• Registrations for collections are handled by appending registrations for the service type one after the other to the registrations list.
  • I’m not exactly sure what this is getting at. This seems to be describing implementation details not behavior.
• If a collection is requested using GetRequiredServices(), the underlying container is expected to return all registered implementations of T, even if the IList only contains a single registration for T.
  • SingleServiceCanBeIEnumerableResolved
  • NonexistentServiceCanBeIEnumerableResolved
• If a single instance is requested using GetRequiredService(), the container is expected to return the last registration for T, in case there are multiple registrations for T.
  • LastServiceReplacesPreviousServices
• If a collection is requested using GetRequiredServices() or injected into a constructor, the order in which the registrations are made in the IList is preserved.
  • This is the issue we’re discussing now which isn’t in the specification tests. It should be added by Add spec test verifying that services maintain registration order #416.
• When creating scopes using the IServiceScopeFactory abstraction, scopes are expected to be explicit and should be able to resolve from an outer scope, even if that outer scope is called after an inner scope is created. This means, for example, that scopes are not expected to work in the same ambient way as the TransactionScope class does. This behavior is specified in the NestedScopedServiceCanBeResolvedWithNoFallbackProvider test.
  • This test still exists: NestedScopedServiceCanBeResolvedWithNoFallbackProvider

[halter73]

Off the top of the head, here are some other things that we do that we probably don’t test:

1. Lazy initialization of services including singletons. I’d be very surprised if there was a container that didn’t do this, but I doubt there’s a specification test. I’m also not sure we rely on this, but it’s probably important for perf.
2. The order we initialize previously uninitialized constructor injected services in. I’m not sure what that order is, and I really hope nothing actually depends on that.
3. I’m not sure how comprehensive our constructor matching specification tests are. You used to require only having one public constructer. We’ve loosened that and added the ServiceContainerPicksConstructorWithLongestMatches test, but I’m not sure that is sufficient to ensure that third-part containers match our constructor matching logic exactly.
4. I’ll try to think of more.

@davidfowl Do any of these warrant adding a specification test for?

[PleasantD]

Registrations for collections are handled by appending registrations for the service type one after the other to the registrations list.

This refers to the fact that some DI containers cannot adapt the IServiceCollections list of services into correct registrations. Some containers do not make assumptions about the cardinality of service registrations and demand explicit declaration of cardinality. You haven't just defined a specification for a service container, but also for a service registration system.

---

Even with all of these "Conforming container" specifications there is still a big impedance mismatch between your container's (and service registration system's) features and the features available on the DI container of MY choice.

The more I look at this problem the more I wonder what exactly is being solved by having IServiceProvider available at all. From a library design point. we don't need it, we just need to implement classes following core DI principals and favouring constructor injection. From a framework design point it forces other frameworks to conform to your container and limit themselves. From an application point of view it encourages Service Locator.

[halter73]

@PleasantD Given multiple registrations for a given service type in an IServiceCollection, containers are free to register those services with an explicit declarations of cardinality.

I personally don't know whether asking containers to maintain ordering is a bridge too far, but I do know that ordering is a "feature" that we have come to rely on in several places in ASP.NET. Initially, we planned to fix our usage not to rely on ordering (and I wish we had/could), but there's a risk that we wouldn't catch everything prior to 1.0.

As for exposing IServiceProviders in the HttpContext and in other places: something needs to resolve the first service. It can't be constructor injection all the way down. If people want to use the service locator pattern, they'll find a way.

[halter73]

@smitpatel Said he could help investigate whether Autofac works as expected with EF. @ajcvickers told me EF should not rely on service ordering.

Edit: added not

[smitpatel]

replacing
var serviceProvider = services.BuildServiceProvider());
with

var builder = new ContainerBuilder();
builder.Populate(services);
var container = builder.Build();

var serviceProvider = container.Resolve<IServiceProvider>();

works as expected in EF functional tests.

[Tornhoof]

While you're extending the Spec Tests, are you sure that the following code snippet from DependencyInjectionSpecificationTests.cs is correct?

public static TheoryData ServiceContainerPicksConstructorWithLongestMatchesData
{
var fakeService = new FakeService();
var multipleService = new FakeService();
var factoryService = new TransientFactoryService();
var scopedService = new FakeService();
  [...]
  new ServiceCollection()
.AddSingleton<IFakeService>(fakeService)
.AddSingleton<IFakeMultipleService>(multipleService)
.AddSingleton<IFakeScopedService>(scopedService)
.AddSingleton<IFactoryService>(factoryService),
new TypeWithSupersetConstructors(multipleService, factoryService, fakeService, scopedService)
}

according to the IFake* names the fourth parameter is a scopedService, but all 4 services are added as singletons. Either the name is wrong or the AddSingleton is wrong. According to the names I would have also expected other implementations.

[pranavkm]

@Tornhoof seems like IFakeScopedService is a misnomer in this case.

[Tragetaschen]

In my Kestrel issue (ObjectDisposedException during shutdown) I noticed that Autofac as DI container doesn't provide an IServiceProvider that implements IDisposable which Hosting would call during shutdown.

I have replaced return container.Resolve<IServiceProvider>(); from the example in the docs with the following code:

// var container = containerBuilder.Build();
var result = new disposingServiceProvider();
result.ServiceProvider = container.Resolve<IServiceProvider>();
result.Container = container;
return result;

with

private class disposingServiceProvider : IServiceProvider, IDisposable
{
    public IServiceProvider ServiceProvider;
    public IContainer Container;

    public object GetService(Type serviceType)
    {
        return ServiceProvider.GetService(serviceType);
    }

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

And this seems to fix my shutdown issues.

[Eilon]

Need to add a spec test for the service provider being disposable to capture that behavior.

[muratg]

@davidfowl let's discuss this when you're around

[Eilon]

We are closing this issue because no further action is planned for this issue. If you still have any issues or questions, please log a new issue with any additional details that you have.