Changes to DependencyInjection requirements

[davidfowl]

Moving the discussion from #416

The tricky part with supporting DI is that not all of the things can be codified into the interface definition. That would make it easier for sure but some of it is just impossible.

Right now we're researching a couple of things:

• We're looking to see which components depend on specific features of the DI container (via some automation) and as part of it, we want to see if we can automate any of it.
• We're looking at @tillig's suggestions (like writing a chaos container, one of our devs already wrote a second one)
• We're trying to see if we build ordering on top via another service type (playing around with IOrdered<T>)
• We're looking at the implementations of other adapters to see what things are impossible to implement (We could use everyone's help/feedback on this one).
• As a result of the previous exercise, we're also looking to see what other requirements we can relax (last registration wins, closed generics falling back to open generics etc)

As for the a specific list of things we think can be candidates for potential removal:

• Ordering - We're trying to build a service on top with some that can potentially handle this.
• Last registration wins - We'll likely change the API around service collection to make sure that if multiple registrations make it into the container, resolving it as a single service fails.

[davidfowl]

From @dotnetjunkie

We're looking at the implementations of other adapters to see what things are impossible to implement (We could use everyone's help/feedback on this one).

There is a lot of implicit behavior in the adapter that will have to be explicitly unspecified. In other words, the contract should explicitly state that in certain conditions an adapter is free to behave as it chooses. For instance, an adapter is allowed to throw an exception.

When considering the current version of Simple Injector, the behavior as specified by the abstraction should be undetermined in the following scenarios:

• A captive dependency is registered. Simple Injector will throw an exception when an object graph contains captive dependencies.
• A disposable transient component is registered. Simple Injector throws an exception during verification when a component that implements IDisposable is registered as transient.
• A component with ambiguous lifestyles is registered. Simple Injector throws an exception during verification when it finds multiple registrations for the same components (e.g. using different interfaces) that are registered with different lifestyles.
• Multiple registrations for the same component and same lifestyle, but with different interfaces. The current version of Simple Injector throws an exception during verification when there are multiple registrations for the same component (e.g. using different interfaces) for the same lifestyle, when the registrations will result in having multiple instances of that component within the same scope. In a future version, we are likely to automatically merge these registrations into one to prevent this (much like Unity already does), but this means that it should be undetermined how the system behaves when this happens. i.e. the options are: registrations could be merged, the container can resolve multiple instances per scope, or an exception can be thrown.
• Registration of types with multiple constructors. Simple Injector throws an exception when a type with multiple constructors is registered.
• Registration of open generic types that overlap with closed generic registrations. Simple Injector does not treat open generic types as fallback, but will instead throws an exception where registrations overlap.
• Resolving transient components from ApplicationServices. The built-in container seems to resolve transient components as singleton, when requested from the ApplicationServices property. Simple Injector would resolve them as transient.
• Resolving scoped components from ApplicationServices. Simple Injector does not allow resolving scoped instances outside the context of an active scope. It throws an exception instead.

Besides the above list of behavior that has to become explicitly unspecified, there are other behaviors of the abstraction that I’m currently unsure of how it should be specified for the current version of Simple Injector to conform. These included:

• Registration and resolving of collections. In its API, Simple Injector explicitly separates the registration of collections from one-to-one mappings. Letting an adapter to Simple Injector register everything as collection (with possibly one element) makes users lose out on one of Simple Injector verification capabilities. Disallowing a single ‘default’ to be resolved when there are multiple registrations (as proposed earlier in this thread) doesn’t change this fact.
• Scopes are explicit. In Simple Injector, scopes are ambient (just like TransactionScope) and the services are always resolves from the container; never directly from a scope instance.

This is what I’ve been able to come up with at this point and that holds for the current version of Simple Injector. As already noted above, there is already a plan to change some behavior of Simple Injector in the next major version. I will likely have missed some things, so I will update this comment as I discover more items.

[davidfowl]

From @slaneyrw

@dotnetjunkie Looking at the perspective of a Unity user I echo most of your points, and explicitly call out the Cardinality problem.

I have been a user of the Unity container for many years and i have a fork of the GitHub that I'm using to convert it to netstandard1.6. When i was trying to build an Adapter for IServiceCollection/IServiceProvider I immediately came across the issue of how to distinguish between a replacement registration and a multiple registration.

TryAdd is easy as you check for an existing registration and add if not present
Add becomes complicated... Do I replace the existing registration or add another one. Without understanding of the intent of a registration at the time of registration the container will have to hedge both bets.

It become vastly more complicated with IServiceScope. It doesn't make sense to me have registrations for any type that is designed for a single instance when resolved to have both a Scope lifetime and singleton/transient lifetime. In Unity a Scoped lifetime would be implemented using the HierarchicalLifetimeManager. If a type was resolved from the composition root ( i.e. IServiceProvider ) or from the scope (IServiceScope ? ) then the same build plan would be used but the instances will be stored in their respective owner container. I would not be possible to store two separate registrations for two different implementation without creating a new registration in the scope container when it is created ( or black magic to "migrate" a parent registration marked as "Scope" ). Once again, having this distinction for a single instance resolve does not make sense to me.

Without any cadinality metadata at registration I will be forced to maintain two sets of registrations. If Add is used then I will have to add a default registration AND a named registration. If Add is used multiple times for the same interface/concrete then last in wins for the default registration.

Having to maintain a specific order for resolved IEnumerable<T> should be easy to accomplish by adding a post resolve build policy for those interfaces what have Order metadata, like IOptions<T>.

However is there anything in the spec that indicates whether the order should be evaluated across the entire resolve set... i.e.

1. Resolve both composition root AND scope, then ordering
2. Resolving and ordering from scope and composition root respectively, in isolation, then concatentating, and in what order
3. Resolving IEnumerable<T> from IServiceScope should ignore composition root

[davidfowl]

The problem with having cardinality defined up front for everything is that you lose the ability to distribute where registrations come from. There might be some default implementations in the box and the user might provide some. I'm not sure how that would work if you were forced to declare everything up front in a single call. I'm not even sure why that matters to be honest.

We're currently going to change things so that an IServiceCollection with multiple entries of the same service type indicate the service is IEnumerable<T>. There's some gotcha's though:

• To support optional services in constructor injection some components use IEnumerable<T> and check for empty as a pattern. We didn't plan to break that but that will conflict with features like this:

Registration and resolving of collections. In its API, Simple Injector explicitly separates the registration of collections from one-to-one mappings. Letting an adapter to Simple Injector register everything as collection (with possibly one element) makes users lose out on one of Simple Injector verification capabilities. Disallowing a single ‘default’ to be resolved when there are multiple registrations (as proposed earlier in this thread) doesn’t change this fact.

We'd need to take a look at places where we do this and see if we can break that pattern.

@slaneyrw about unity

1. Resolve both composition root AND scope, then ordering
   1. Resolving and ordering from scope and composition root respectively, in isolation, then concatentating, and in what order
   2. Resolving IEnumerable from IServiceScope should ignore composition root

Always preserve registration order (currently) using what. As I said before, we're looking at relaxing that requirement. You need to look at both the "composition root" and the "IServiceScope" depending on the lifetimes of the resolved services.

[mattnischan]

To support optional services in constructor injection some components use IEnumerable and check for empty as a pattern.

That's crazy evil. Collection resolution is pretty important, especially when you have strategy pattern type services like something resembling an IEnumerable<IFilter> or IEnumerable<IRule>. You might want all of them to throw your data at.

Optional services should still be able to be injected by either expanding the expression generator to mock up a default class for that service interface or by injecting default(T). That would make it more compatible with most other containers.

[dotnetjunkie]

or by injecting default(T). That would make it more compatible with most other containers.

Think again. Most containers disallow injecting null references into constructors. Constructor arguments should never be optional.

[mattnischan]

Not a huge fan of optional constructor injection either, honestly. Just looking for the most easily cross container compatible answer without resorting to something like IOptional<T> and a static Optional<T>.Empty.

[slaneyrw]

Almost finished a ServiceCollection adapter for Unity and came across a few nasties.

1. There is no way of getting a IServiceProvider from IServiceCollection, so I tried using your BuildServiceProvider() extension. However your implementation uses hardcodes a ServiceProvider instance passing all the ServiceDescriptors, instead of resolving it or asking ServiceCollection. Rounding tripped registrations via ServiceDescriptor means I'll lose information.
2. TryAdd extension iterates through all the ServiceDescriptor in the ServiceCollection. Maybe IServiceCollection should have an IsRegistered ( ServiceDescriptor ) => bool

I think both of the aspects should be on the ServiceCollection interface so implementors can override the behaviour

[davidfowl]

1. You're supposed to create your own service provider implementation over the list of service descriptors. Look at the other adapters for an example.
2. Why? Also changing the interface is breaking so that's not going to be happening in the short term. We can look in a future release.

[slaneyrw]

Yes, I am creating my own service provider, but NOT over the original ServiceDescriptors. The ServiceDescriptor is too simplistic for all but the basic mapping type to implementation. What about property injection, method invocation, interface and virtual method interception. These are all tools in the arsenal that are not supported using ServiceDescriptor.

Asking the ServiceCollection for all the descriptors and giving it to a ServiceProvider feels wrong to me. It believe should be the responsibility of the ServiceCollection to initialise the ServiceProvider.

So at the moment I have to return an IServiceProvider from the ConfigureServices method instead of relying on MVC to create it. I hope you don't remove that capability! Maybe the default MVC template should be changed to make it more explicit and return IServiceProvider from ConfigureServices.

[davidfowl]

Yes, I am creating my own service provider, but NOT over the original ServiceDescriptors. The ServiceDescriptor is too simplistic for all but the basic mapping type to implementation. What about property injection, method invocation, interface and virtual method interception. These are all tools in the arsenal that are not supported using ServiceDescriptor.

I don't know why that affects any of those features.

Asking the ServiceCollection for all the descriptors and giving it to a ServiceProvider feels wrong to me. It believe should be the responsibility of the ServiceCollection to initialise the ServiceProvider.

The service collection is just metadata for your actual container implementation. The service collection isn't your container registration API. So its the container implementors job to take the "metadata" and turn it into container specific API calls. The container implementor also needs to return an IServiceProvider that calls through the container implementation.

So at the moment I have to return an IServiceProvider from the ConfigureServices method instead of relying on MVC to create it.

That's how it works and will continue to work.

I hope you don't remove that capability! Maybe the default MVC template should be changed to make it more explicit and return IServiceProvider from ConfigureServices.

We landed on leaving it implicit for the default case.

[davidfowl]

We've started on some of the proposed changes here:

#437

• Preserving ordering is no longer a requirement - There are new APIs and a new type IOrdered<T> to get a list of ordered things described via services.AddOrdered<T> calls. Specific containers can choose to override the default implementation and do something more efficient.
• Explicit IEnumerable<T> services - If you ask for IEnumerable<T> without saying the service is IEnumerable<T> it will fail.
• New service descriptor (breaking change) implementation - ServiceDescriptor is the base class and there's derived ServiceDescriptor(s) for each of the service types (Instance, Type, Factory, Enumerable etc).

/cc @tillig @alexmg @nblumhardt @seesharper @jeremydmiller @khellang

[tillig]

I'm a bit torn on the explicit IEnumerable<T> services. I can see enforcing that adding multiple services of the same type to a ServiceCollection may fail unless you specify that you're adding to a member of a collection (intentionally), but the resolution of the IEnumerable<T> may need to be unspecified.

For example, Autofac intentionally returns an empty IEnumerable<T> if you don't have any members registered so things like message processors that want a list of handlers won't just fail out of hand - you won't have to check for null-or-empty, just empty. And if you only register one thing, great, you get an enumerable of one.

We couldn't even really add anything to the service provider to track which explicit IEnumerable<T> contributors have been registered because we'd need to push the tracking down into Autofac (so things registered by assembly scanning or whatever are also tracked)... and then it'd break out of the box behavior.

Long story too long - I think it's cool to enforce the different registration mechanism as part of ServiceCollection but not so much for the actual backing IServiceProvider doing the resolving.

[davidfowl]

Long story too long - I think it's cool to enforce the different registration mechanism as part of ServiceCollection but not so much for the actual backing IServiceProvider doing the resolving.

We have to, it's one of the things that ends up being hard to implement in some containers. We can't enforce IEnumerable<T> is ok to resolve if the service wasn't registered as IEnumerable<T>, that can be an autofac thing but we won't rely on it anymore. Also, we need to call out the behavior of GetService vs ctor injection so that it's mostly consistent (where possible).

[tillig]

I think it's fine if you don't rely on it as long as it's not also wrong if it isn't explicitly disallowed.

[khellang]

@davidfowl Just thought about something... Does ctor injection go through the ISupportRequiredServices path? Or does it fail on ctor selection?

[khellang]

Uh, never mind. Just realized that it all depends on the call at the composition root and what the individual container does from the on.

[davidfowl]

@tillig That's always a problem right? What should "the spec" recommend? If any framework relies on it then it won't work with whatever container doesn't support it.

[tillig]

@davidfowl Yeah, that is the problem. Of course, if part of this is in an attempt to loosen the requirements, then saying "behavior is unspecified and shouldn't be counted on" is loosening the requirements and I'm for it. Adding the notion that "if a component isn't registered via the ServiceCollection with an explicit bent toward IEnumerable<T> then the backing container should also explicitly fail to resolve the IEnumerable<T>" is actually a new requirement and I'm not keen on that.

Just off the top of my head, that one would actually be harder to implement on Autofac with more breaking changes than the ordering requirement that was previously handled.

So... "don't count on it because it's intentionally unspecified" == cool; "don't count on it and container must implement a behavior to disallow it" == not so cool.

[slaneyrw]

Explicit IEnumerable services - If you ask for IEnumerable without saying the service is IEnumerable it will fail.
New service descriptor (breaking change) implementation - ServiceDescriptor is the base class and there's derived ServiceDescriptor(s) for each of the service types (Instance, Type, Factory, Enumerable etc).

For what it's worth, makes me happy. Should be a lot easier to implement container adaptors.
Will the new ServiceDescriptors be sealed ?
What is the expected behaviour if a new ServiceDescriptor subclass is presented to ServiceProvider ?

[davidfowl]

Will the new ServiceDescriptors be sealed ?

@slaneyrw sealed? No.

What is the expected behaviour if a new ServiceDescriptor subclass is presented to ServiceProvider ?

Explode.

[davidfowl]

@tillig

So... "don't count on it because it's intentionally unspecified" == cool; "don't count on it and container must implement a behavior to disallow it" == not so cool.

That's fine. Our spec tests won't require it to throw but our default container will throw. At the same time, we need a way to make sure that frameworks don't rely on things like this. It's fine for applications to rely on it though.

[davidfowl]

/cc @dadhi @seesharper

[davidfowl]

Some questions for the thread as we go further with the current plan:

• What should the behavior be if the adapter is passed an unknown service descriptor? I mentioned earlier it should blow up but I'm having second thoughts on that one. We're implementing IOrdered<T> as a specific ServiceDescriptor that keeps track of the registration order of things. It means if somebody adds a new derived service descriptor, it'll be silently ignored by container adapters so error detection is harder. On the other hand, allowing different kinds of service descriptors lets different containers light up behaviors that others may not be able to handle. It would be nice to have a way for containers to replace behaviors built on top with native implementations.
• For things that resolve as IEnumerable<T>, can the lifetimes be mixed? In the current prototype we only support transient services in IEnumerable<T>. How do other containers handle this and what are the side effects?

[tillig]

I think I like the ability to have custom derived service descriptors as an extension mechanism, though admittedly I don't have a completely thought out use case right now. Maybe the ability to register decorator types? Just brainstorming.

I think lifetimes should be mixable in IEnumerable<T>.

By way of example, in Autofac if you register a singleton, request scope, and transient into the same container and resolve the enumerable from the request scope, you'll get all three.

For example, day you have some message processor and you register handlers for messages. One handler may be expensive to instantiate or otherwise initialize but is stateless, so you can optimize by registering it as singleton. Another handler may be cheaper or stateful, so it's transient. When you resolve all the handlers you still want both.

[ajcvickers]

Just a note about EF. EF relies on collections of scoped services being constructor injected as IEnumerable<T>. So are far as I can tell these changes will break EF. It will also break all our providers since provider resolution also relies on this.

Question about versioning: since these are very significant breaking changes shouldn't it bump the version of D.I. to 2.0? Otherwise NuGet is going to make it very easy for people to get combinations of packages that don't work together.

[davidfowl]

Just a note about EF. EF relies on collections of scoped services being constructor injected as IEnumerable. So are far as I can tell these changes will break EF. It will also break all our providers since provider resolution also relies on this.

What's the lifetime of the thing it's being injected into? Is that itself scoped? Would be good to look at some concrete examples.

Question about versioning: since these are very significant breaking changes shouldn't it bump the version of D.I. to 2.0?

This was one of the breaking changes we were going to take for 1.1 because of the impact on 3rd party containers.

Otherwise NuGet is going to make it very easy for people to get combinations of packages that don't work together.

Calling it 2.0 doesn't make it any harder for nuget to give you invalid combinations of packages. NuGet will roll you forward if any package depends on 2.0.

[dotnetjunkie]

Since I can't update this comment, here's a little update about Simple Injector and IEnumerable<T>:

In Simple Injector IEnumerable<T> dependencies are streams. What this means is that those components will only be created when the IEnumerable<T> is iterated and components will be resolved according their registered lifestyle. What this means is that a transient component will be created everytime the collection is iterated. In other words, the IEnumerable<T> dependency itself is a singleton that has factory-like behavior. It can be safely injected into a singleton component, even though its elements might be transient, scoped or singleton. So obviously Simple Injector allows mixed lifestyles within a single IEnumerable<T>, but the behavioral difference goes even further.

Since other DI containers behave differently in this respect, the DI abstraction should make the exact behavior undetermined.

[naasking]

+1 to closed generics overriding an open generic instance.

[pakrym]

/cc @muratg

[dotnetjunkie]

Here are my suggestions for improving integration for non-conformers.

[dotnetjunkie]

Did anything discussed above end up in the just released ASP.NET Core 1.1?

[khellang]

I think (at least) the following was fixed in 1.1 (with #430):

• A captive dependency is registered. (This might not be until resolution though. Not sure)
• Resolving transient components from ApplicationServices.
• Resolving scoped components from ApplicationServices.

[nvivo]

Any news on removing the tracking of transient disposables outside of scopes?

[slaneyrw]

Is this still to be done? I cannot find any subclasses of ServiceDescriptor on the Dev branch

[ErikSchierboom]

This looks great. I would love to have improved integration when using SimpleInjector, preferrably such that SimpleInjector would be able to inject dependencies as action parameters (which is a brilliant feature).

By the way, SimpleInjector 4 has just been released. Don't know if that helps.

[slaneyrw]

Just looking at the API for v2, looks like none of the service descriptor differentiation still hasn't surfaced

Also new issues with bad constructor discovery

1. LoggerFactory now has 2 constructors of length 2 and Unity is throwing an ambigious constructor exception.
2. ConsoleLoggerProvider's longest constructor takes a Func<string, LogLevel, bool> for the first argument, which is not injectable.
3. DebuggerLoggerProvider has the same problem
4. HttpContextFactory longest constructor takes an IHttpContextAccessor, which is not registered in the default service collection. AppInsights also has this pattern.

[davidfowl]

Realistically, we can't change anything here. The best bet for making things like unity, castle and other containers work would be to go to @dotnetjunkie approach outlined here aspnet/Mvc#5403.

TL;DR if the container implementation can't conform then there's another approach that includes adapting various composition roots. I think this is the best path forward to making containers with completely incompatible semantics work. It does mean that both containers need to be bridged but that's basically how things used to work in the older days (pre-ASP.NET Core).

[slaneyrw]

Just had a look at @dotnetjunkie 's repo and I think there are major problems trying to run both side-by-side. I prefer that the container OWNS the composition root, and that is how I've got unity to work

Using the IServiceProviderFactory approach make the most architectural sense to me. I ended up creating a subclass of ServiceDescriptor where I can add all the Unity specific registration metadata and handle the registration in the CreateBuilder phase. I've created extensions off IServiceCollection to build the new descriptor, now I can support all the more complicated DI aspects, like Property and Method injection, parallel dependency graphs, as well as interception mechanisms ( transparent caching )

I've solved the issues with the constructor selection by switching the constructor selector policy to match the default DI's behaviour, although I can't work out how it makes a distinction between LoggerFactory's 2 constructors - I think there is some undocumented behaviour in there.

Cardinality hints ( or lack there of ) is still an issue, I was really hoping the v2 API was going to support this, but I can work around it.

[aspnet-hello]

This issue was moved to dotnet/aspnetcore#2346