Integration for Microsoft.Extensions.DependencyInjection (ASP.NET Core) #120
Comments
|
I moved the code into a repository. you can find it here. |
|
@cwe1ss sorry for not replying to your original post, I've had some personal things going on over the last couple of months which have really pulled me away from Castle. Firstly thanks for building the integration. Unfortunately I've done zero with ASP.NET Core and the new DI framework built into it so can't really comment as to the problems you've encountered. I assume you'd like Windsor to ship with this integration? You pasted links to code and problems, followed by "best regards", unfortunately it isn't going to get into Windsor without someone contributing the work. Your gist doesn't have a license, while your repo is licensed under the MIT license, contributions to Windsor must be licensed under the Apache 2 license. At this point in the lifecycle of ASP.NET/DNX/CLI/etc I think it is probably best if you maintain the integration until things settle down and stabilise. My focus at the moment is on Castle Core and getting it ported to .NET Core. Windsor will obviously come next, but at the moment the build system of Windsor is completely incompatible with project.json so our learnings from Castle Core will flow into fixing that up too along with porting to .NET Core so we can accept this integration without it being just for the .NET Framework. Thoughts, opinions, feedback? |
|
Hi @jonorossi, I don't know much about the Windsor internals, so I didn't want to invest too much time. Instead, we decided to remove Windsor and go with the integrated DI from Microsoft for now (albeit it has a lot less features). This means I don't really have a motivation to maintain the integration. So it's up to you to decide if and when to do something in this area! Feel free to fork my repository, in case you need the files for future reference. I will delete the repository in a few weeks, since I believe it's broken and therefore not really helpful for anyone except you. |
|
Hi @cwe1ss I'm using Castle Windsor in aspnet boilerplate (http://www.aspnetboilerplate.com/) and now want to move to AspNet Core. I faced to the same problem with you. It seems the biggest problem is the Scope implementation. |
|
No. As far as I understand them, windsor child containers are used at service registration time and not at resolve time. ( http://mikehadlow.blogspot.co.at/2010/05/10-advanced-windsor-tricks-13-use.html ). This could be wrong though - I don't know much about Windsor. |
|
Hmmm... yes, I'm creating some unit tests and it's like that. |
|
I'm investigating docs and source code of Windsor and I suppose we should implement a custom LifeStyleManager (https://github.com/castleproject/Windsor/blob/master/docs/lifestyles.md#custom-lifestyles). This manager should return the same instance for same scope and destroy all instances when IServiceScope.Dispose() is called by AspNet. |
|
I'm not sure if this will be enough. ASP.NET creates a scope (= a child ServiceProvider) per request. You can have transient and scoped services in such a scope so they have a different lifestyle (scoped = once per request). All of them have to be disposed on scope.Dispose(). but feel free to fork my repository and give it a go :) |
|
OK, I created an adapter library based on your solution and it's passing all unit tests: https://github.com/volosoft/castle-windsor-ms-adapter |
|
I guess the usage of ThreadStatic might be problematic - AFAIK it's not compatible with the new async/await world. You might have to store this in CallContext, AsyncLocal ? Also, what would happen if there were multiple child scopes within one request? |
|
Hi, I know ThreadStatic has problem with async/await. But I used it just while resolving dependencies and Windsor's resolving operation does not include async/await. Whole resolving progress is performed in a single thread (Windsor's resolve mechanism has also similar ThreadStatic mechanism) So, it should not be a problem. It supports multiple scope (unit tests also have such cases). Also, there is no child scope actually in MS's DI. There can be multiple scopes nested, but they don't have child/parent relation. While this does not effect it, I just wanted to notice. |
|
I suppose finally it's working. You can try it from nuget: https://github.com/volosoft/castle-windsor-ms-adapter#how-to-use But surely it not works for .NET Core, it working for full .NET Framework since .NET Core is not supported by Windsor yet. |
|
I won't pretend I know anything about how the DI container works in ASP.NET Core and I haven't looked at @hikalkan's implementation, however have you guys looked at how the current per web request lifestyle works (i.e. |
|
We can not use webrequestscopeaccesor, because Microsoft.Extensions.DependencyInjection's scope approach is different and related to an IServiceScope instance. |
@hikalkan I didn't mean to imply that you could use Scoped lifestyles in Windsor 3.0+ already implement the interfaces you've implemented yourself. You'll see the Using the Windsor scoped implementation should simply the implementation to very little, unless I'm missing something fundamental with how the ASP.NET Core DI works, which I may be as I can't find any documentation. Nancy's Windsor integration might help to show how they are using |
|
Hi, Surely, I opened Windsor's source code in my Visual Studio and investigated reated classes including WebRequestScopeAccessor for hours :) I also investigated source code of Microsoft.Extensions.DependencyInjection. CallContext can not work since AspNet Core's lifetime does not depend on callcontext. You can create arbitrary scope objects (IServiceScope), store and use them whenever you need. ThreadStatic is worse since it can not work with async pattern. Even there is a HttpContext, we don't rely on it. Also, Microsoft.Extensions.DependencyInjection package is independent from web pipeline. |
|
@hikalkan ah, I didn't realise the |
|
I would not want to use var myService = _serviceScope.ServiceProvider.GetService(typeof(MyService));myService instance and all dependencies and all their dependencies should be related to the _serviceScope instance and all they should be released when _serviceScope.Dispose() is called by AspNet. Without a threadstatic, how can I relate this serviceScope with the all objects resolved in this particular GetService method call? |
|
If you need something that works with TPL, check AsyncLocal<T>, or more error prone LogicalCallContext. |
|
threadstatic/async stuff is not a problem, believe me. Because, Windsor's Resolve code is single threaded (it even use ThreadStatic internally, it won't work if this was a problem). And also, https://github.com/volosoft/castle-windsor-ms-adapter works properly as I tested. But would be good to get some code review. |
|
Any news about this one? |
|
@Kralizek - Please see #283. There is a wealth of info there. I have not touched on this for a while because there were other things that took priority. I honestly don't believe supporting the abstractions for Microsoft.Extensions.DependencyInjection is viable because of impedance mismatches in the design. It leads to behaviours that are difficult to maintain. It has also earned Windsor the title of a non-conforming container. Not even sure what that means. I am planning to dust off the work I have been pursuing on this in the next few weeks. |
|
How is people supposed to use Castle Windsor in ASP.NET Core applications? Should we do all the built-in registrations again? Non supporting the de-facto standard in .NET (Core) will eventually mean pushing Castle Windsor into oblivion. And that would be sad. |
In a word yes. We simply don’t treat scopes and singletons as the same thing. I tried “conforming” this container almost a year ago but the impedance mismatch in life cycles alone make it impossible. I am still hopeful we can make this work. |
Curious, it solves the abuse that has already happened in hosting(and might be proliferated through all sorts of third party libraries). After falling victim to this myself I would like see some safety built into dependency injection. Would you reconsider? |
|
The thing is, today the effect is that you get different instances because there are different universes. I still haven't looked closely enough to see what is different about castle core's implementation that makes it a deal breaker. That isn't to say they isn't a problem but it's been there since the beginning so we just have to understand why it matters more now.
Maybe but the bar is extremely high for making changes to our default container. It's pretty much frozen unless something is outright broken (and wouldn't be a breaking change to fix). |
Are scopes universes also?
I would love to explore this a little more. Scopes are completely incompatible between our containers.
It always mattered. Just getting to it now.
Please bear with me. The least that can come out of this is that I will have a better understanding. I am incredibly interested in the CallSiteRuntimeResolver right now. Is there any way I could override the VisitSingleton method to use a static collection of resolved services? This would probably sort the problems in hosting out and also not be a breaking change. Hope you are open to discussing this. |
No. Scopes are very simple. The scoped IServiceProvider is just a disposable root for scoped and transient services. If something is declared as scoped, then it's a singleton within the scope container it gets resolved from (instance per lifetime scope in autofac speak).
public interface IThing { }
public class Thing : IThing, IDisposable
{
public void Dispose() => Console.WriteLine("Thing.Dispose()");
}
var sp = new ServiceCollection().AddScoped<IThing, Thing>()
.BuildServiceProvider();
using (var scope = sp.CreateScope())
{
var thing = scope.ServiceProvider.GetService<IThing>();
var thingAgain = scope.ServiceProvider.GetService<IThing>();
// These are the same instance since they were resolved from the same scope container
}When I say different universes I literally mean we create different instances of IServiceProvider: var universe1 = new ServiceCollection().AddSingleton<IFoo, Foo>().BuildServiceProvider();
var universe2 = new ServiceCollection().AddSingleton<IFoo, Foo>().BuildServiceProvider();Those are 2 different worlds even if they share the same service descriptors. This is what hosting does because they are stages.
When I say it didn't matter, what I meant is that you get different instances at each stage which is by design. It means if you add a singleton in the hosting container, you get 2 different versions because they are 2 different containers. That's the quirk. It surfaces when you resolve a service from the Startup constructor and then from the Configure method.
I don't see that as a viable option to change how the default service provider works to plug in a 3rd party container. As far as I'm concerned those are implementation details of the default container. |
Good explanation. In Windsor a service that is registered with a Singletons however are scope agnostic. I think this is a sticking point for me.
Two containers. Got it.
Stages of what? We have 2 containers here.
Why have N+1 containers? We tried parent/child containers, it exposed all sorts of problems on our end.
Yup but do new scopes yield new singletons? This is not right.
https://en.wikipedia.org/wiki/Singleton_pattern
Hope you understand that there are some fundamentally different things happening when it comes to memory management here. It affects downstream performance heavily if you keep reinstantiating singletons that pull config in the cloud. Hope you will reconsider this approach. |
One use case could be integration testing. You might have 1 container for services used by the test code, and 1 container for services used by the application code. So having 2 "singleton" instances would then make sense since you don't pollute the application environment with components registered for the test environment. |
This happens in aspnet/hosting so this is not test code. |
|
I believe we can fix this by passing the root scope to the call run-time resolver for singletons. |
They have to be, if they weren't, singletons would be the same as scoped. Put another way singletons are scoped within the root container.
If you look at how hosting wires things up there are 2 phases to Startup: public class Startup
{
public Startup(ILogger<Startup> logger) // Startup gets resolved from the first container (the hosting container)
{
}
public void ConfigureServices(IServiceCollection services) // This is the second container definition (the application container)
{
}
public void Configure(IApplicationBuilder app, ILogger<Startup> logger2) { } // We resolve services from the second container here (the application container)
}
It's not a new scope, its an entirely new container. There's the hosting container which is used to resolve services in the Startup constructor and there's the application container which uses the service collection provided in ConfigureServices to be created.
I'm not sure what you mean. |
But your call site run-time resolver says you resolve singletons as scoped. https://github.com/aspnet/DependencyInjection/blob/dev/src/DI/ServiceLookup/CallSiteRuntimeResolver.cs#L39 |
New scope = new container? |
No, a new scope is not a new container. We should stop conflating these things. This is a new container: var sp1 = new ServiceCollection()
.AddScoped<ScopedService>()
.BuildServiceProvider();
var sp2 = new ServiceCollection()
.AddSingleton<SingletonService>()
.BuildServiceProvider();sp1 and sp2 are different containers, they don't share service descriptors, they are independent of each other. They are both distinct top level root containers and can each create their own scopes independently.
Lets not discuss the implementation details of the default container. That just makes the conversation more confusing. Let me try to explain again:
Example: var sp = new ServiceCollection()
.AddSingleton<SingletonService>()
.BuildServiceProvider();
var singleton1 = sp.GetService<SingletonService>();
using (var scoped = sp.CreateScope())
{
var singleton2 = scoped.ServiceProvider.GetService<SingletonService>();
Assert.Same(singleton1, singleton2);
}In the above code snippet, the singleton object is the exact same object even though it singleton1 was resolved from the root container (sp) and singleton2 was resolved from the scoped container. Here's an example of scoped services: var sp = new ServiceCollection()
.AddScoped<ScopedService>()
.BuildServiceProvider();
using (var scoped = sp.CreateScope())
{
var scoped1 = scoped.ServiceProvider.GetService<ScopedService>();
var scoped2 = scoped.ServiceProvider.GetService<ScopedService>();
Assert.Same(scoped1, scoped2);
}
using (var scoped = sp.CreateScope())
{
// New instance
var scoped3 = scoped.ServiceProvider.GetService<ScopedService>();
var scoped4 = scoped.ServiceProvider.GetService<ScopedService>();
Assert.Same(scoped3, scoped4);
}
// Resolving a scoped service from the root container will throw by default in ASP.NET Core because
// ValidateScopes is on (a feature of the default container)
sp.GetService<ScopedService>(); |
Sorry for kicking in, but to make sure - so typical WebHost creation like creates and uses "the first container (the hosting container)" just to help in creating the That would explain some confusing things I've seen when I tried to setup some common logging for app startup and app runtime.. Ok, but then, once application starts, what happens to the first container? is it totally forgotten and left for GC to cleanup, or is it kept and later something other than Startup is resolved from it as well? |
@quetzalcoatl that's absolutely correct.
Yes, it's a PITA.
It sticks around until the WebHost is disposed but nothing else is ever resolved from it (it could really be disposed earlier). |
|
You have cleared up quite a bit, I managed to clear up quite a few things in my PR as a result. Disposables are working properly and I don't have any issues with singletons anymore. The last thing I need to look at is why enabling scope validation in the facility is triggering exceptions. Everything you have said so far is making perfect sense. I not sure what stopped us from conforming the first time, we had all sorts of problems making those tests pass but that was a while ago. I might take another look at this later on. Thanks for explaining. 👍 |
|
We are going ahead with this cross-wiring non-conforming thing for now (I do know there is a problem with scopes just not clear on why yet). I do think our next issue needs to be a conforming one. I am not going to try and address this here because this is an issue already has a long tail. Thank you for your input, look forward to to making this happen. I think we have cleared up most of the misconceptions I have had. Thank you for your input. If you can see something I missed @ PR: #394 please chime in. @generik0 this includes you. Latest artifacts are here: https://ci.appveyor.com/project/castleproject/windsor/build/0.0.423/artifacts |
|
If you need any more help or clarification let me know. Castle Windsor is a popular container and it would be huge if an adapter could be built. |
|
one of the reasons that was preventing me it seems that it's going to be possible now. i'm excited |
|
@hikalkan looking a your impl, it seems as though scoping is the hardest thing to implement right? A cursory glance tells me there's no built in unit of work style scope that isn't tied to some ambient state (call context, async local etc). I haven't looked but I'm also guessing there's no support for child containers or nested containers in windsor? This is the model that most closely matches the semantics of Microsoft.Extensions.DependencyInjection: |
We use async local for scoped ambient state here: https://github.com/castleproject/Windsor/blob/master/src/Castle.Windsor/MicroKernel/Lifestyle/Scoped/CallContextLifetimeScope.cs#L48
We have them: https://github.com/castleproject/Windsor/blob/master/src/Castle.Windsor/Windsor/IWindsorContainer.cs#L56 They behave wierdly in some cases. Wont pollute with detail. |
|
After looking at @hikalkan's code looks like there is ambient state for scopes happening here: https://github.com/volosoft/castle-windsor-ms-adapter/blob/master/src/Castle.Windsor.MsDependencyInjection/MsLifeTimeScope.cs#L32 |
Right, I was playing around with it last night and there seems to be a few gaps with the build in scope that https://github.com/volosoft/castle-windsor-ms-adapter/blob/master/src/Castle.Windsor.MsDependencyInjection/MsLifeTimeScope.cs#L32 tries to address:
It's using async local because there's no way to set enough state from the call site of the resolution itself so that you can get it back when the custom scope infrastructure calls you back. Here's my super basic example of one of the problems: using System;
using Castle.MicroKernel.Context;
using Castle.MicroKernel.Lifestyle;
using Castle.MicroKernel.Lifestyle.Scoped;
using Castle.MicroKernel.Registration;
using Castle.Windsor;
namespace windsor_tests
{
class Program
{
static void Main(string[] args)
{
var container = new WindsorContainer();
container.Register(Component.For<IFoo>().ImplementedBy<Foo>().LifestyleCustom<LifetimeObjectScopeLifestyleManager>());
// This object represents the lifetime of resolved services. Anything disposable component associated with this lifetimte object
// should be disposed when it is disposed
using (var lifetime = new DefaultLifetimeScope())
{
container.Resolve<IFoo>();
}
}
}
public class LifetimeObjectScopeLifestyleManager : ScopedLifestyleManager
{
public LifetimeObjectScopeLifestyleManager() : base(new LifetimeObjectScopeAccessor())
{
}
}
public class LifetimeObjectScopeAccessor : IScopeAccessor
{
public void Dispose()
{
// TODO: Dispose the lifetime instance
}
public ILifetimeScope GetScope(CreationContext context)
{
// How do I get the lifetime instance here?
}
}
class Foo : IFoo, IDisposable
{
public Foo()
{
System.Console.WriteLine("Created Foo");
}
public void Dispose()
{
System.Console.WriteLine("Disposing Foo");
}
}
interface IFoo
{
}
}Ideally, I could shove the DefaultLifetimeScope into the CreationContext so that it was accessible in LifetimeObjectScopeAccessor.GetScope. It would also need to be available when disposing. I'm sure this isn't the only problem, and I don't really know that much about Castle Windsor so feel free to call me out on anything I misunderstood. |
|
Hi @davidfowl, @fir3pho3nixx and all others :)
Yes, definitely. Because Windsor does not have such a scoping feature natively. I implemented it using This ensures to relate the resolved transient and scoped services to the current service scope, so it can dispose them when the service scope is disposed. |
|
@hikalkan Of course! Modelling the provider impedance mismatches was definitely the right way to go. You really kicked some ass leading this(I followed the same path and reached the same conclusion, I do know this is not the complexity Castle wants to own). Could you kindly provide some more context around your code quote? using (MsLifetimeScope.Using(OwnMsLifetimeScope))
{
var isAlreadyInResolving = IsInResolving;
if (!isAlreadyInResolving)
{
IsInResolving = true;
}
object instance = null;
try
{
return instance = ResolveInstanceOrNull(serviceType, isOptional);
}
finally
{
if (!isAlreadyInResolving)
{
if (instance != null)
{
OwnMsLifetimeScope?.AddInstance(instance);
}
IsInResolving = false;
}
}
} |
|
@davidfowl I need more time to respond to this (sorry man I know what you are asking for). Please bear with me. |
@fir3pho3nixx it's cool. I have a suggestion on how to fix it after you respond but I haven't dug into the code to see how hard it would be to implement.
The code probably doesn't need to be that complex. For example there is no nested scope requirement. Scopes can be at a single level they don't need to support overlapping. Because of the ambient state model though I see why you feel the need to handle it. Consider this: using (var lifetime = new DefaultLifetimeScope())
using (var lifetime = new DefaultLifetimeScope())
{
container.Resolve<IFoo>();
}There's no way to resolve from a specific scope there. The last one will win because it's the most recent. This might be an edge case though but somebody could run into it. |
|
Guys/Gals, I am closing this in light of PR: #394 @jonorossi Thanks for reviewing and cleaning up all my misconceptions about Windsor. You are a fucking legend. @cwe1ss Thank you for raising the issue. @hikalkan Thanks for leading the way on https://github.com/volosoft/castle-windsor-ms-adapter. Seriously man, good work. @dotnetjunkie You informed and led this process from eons ago. Thank you. @davidfowl Thank you for engaging us. Hope you don't mind if we call on you in the future. Seriously thank you. This issue only took 2 years, 4 months, 18 days excluding the end date to solve(kind of). |
ghost
locked as resolved and limited conversation to collaborators
Hi,
I created an integration for Microsoft.Extensions.DependencyInjection.
You can find the code here: https://gist.github.com/cwe1ss/050a531e2711f5b62ab0
The code is based on the following links:
I adjusted the code for RC1 and fixed some issues:
I'm not sure why the call to BeginScope() in Startup.cs is necessary - if it's missing, the first request fails. Seems like ASP.NET Core doesn't start the scope for the first request or some Scoped service is requested before it does.
best regards!
The text was updated successfully, but these errors were encountered: