simpleinjector / SimpleInjector

An easy, flexible, and fast Dependency Injection library that promotes best practice to steer developers towards the pit of success.
https://simpleinjector.org
MIT License
1.22k stars 152 forks source link

Add support for IServiceCollection and IServiceScope #180

Closed qujck closed 8 years ago

qujck commented 8 years ago

ASP.NET Core defines the abstractions IServiceCollection and IServiceScope to complement, and work in conjunction with, the IServiceProvider abstraction. Just as IServiceProvider defines a mechanism for retrieving object instances, IServiceCollection defines a mechanism for capturing the abstraction to implementation mapping and the desired instance lifetime and IServiceScope holds references to object instances for each lifetime instance.

I have read many well formed arguments detailing the problems implementing the abstractions and I have heard requests from our users to do so anyway. IMO the current Integration package is not fit for purpose as it does not seamlessly integrate object instances resolved by Simple Injector and those same objects resolved by the default container.

IServiceCollection contains the definitions for registrations and should easily map to Simple Injector equivalents (especially since the addition of DefaultScopedLifestyle). IServiceScope is an abstraction for explicit lifetime management that does not seamlessly fit with Simple Injectors Ambient Scopes but this, in itself, is not a reason to not attempt a Simple Injector implementation. For example, a SimpleInjectorServiceScope could be resolved within the Scope it is instantiated within and registered for disposal along with its owning Scope - doing so would directly link the lifetime of each IServiceScope instance with the Scope instance it represents.

These 2 new abstractions are part of ASP.NET Core, EF Core and will become a requirement for other modern libraries and frameworks in the future. I propose we add first class support for both IServiceCollection and IServiceScope to ensure that Simple Injector remains a simple option for the future. We may not be able to achieve 100% compatibility with the expectations made by the ASP.NET team but we can achieve 100% compatibility with the expectations of our users: a Simple Injector for ASP.NET Core.

Reonekot commented 8 years ago

Any news on this? :+1:

We have started a rewrite of our in-house "template" project on ASP.NET Core MVC RC2 and I have gotten the default VS template of MVC working with Simple Injector and to some extend our own project following: https://simpleinjector.readthedocs.io/en/latest/aspnetintegration.html. (With Verify() already catching one life-type mismatch error in our rather simple case... :) )

But we have quite a few components developed internally, which follows the pattern for "registration" that the ASP.NET uses - fx.: public static IServiceCollection AddNavigation(this IServiceCollection services);

Which underneath looks like the default MVC functions:

public static IServiceCollection AddNavigation(this IServiceCollection services)
{
    OptionsServiceCollectionExtensions.AddOptions(services);
    Microsoft.Extensions.DependencyInjection.Extensions.ServiceCollectionExtensions
        .TryAdd(services, ServiceDescriptor.Singleton<Navigation, Navigation>());
    return services;
}

I can't really make these components depend on Simple Injector, so I'm not sure how to handle this without an IServiceCollection extension from Simple Injector. (Besides CrossWire the components, which seems kind of fragile, as I need to check the extension methods (ie. AddNavigation()) to know which services to CrossWire. (Or depend of Verify().)

dotnetjunkie commented 8 years ago

Why do you need those components to be registered both in MVC and in Simple Injector?

Reonekot commented 8 years ago

Currently I'm not sure I do - but I anticipate that we'll have components (internal Nuget packages) that will follow the above pattern, which should be registered with SimpleInjector and I'm not sure how to handle this, without having the components depend on SimpleInjector. I don't want to force a dependency on SimpleInjector on the rest of company, if they want to use the same components. Maybe I'm missing an obvious way to do that...

I have the same problem with SaasKit.Multitenancy, which has a similar registration model: https://github.com/saaskit/saaskit/blob/master/src/SaasKit.Multitenancy/MultitenancyServiceCollectionExtensions.cs

I'm not 100% sure, but I assume I can get away with just adding this to the SimpleInjector container, but then I need to make either a SaasKit.Multitenancy.SimpleInjector package (I assume SaasKit has a StructureMap package for this reason, but haven't looked at it) or copy the registration code into my own project. (Which might miss updates made on the official version of AddMultitenancy().)

So this isn't about registering the services in both containers, but about that the components shouldn't care and currently are exposing IServiceCollection to wire up the components.

dotnetjunkie commented 8 years ago

and I'm not sure how to handle this, without having the components depend on SimpleInjector.

NuGet packages are reusable libraries and for those you should follow the DI-friendly library guidelines.

Thing is, Simple Injector is simply not compatible with the registration model of the .NET Core DI abstraction (and its impossible to make it compatible without loosing much of the behavior and features that make Simple Injector unique). As discussed before, trying to force this square pig in a round hole, best is to keep the worlds separated. Reusable libraries should be DI friendly, and (according to the Dependency Inversion Principle) application code should be decoupled from external libraries. This the same practice that you can apply to decouple Simple Injector from your application and external libraries. In fact, the existance of the CrossWire extension method in Simple Injector's extension package is an anti-pattern; the reason we have it is to make it easier for developers to get started and to work with external libraries that force registration within the .NET Core abstractions.

Reonekot commented 8 years ago

The problem is, IMHO, that a lot of Nuget packages will probably follow the same path we and SaasKit.Multitenancy has taken and follow how the MVC team did it. These packages will sometime need to be added to the application (SI) container, which will be a "pain" with SI.

I do follow your reasoning on this and again really appreciate your thoughts and guidance on the subject (and feedback to the MVC team around your concerns) - but that doesn't make it easier to work with some times unfortunately. :)

I'll drop our dependency on the Multitenancy packages, as I can't see how I can't that to work cleanly currently. (And we pretty much have our own system in place anyway.)

dotnetjunkie commented 8 years ago

These packages will sometime need to be added to the application (SI) container, which will be a "pain" with SI

I don't really see the pain here. The SaasKit NuGet package has choosen to use the .NET Core DI library as its dependency mechanism. This is an internal concern to this package. Your application code should typically not have a dependency on SaasKit at all (because of DIP), which means that you don't need to register its dependencies in Simple Injector. You will instead create an adapter based on an interface that is known by (and tailored for) your application. This adapter will call into the core DI container to get the Saas component, and this adapter should be registered in Simple Injector and injected into your application components. But even if you don't use an adapter, your application will typically just use at most a few dependencies of such external library, so you will only have to 'cross wire' those few dependencies. Never the whole SaasKit dependency graph.

That said, there will unfortunately always be NuGet packages that don't follow those good practices and chose to integrate and dependent so deeply on the .NET Core DI abstraction that they will cause pain for you and even other developers of other containers (note that the adapter is so poorly specified that in fact nobody really knows how to build an adapter).

There's unfortunately not much the Simple Injector Contributors can do about this; it's impossible for us to create a completely working adapter without ripping out every compelling feature that makes Simple Injector different. You can't blame us for this.

We can and will however, try to educate developers in how to work around most of the problems they face with integrating with .NET core by improving their application design. But there's no doubt in my mind that this won't solve all their problems; there will be libraries that simply will be a PITA to integrate with because of their dependency with the core container.

Reonekot commented 8 years ago

Well anything but Install-Package + AddXxx() in Statup.cs is what I mean by "pain" here - maybe barrier would have been a better word. :) We'll see how it plays out in the coming months and hopefully it won't be a big problem. I removed our dependency on Multitenancy, which really just made our application simpler / more testable anyway.

Again, I really do appreciate your opinion, guidance and education on this which is why I trust the project so much.

dotnetjunkie commented 8 years ago

I close this issue, because it's impossibl to create an adapter for Simple Injector that is compatible with the new .NET Core DI system. We will have to solve this problem by educating application developers on how to deal with these issues mainly by improving their application design.

ronnyek commented 7 years ago

I know this has been long closed... but is this really true? Autofac has implementations of IServiceProvider and IServiceScopeProvider. I've got simple injector injecting controllers and viewhelpers via activators... but the inability to just know that wherever things are registered, places that expect it will get it.

Eg, asp.net injects middleware whilst instantiating in app.UseMiddleware(), and also, I've seen lots of code that gets called in services.Add...., things that are expected to be there while constructing controllers etc. For each of these I have to decompile, find sources and manually register them bypassing the library's default method of registering services.

The whole thing seems ok on the surface, but really I don't see a great way of NOT copying all the registrations from IServiceCollection, into simple injector and expecting things to work.

dotnetjunkie commented 7 years ago

Hi @ronnyek,

Let me start by saying that although you might think that Autofac has a fully working adapter for ASP.NET Core, the fact is that they don't. Many of the DI maintainers have been discussing with Microsoft for a long time, where the Autofac maintainers actually explained that the current ASP.NET DI abstraction is incompatible Autofac beyond their ability to fix. This has led to the initiative from Microsoft to introduce breaking changes in both the DI container and the abstraction around this, and although some changes have been introduced in v1.1, these changes still don't allow the Autofac developers to create a completely working adapter. The Autofac maintainers are still waiting for Microsoft to introduce more breaking changes in .NET Core v1.2 which would allow them to fix their adapter. So although you might think that Autofac has an implementation (they do have a Nuget package), that adapter is in beta phase, is not fully compatible with Microsoft's abstraction and will break on users and 3rd party libraries that are integrating with ASP.NET Core.

And this leads me to the core of the problem: this IServiceProvider abstraction is broken beyond repair. In the end Microsoft might move into a model where Autofac is able to comply, they have haven't even tried to address the issues that I stated that are causing problems for Simple Injector. But if they do, they will immediately break the adapter again for Autofac, or StructureMap, or... any other library.

As I understand, you are trying to duplicate many registrations that are done in AddXXX methods, but this is not the way to go. I understand why you are trying to do this. It is because Microsoft keeps showing us –only- examples of the design that depends on adapters. This has mislead you to believe that to be able to configure Simple Injector, you need to duplicate those registrations in Simple Injector, but this will lead you into the wrong direction -as you probably already noticed.

When working with Simple Injector (or any time you plug-in any container without using an adapter for ASP.NET Core), you should keep your application registrations separate from the registration that ASP.NET Core does (and 3rd party components do) inside the built-in container. In other words, you don’t replace the built-in container, you let it be and let ASP.NET Core use it internally for its own registrations. Besides this you use Simple Injector and you register your own application components in Simple Injector. This keeps your registrations simple, and prevents you from having to duplicate an enormous amount of registrations that you are not interested in (since it’s just ASP.NET Core internals). There are some rare cases (usually just a couple) where your application code needs to take a dependency on a framework component/abstraction. Only in such case you ‘cross-wire’ such component. There are multiple ways to do this, but you can, for instance, register a delegate into Simple Injector that fetches that component from the built-in container. Do note however that you want to keep this coupling to an absolute minimum. This keeps your application configuration as isolated as possible from all the framework internals. This prevents you from having to decompile, or look at source code, and try to mimic the registrations that are done in the built-in container.

About registering middleware, I find ASP.NET’s model of registering middleware really unfortunate, because it forces application components to be constructed with runtime data which is an anti-pattern.

Instead, by making just a small adjustment to your middleware class, you can still hook your middleware trivially with Simple Injector. No strings attached.

In the previous two links I already referenced this repository. That repository is part of a discussion with Microsoft here that’s about making it easier for containers like Simple Injector, Ninject, Unity and Castle Windsor that don’t have an adapter for ASP.NET Core. One of the links points at the Startup.cs of the Simple Injector sample project, which tries to show how one would integrate Simple Injector into ASP.NET Core. You might want to take a good look at it. The sample project shows a very basic, but completely working application with Simple Injector as application container. I hope this code proves that it is actually possible (and not hard at all) to plug-in Simple Injector into the mix, but admittedly, you might view this thing from a different angle. This angle is something that is currently a bit hidden, mainly because all the examples that Microsoft shows you take a completely different angle.

I hope these examples allow you to move forward. If you have any questions about how to integrate Simple Injector for specific use cases, feel free to post a new issue here on Github. We will try to help you a.s.a.p.

ronnyek commented 7 years ago

I see what you are saying and actually like the implementation you came up with. I do however have one question... I see your example has the crosswiring... in that example should having a controller parameter of IViewBufferScope then automatically be injected? Seems like this implementation should work, but making a IViewBufferScope a dependency of the construction of HomeController, the controller fails to activate.

dotnetjunkie commented 7 years ago

in that example should having a controller parameter of IViewBufferScope then automatically be injected?

Note that the registration does not register an IViewBufferScope but an Func<IViewBufferScope> instead. Func<IViewBufferScope> is a completely different registration and this means you should inject a Func<IViewBufferScope> into your controller. By invoking that delegate you can aquire the IViewBufferScope instance.

Reason for the registration of a Func<IViewBufferScope> instead of an IViewBufferScope directly is to demonstrate that you nor your application container has (nor should have) any knowledge about the lifetime of such framework abstraction. Reason is that lifestyles of framework abstractions might change (due to changes in the framework or due to 3rd party libraries that you use). Depending on such a volatile thing as lifestyle without the possibility of Simple Injector protecting you from such misconfiguration is a dangerous thing and should generally be prevented.

ronnyek commented 7 years ago

That's fine... trying to inject that function throws because its saying http context accessor doesn't have a http context. Maybe I just need to massage that get request service method

dotnetjunkie commented 7 years ago

This should work. Look at the code I just committed. When I run the sample application, it works just fine.