Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integration for Microsoft.Extensions.DependencyInjection (ASP.NET Core) #120

Closed
cwe1ss opened this issue Feb 6, 2016 · 93 comments
Closed

Integration for Microsoft.Extensions.DependencyInjection (ASP.NET Core) #120

cwe1ss opened this issue Feb 6, 2016 · 93 comments

Comments

@cwe1ss
Copy link

@cwe1ss cwe1ss commented Feb 6, 2016

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:

  • Components were added with .OnlyNewServices() which resulted in services that are supposed to exist multiple times not being added
  • GetService<IEnumerable>() was not resolved properly. I had to add special handling for IEnumerable<>
  • ASP.NET Core doesn't expect GetService() to return an exception if the component wasn't registered. (they use it for optional dependencies). For this reason, I added a check to Kernel.HasComponent()

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!

@cwe1ss
Copy link
Author

@cwe1ss cwe1ss commented Mar 11, 2016

I moved the code into a repository. you can find it here.

@jonorossi
Copy link
Member

@jonorossi jonorossi commented Mar 11, 2016

@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?

@cwe1ss
Copy link
Author

@cwe1ss cwe1ss commented May 18, 2016

Hi @jonorossi,
we've had some serious issues with the integration during the last weeks/months - mostly related to how scopes and sub-containers are handled in Microsoft's DI framework. I'm not sure if Windsor is really compatible with their container. Seems like other DI vendors (especially SimpleInjector) don't even want to create an adapter.

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.

@hikalkan
Copy link

@hikalkan hikalkan commented May 18, 2016

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.
Did you try to use WindsorContainer.AddChildContainer method to return a ServiceScope?

@cwe1ss
Copy link
Author

@cwe1ss cwe1ss commented May 18, 2016

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.

@hikalkan
Copy link

@hikalkan hikalkan commented May 18, 2016

Hmmm... yes, I'm creating some unit tests and it's like that.
I created a child container, added to main container (thus I can resolve classes registered to the main container), resolved some transient objects from child container then disposed the child container but it did not release resolved objects.

@hikalkan
Copy link

@hikalkan hikalkan commented May 18, 2016

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.

@cwe1ss
Copy link
Author

@cwe1ss cwe1ss commented May 18, 2016

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 :)

@hikalkan
Copy link

@hikalkan hikalkan commented May 19, 2016

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
Can you check it? I will publish it to nuget but there is a problem with nuget.exe with AspNet RC2 packages currently.

@cwe1ss
Copy link
Author

@cwe1ss cwe1ss commented May 19, 2016

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?

@hikalkan
Copy link

@hikalkan hikalkan commented May 19, 2016

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.

@hikalkan
Copy link

@hikalkan hikalkan commented May 23, 2016

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.

@jonorossi
Copy link
Member

@jonorossi jonorossi commented May 27, 2016

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. ScopedLifestyleManager used by all scoped lifestyles and WebRequestScopeAccessor)?

@hikalkan
Copy link

@hikalkan hikalkan commented May 28, 2016

We can not use webrequestscopeaccesor, because Microsoft.Extensions.DependencyInjection's scope approach is different and related to an IServiceScope instance.

@jonorossi
Copy link
Member

@jonorossi jonorossi commented May 28, 2016

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 WebRequestScopeAccessor, I'm aware that HttpContext.Current is no longer present in ASP.NET Core, I meant have you looked at how they are implemented.

Scoped lifestyles in Windsor 3.0+ already implement the interfaces you've implemented yourself. You'll see the LifetimeScopeAccessor will keep track of the scope via CallContext (and only fallback to ThreadStatic on Silverlight). For ASP.NET you need to use CallContext rather than ThreadStatic (as HttpContext does) because IIS performs thread agility where it reuses OS threads while a request is blocked on I/O.

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 LifetimeScopeAccessor (and WebRequestScopeAccessor) to handle non-ASP.NET pipeline (and ASP.NET pipeline requests): https://github.com/NancyFx/Nancy.Bootstrappers.Windsor. You obviously don't care about the WebRequestScopeAccessor bit but the other half.

@hikalkan
Copy link

@hikalkan hikalkan commented May 28, 2016

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.
Surely, I would use a standard mechanism of Windsor if it's possible, but I suppose it's not. My implementation is the most similar implementation to AspNet's, I suppose :)

@jonorossi
Copy link
Member

@jonorossi jonorossi commented May 28, 2016

@hikalkan ah, I didn't realise the IServiceScope was just an arbitrary scope object, thought they were created and destroyed in the call chain. Could you implement Windsor's IScopeAccessor using a ConcurrentDictionary<IServiceScope, ILifetimeScope> rather than a ThreadStatic?

@hikalkan
Copy link

@hikalkan hikalkan commented May 28, 2016

I would not want to use ThreadStatic at all actually. I used it just while resolving an object to understand which scope is being used in this resolving operation. If I use that dictionary, how I will transfer the scope information to the resolving code to ensure they use the same lifetimescope. Resolving code can be something like that:

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?

@hammett
Copy link
Contributor

@hammett hammett commented May 28, 2016

If you need something that works with TPL, check AsyncLocal<T>, or more error prone LogicalCallContext.

@hikalkan
Copy link

@hikalkan hikalkan commented Jun 6, 2016

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.

@Kralizek
Copy link

@Kralizek Kralizek commented Oct 5, 2017

Any news about this one?

@ghost
Copy link

@ghost ghost commented Oct 5, 2017

@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.

@Kralizek
Copy link

@Kralizek Kralizek commented Oct 5, 2017

How is people supposed to use Castle Windsor in ASP.NET Core applications? Should we do all the built-in registrations again?
Also, many library creators are creating setup helpers based on IServiceCollection.

Non supporting the de-facto standard in .NET (Core) will eventually mean pushing Castle Windsor into oblivion. And that would be sad.

@ghost
Copy link

@ghost ghost commented May 14, 2018

Is this basically trying to make castle work with ASP.NET Core without being a "conforming" container?

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.

@ghost
Copy link

@ghost ghost commented May 14, 2018

That fix is in the wrong place. Hosting is the one in the "wrong" not the DI implementation itself so that's not the right place to tackle the issue.

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?

@davidfowl
Copy link

@davidfowl davidfowl commented May 14, 2018

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.

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?

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).

@ghost
Copy link

@ghost ghost commented May 14, 2018

The thing is, today the effect is that you get different instances because there are different universes

Are scopes universes also?

I still haven't looked closely enough to see what is different about castle core's implementation that makes it a deal breaker.

I would love to explore this a little more. Scopes are completely incompatible between our containers.

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.

It always mattered. Just getting to it 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).

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.

@davidfowl
Copy link

@davidfowl davidfowl commented May 15, 2018

Are scopes universes also?

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).

I would love to explore this a little more. Scopes are completely incompatible between our containers.

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.

It always mattered. Just getting to it now.

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 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.

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.

@ghost
Copy link

@ghost ghost commented May 17, 2018

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).

Good explanation.

In Windsor a service that is registered with a scoped lifestyle will throw an exception if it is resolved from the container(or root scope) unless a scope is created first. Scoped instances live for exactly as long as the scope until it gets disposed. Child scopes yield new scoped lifestyle instances. Transient instances will be instantiated N+1 times dependending how many different service types consumed them as dependencies and their disposables are scoped tracked.

Singletons however are scope agnostic. I think this is a sticking point for me.

When I say different universes I literally mean we create different instances of IServiceProvider:

Two containers. Got it.

Those are 2 different worlds even if they share the same service descriptors. This is what hosting does because they are stages.

Stages of what? We have 2 containers here.

When I say it didn't matter, what I meant is that you get different instances at each stage which is by design.

Why have N+1 containers? We tried parent/child containers, it exposed all sorts of problems on our end.

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.

Yup but do new scopes yield new singletons? This is not right.

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.

https://en.wikipedia.org/wiki/Singleton_pattern

quote end

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.

@greenmooseSE
Copy link

@greenmooseSE greenmooseSE commented May 17, 2018

Why have N+1 containers?

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.

@ghost
Copy link

@ghost ghost commented May 17, 2018

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.

@ghost
Copy link

@ghost ghost commented May 17, 2018

@davidfowl

I believe we can fix this by passing the root scope to the call run-time resolver for singletons.

@davidfowl
Copy link

@davidfowl davidfowl commented May 17, 2018

Singletons however are scope agnostic. I think this is a sticking point for me.

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.

Stages of what? We have 2 containers here.

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)
}

Yup but do new scopes yield new singletons? This is not right.

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.

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.

I'm not sure what you mean.

@ghost
Copy link

@ghost ghost commented May 17, 2018

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.

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

@ghost
Copy link

@ghost ghost commented May 17, 2018

It's not a new scope, its an entirely new container

New scope = new container?

@davidfowl
Copy link

@davidfowl davidfowl commented May 18, 2018

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.

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

Lets not discuss the implementation details of the default container. That just makes the conversation more confusing.

Let me try to explain again:

  • Scoped services are singletons within a specific scoped container instance.
  • Singleton services are the same instance across all scoped containers.

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>(); 
@quetzalcoatl
Copy link

@quetzalcoatl quetzalcoatl commented May 18, 2018

Stages of what? We have 2 containers here.

If you look at how hosting wires things up there are 2 phases to Startup:

public class Startup
{
   // Startup gets resolved from the first container (the hosting container)
   public Startup(ILogger<Startup> logger)

   // This is the second container definition (the application container)
   public void ConfigureServices(IServiceCollection services)

Sorry for kicking in, but to make sure - so typical WebHost creation like

            return WebHost.CreateDefaultBuilder(args)
                    .UseConfiguration(configuration)
                    .UseStartup<Startup>()
                    .Build();

creates and uses "the first container (the hosting container)" just to help in creating the Startup object, and then (more or less) forgets the container and then, only after creation of the instance of Startup, we lets the Startup bootstrap its own, new, second container instance "(the application container)" thats later used through out the app's lifetime?

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?

@davidfowl
Copy link

@davidfowl davidfowl commented May 18, 2018

creates and uses "the first container (the hosting container)" just to help in creating the Startup object, and then (more or less) forgets the container and then, only after creation of the instance of Startup, we lets the Startup bootstrap its own, new, second container instance "(the application container)" thats later used through out the app's lifetime?

@quetzalcoatl that's absolutely correct.

That would explain some confusing things I've seen when I tried to setup some common logging for app startup and app runtime..

Yes, it's a PITA.

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?

It sticks around until the WebHost is disposed but nothing else is ever resolved from it (it could really be disposed earlier).

@ghost
Copy link

@ghost ghost commented May 20, 2018

@davidfowl

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. 👍

@ghost
Copy link

@ghost ghost commented May 26, 2018

@davidfowl

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.

@dotnetjunkie

I think we have cleared up most of the misconceptions I have had. Thank you for your input.

@ALL

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

@davidfowl
Copy link

@davidfowl davidfowl commented May 27, 2018

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.

@cayroso
Copy link

@cayroso cayroso commented May 28, 2018

one of the reasons that was preventing me
from taking brown field projects (upgrading from mvc5 to core)
is not being able to use castle windsor.

it seems that it's going to be possible now. i'm excited
thank you guys!. :)

@davidfowl
Copy link

@davidfowl davidfowl commented May 29, 2018

@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:

@ghost
Copy link

@ghost ghost commented May 29, 2018

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)

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

I haven't looked but I'm also guessing there's no support for child containers or nested containers in windsor

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.

@ghost
Copy link

@ghost ghost commented May 29, 2018

@davidfowl
Copy link

@davidfowl davidfowl commented May 30, 2018

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

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:

  • Disposing transients, transient disposable services resolved in a scope are disposed (is that configurable?)
  • Disposable order

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.

@hikalkan
Copy link

@hikalkan hikalkan commented May 30, 2018

Hi @davidfowl, @fir3pho3nixx and all others :)

it seems as though scoping is the hardest thing to implement right?

Yes, definitely. Because Windsor does not have such a scoping feature natively. I implemented it using AsyncLocal and created ScopedWindsorServiceProvider. The secret code is:

https://github.com/volosoft/castle-windsor-ms-adapter/blob/80964d709cd48db208387c04ecf664605f7f2ff6/src/Castle.Windsor.MsDependencyInjection/ScopedWindsorServiceProvider.cs#L43-L69

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.

@ghost
Copy link

@ghost ghost commented Jun 7, 2018

@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;
		}
	}
}
@ghost
Copy link

@ghost ghost commented Jun 7, 2018

@davidfowl I need more time to respond to this (sorry man I know what you are asking for). Please bear with me.

@davidfowl
Copy link

@davidfowl davidfowl commented Jun 8, 2018

@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.

@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?

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.

@ghost
Copy link

@ghost ghost commented Jul 3, 2018

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 ghost closed this Jul 3, 2018
@castleproject castleproject locked as resolved and limited conversation to collaborators Jul 3, 2018
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
13 participants
You can’t perform that action at this time.