-
Notifications
You must be signed in to change notification settings - Fork 317
Add spec test verifying that services maintain registration order #416
Conversation
Currently this passes with our default container. It fails as follows with Autofac
|
We could test this more thoroughly by ensuring that things work the same way with constructor injection as it does when I don't think it's necessary to cover that much however. If we did, we should change every specification test to ensure that changing these hopefully orthogonal variables doesn't cause them to fail. |
This looks good to me so far. |
|
@@ -158,6 +158,33 @@ public void MultipleServiceCanBeIEnumerableResolved() | |||
} | |||
|
|||
[Fact] | |||
public void RegistrationOrderIsPreservedWhenServicesAreIEnumerableResolved() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's another expectation we have with our DI system viz when requesting a single service, the most recently registered service wins. Could you add a test for that scenario as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the expectations that I point out is already tested in #379 (comment)
- 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.
May be offtop, but due the multiple asserts in spec tests, it would be good to add assert messages at least, to quickly indicate what's failing. For instance, in DisposingScopeDisposesService, too many things can fail, and it is hard to find what without debug. |
|
d42d90f
to
6b94c82
Compare
|
||
var provider = CreateServiceProvider(collection); | ||
|
||
collection.Reverse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really reversing the collection? ServiceCollection
only implements IList<ServiceDescriptor>
and AFAIK that doesn't have a Reverse
method.
That means this probably is the Enumerable.Reverse
LINQ method, which doesn't alter the original source, but returns a new IEnumerable<ServiceDescriptor>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's calling List<T>.Reverse
since ServiceCollection : List<ServiceDescriptor>
. This should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, it's this https://github.com/aspnet/DependencyInjection/blob/dev/src/Microsoft.Extensions.DependencyInjection.Specification.Tests/ServiceCollection.cs not the one in Microsoft.Extensions.DependencyInjection
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #419 to rename the one in the test specification.
Meeting this requirement is proving to be a real PITA. We support registration sources being added to the container during the registration process and add several built-in ones by default as well. These registration sources can dynamically provide registrations at runtime if they are able to. Autofac capabilities such as resolving The source registrations could come from other registration sources, parent lifetime scopes, or even other registration sources in parent lifetime scopes. A user can also add new registrations or registration sources while creating a new lifetime scope. Combine this with the fact that the user can choose whether or not a particular registration should become the default for when a single instance is requested and the permutations are significant. Just managing the default registration across the lifetime scopes and registration sources is a challenge in itself. Determining what order a registration was added to the container only works within a limit subset of the possible types of registrations. After that the behaviour is too unpredictable for a user to make any sense out of, and in most cases is simply out of their and our control. We can't predict exactly what registrations we might be able to piece together based on explicit user registrations combined with the built-in and user-defined registration sources. At this stage simply abandoning the adaptor altogether is looking like a real possibility. You have forced every container to behave exactly the same as your own even though it only supports a minimal set of behaviour. That sounds like it should make things easier but it actually doesn't when you start to consider the more advanced capabilities that other containers support. Adding more features to your container will also make the situation worse and further reduce the number of containers that can conform. The overarching issue is trying to make all containers behave exactly the same regardless of their functionality. I got the impression from @halter73 in his comment above (#379 (comment)) that he had already figured out that forcing a connection between the order a registration is added and what happens at the time of resolution was indeed asking too much. You even had an issue raised around this very problem (aspnet/Options#150) after regressing from a better position in earlier versions of ASP.NET Core and the original MVC and Web API frameworks. The disappointing part is that this was ignored simply to meet the ship date. It really annoyed me seeing the pressure that people were putting on the team to release, instead of worrying about whether or not what was going to be released would be the best foundation possible for the next decade of .NET. |
I am looking at the exact same thing for LightInject and while doable, this is going to hurt, a LOT. More and more expected behaviour is being baked into the default implementation and that is also part of the contract that every adapter needs to confirm to. The very fact that not even Autofac can meet the expectations here, proves more than anything that this abstraction is going down the wrong path. We had a similar discussion in this issue. |
Ok there's been sufficient feedback from DI authors about this that I'd like to discuss some alternatives (purely for discussions sake, not committing to anything yet). If we removed all ability to replace the DI container for ASP.NET Core all up, what would 3rd party DI containers do instead? Would we basically resort to having container specific composite roots? One for SignalR, one for MVC, one for middleware without the ability to interop with any of the services (SimpleInject has a CrossWire function to make this work)? |
Is it really necessary to remove DI replacement altogether though? I assume this requirement is in place to prevent the app behavior from changing when you change DI system, but if I use say, Autofac, I think it would be reasonable for Autofacs rules to apply here, even if that changes the behavior of my app. Maybe its worth reiterating why this specific requirement is in place and what the implications would be if it was removed? |
@davidfowl This might be a dumb question, but for the folks like me who don't know the backstory:
|
@davidfowl I’m really pleased that you are joining the discussion and keeping an open mind. No doubt the community will see that as a positive thing. I think the problem is broader than just whether or not the container is replaced in ASP.NET Core, as general availability of the DI abstraction and the associated default container implementation has spread the issue further. The pain we are feeling here with ASP.NET Core is going to be felt in other places too, as the DI abstraction and default container (this needs a cool name) were written to be general purpose and reusable. For example, I noticed Microsoft Orleans already started using the new abstraction for its DI support. They aren’t currently falling back to the default container when no service provider is returned and are instead just using If they decided not to add the appropriate hook for grain activation outside of the DI abstraction, then non-conforming containers are simply locked out. Based on their current usage of the service provider even a container that had limited conformance would almost certainly meet their needs, but that doesn’t matter because they are free to expand their usage at any time. Using the abstraction seems like overkill for their current requirements, but because it’s available and appears to be a simple way of getting broad container support for basically no effort, the temptation to use it is just too great. If that’s already happening inside Microsoft there is definitely no chance of stopping this in the broader community. This is a pattern that I suspect will continue to repeat itself. When the conforming container issue was first raised you made it clear that the approach wasn’t going to change, so we strapped ourselves in for the ride, figuring that would be better than being left behind. Even with the current situation my views haven’t really changed: I don’t like the conforming container, but I also don’t want Autofac to be one of those containers locked out every time someone decides to use the abstraction, be it with ASP.NET Core or any other framework. Unless you are willing to make a radical shift immediately after RTM then options are a bit limited. Because the current DI packages encourage all services to be registered in a single container through a single registration API, even if ASP.NET Core takes a different approach with separate registrations and containers (with cross wiring as @dotnetjunkie describes it), how would others be stopped from using what is already available? It’s definitely a difficult situation. I’m not sure how many containers have actually attempted to create an adaptor at this stage, and whether or not they have actually run through the specification tests. Regardless of what happens next, working hard on reducing the number of requirements in the specification would be a good thing. |
@cottsak With previous DI interfaces such as the dependency resolver interfaces in MVC and Web API, the frameworks weren’t using a container internally as ASP.NET Core is now. When a service was required the framework would first check the container for an implementation, and if one wasn’t available, would fall back to a hard coded default service. Now all services are resolved from either the default container or the 3rd party container. That means that the services registered through the registration API that is part of the new DI abstraction, are passed over to the 3rd party container to be registered with it (currently 145 services in a basic Web API application). Unfortunately, that means that any implicit behaviours the default container infers from the registrations must be matched in the 3rd party container. For example, a subsequent registration for a service will replace any existing ones as the default. Also, any behaviour that the default container has needs to be matched by the 3rd party container because the framework is now relying on that behaviour internally, regardless of whether the default or 3rd party container is being used. Each behaviour that ASP.NET Core relied on internally from its own container bled directly through the contract and became part of the specification. It's worth noting that the previous DI interfaces also had their problems, but because there was no shared registration API or default container, they were fewer than what we are seeing now. I remember the dependency resolver interface in Web API added support for scoping, which meant that the containers didn't have to try and find their own hooks for the creation and release of scopes, but then they decided to start caching services returned from the container breaking the user configured scoping of the services being resolved. Hopefully that clears things up a bit for those that are just joining the thread now. |
@alexmg Yeh that does help a bit. So the "no MS container" and default impls really made the 3rd party containers possible in the first gen aspnet. Is that right? And now the built in container is leaking it's internal behaviour outside the interface contracts due to the internal wiring expectations, forcing all 3rd parties to adhere to this behaviour too. Problem is that this internal behaviour is incompatible with many features of different 3rd party containers. Am I getting it right? |
@cottsak That's essentially it. The way default services were handled and the absence of a default container certainly made it easier for a container to conform with what was required from the dependency resolver interfaces. There were also less assumptions about what was returned via that interface. For example, ordering was made explicit were required, such as through the Order property on filters. This made it easier in the earlier generation frameworks. The expectation of common behaviour across all containers, with the internal container driving those expectations, is now making it harder. |
Because the way we chose to replace the container was by making sure there is a single container for both framework and application, this solves some problems we've had in the past. See http://forums.asp.net/post/5708079.aspx for more details (though we did drop container chaining because it proved too hard to implement everywhere).
We do ctor injection where possible but that's the tip of the iceberg. What gets injected and how we expect that to behave is a different story, for e.g.:
Calling out the specific behaviors and what components use them would make it easy to see why things were needed. The spec tests try to enumerate the things we rely on. This happened organically and we didn't do enough to restrict ourselves really. Throughout the course of the 3 years developing this product we drastically changed the design of the DI system. @aL3891 It doesn't need to be removed, but replacing it means you pass the DI spec tests. One of the main complaints is that container authors could choose to integrate in different ways other than conforming to our entire spec (which is what simple injector does). The downside is that it's a much rougher experience when you want to get services that the framework provides.
I absolutely disagree that we force a registration API on you or that you are unable to take advantage of container specific behavior. That's just untrue. What we have at the lowest level is metadata that describes service bindings (you know this, you own the autofac adapter...). This is the one part of the conforming container argument that isn't right. You can use the container API to do all of the wire up and get the container specific behavior and services (like
Right, we're not going to move away from having this library or built in DI in ASP.NET because it's actually goodness. The way SimpleInjector wants to interop is very different and I'm mostly asking if other DI authors want to take that approach. My opinion is that people won't use 3rd party containers if the integration isn't seamless and I find that approach less than ideal so here we are discussing alternatives... |
I actually said, "the current DI packages encourage all services to be registered in a single container through a single registration API". I don't disagree at all that it's possible to use the Autofac registration API directly for registering your own services. This is exactly what I have been doing and putting in sample code to help reinforce the fact that it is indeed possible and not a constraint. The point was more about the fact that your registration API is what people are seeing most, and when using that they don't get the full benefit of what Autofac has to offer, such as adding a registration and allowing the existing default service to be preserved.
It is these expected behaviours that are causing the issues for containers attempting to build adaptors. In our case the expectation that services returned via IEnumerable are always in the order they were registered. A particular issue when those services aren't registered explicitly. In aspnet/Options#150 it's the fact that we aren't behaving exactly the same as your internal container that is causing headaches. The most concerning thing was that you decided not to fix it using approaches from the past that were know to work, and instead added more conformance requirements for every other container to meet. No doubt a decision based mostly on meeting a ship date (not something I'm holding you personally responsible for BTW).
I mentioned above that we want to work with the current system because ASP.NET Core won't be the only framework using this abstraction. There won't be any alternatives unless all frameworks using the abstraction are some how forced to allow for such an alternative. I'm not sure at the moment how that could be achieved starting from where things stand now. I share the same concern about seamless integration and that's why I suggested making the specification easier to conform to would be a good starting point. That definitely won't keep everyone happy from a design principles perspective but it's better than doing nothing and having good containers drop out of the ecosystem, or forcing users to switch to a different container when using a framework other than ASP.NET Core. |
As a consumer I actually do like the abstractions for DI in general. I love Autofac, especially the dynamic stuff where you can do things like generate an interface implementation on the fly, that's super powerful and I've used it a lot. But a lot of times registration is also a lot simpler and advanced DI frameworks like Autofac can be a lot to take in. I see a lot of value in being able to get up quickly and be able to switch out the DI for a more advanced one without going though the motions of changing all the registration code. That said, i do think these implicit requirements are a bit strong. It feels like enforcing a very strict behavior to achieve something more general, like that a previously registered component is replaced by one registered later. (I don't know all the details though) For me at least it would be ok if the app would potentially behave a little different when switching DI if that also meant I had more control. To me it seems like if things end up in the wrong order in a way that causes a problem, that's up to me to solve with my registration code. There might be times where people actually do want to change that order for what ever reason. So tl;dr; I'm in favor of dropping the hard requirements on certain behavior and let the app sort that out if it becomes a problem (possibly using the DI systems own api). Documenting the order dependent cases would also go a long way. |
I agree that we should make the specification as easy as possible to conform to. There is a balance we have to strike though. Even though framework code shouldn't need to rely on esoteric IoC features that would create undue burdens in third-party containers to support, some things are just too good to give up. @davidfowl mentioned a few such as open generics and ASP.NET Core (or Project K) has required I already brought this up in autofac/Autofac#755, but it seems to me that the reason that it is hard for Autofac to preserve order is because it also needs to support the older requirement of overriding services described in the service collection with ones registered later. IMHO this older requirement is far less important. I don't see any reason for a framework to resolve the same service type singulalry and via I'll admit that I don't entirely understand how registrations sources make it more difficult for Autofac to meet the ordering requirement as brought up several times in autofac/Autofac#755. It seems to me at the point the tl;dr: Let's loosen the specifications in an upcoming release, but instead of removing the ordering requirement which is super awesome and useful, let's remove the service replacement via |
@halter73 Why does it has to be registration order as opposed to order/priority that the DI system thinks components should be used in though? I'm sure library authors are willing to implement that if the benefits are strong enough, but maybe those benefits can be illustrated more clearly? |
So @davidfowl you're answer to the conforming container is another container? It sounds like "we're not going to move. if you don't like our container, use another one beside it; but we're keeping ours". Am I wrong? And since it seems obvious that two containers is worse than the [single] conforming one, you're essentially forcing folks to capitulate to your way since no one wants the alternative. Can we expect some cooperation or not? |
That's not my answer to anything but we're never going to get rid of the built in container. The only thing changing is to make sure we can reduce the amount of implicit behaviors and requirements needed for ASP.NET (and other frameworks that choose to use this abstraction) can work. For containers that don't want to replace the built in one, we'll have the other approach which is replacing the appropriate composition roots (which if you read this entire thread again is what @dotnetjunkie wants). |
I guess @davidfowl's comments above makes sense. It's a bit from both worlds. It pleases me that the @aspnet team at least listens on both ends of the spectrum of it's community and tries to find a middle way. That's the best you can do. |
DLDR: There is an internal container that 3rd party containers need to "conform" to in order to have a single container approach. There is a 2 container approach for non-conforming containers. Seems legit. Bonus: the fewer requirements needed to be considered a "conforming container" - the easier it will be for 3rd party containers to conform in a single container solution. |
Having contributed a tiny bit to LightInject, and being familiar with that code, I can tell you that the ordering requirement without an explicit interface contract is a poor design and will be very hard to support for some containers. Of course, this has all been said already. What I haven't seen proposed yet is actually making that part of the contract. Instead of imposing an invisible implementation, isn't is possible to support additional service registration methods that allow a priority to be passed in, something like That would make it explicit that conforming containers would need to support some kind of service priority mechanism. I'm not saying that will be a cakewalk for all container maintainers, but at least it isn't a totally hidden implementation detail. |
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:
As for the a specific list of things we think can be candidates for potential removal:
|
@mattnischan It was briefly mentioned by @halter73 a bit ago:
|
Looks like some good ideas there... 👍
What about being more specific WRT cardinality up-front? In Nancy, we have a What about a It might be harder to do with ASP.NET than with Nancy, since you might not have control over cardinality up-front, though. |
I don't see why that makes it easier TBH and it also is a bit more restrictive as we'd need to support registering the 3 things you can always register (type, instance, delegate). The contract could be as simple as "if there's multiple things with the same service type, it can't be resolved as a single". |
DryIoc provides such failing behavior by default, to be more deterministic. In addition, exception message lists the registrations found. Then container may have an opt-in setting to UseLastRegistration, or First, or whatever matching the condition. |
@davidfowl If you had a way to express which registration (that's part of a collection) was the "main" registration, the container adapter could do what it needs to do. I don't see a way of doing that using a |
I've looked into DI container feature usage in ASP.NET Core applications, there is some data here Legend:
Click on service names to get stack traces |
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:
Since other DI containers behave differently in this respect, the DI abstraction should make the exact behavior undetermined. 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:
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. |
@pakrym Looking at your generated stack traces I'm dismayed to see the prevalence of ServiceLocator patterns. Most of them are in application initialization; understandable ( but However |
@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.
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 Having to maintain a specific order for resolved However is there anything in the spec that indicates whether the order should be evaluated across the entire resolve set... i.e.
|
Let's move this discussion to #433 so we don't have to comment on a closed pull request. |
Start of work for #379
@davidfowl @muratg