search

castleproject / Windsor

Castle Windsor is a best of breed, mature Inversion of Control container available for .NET
http://www.castleproject.org
Apache License 2.0
1.52k stars 455 forks source link

OWIN-compatible implementation for "PerWebRequest" lifestyle #283

Closed mario-d-s closed 6 years ago

mario-d-s commented 7 years ago

The PerWebRequest HttpModule has served everyone well for applications hosted in IIS, to enable "Per webrequest" lifestyle.

However, in recent years, OWIN has become more popular and with it, the ability to self-host websites. Windsor's PerWebRequestLifeStyle is incompatible with OWIN's self-hosting mechanism, as HTTP modules only work in the IIS pipeline.

Some OWIN-compatible implementations of this lifestyle have been hacked together, see StackOverflow and GitHub.

However I think it would be best if such implementation was cleaned up, put under test, and integrated with the main project here. It would be a nice bonus if the same configuration syntax could be used as well (i.e. calling LifestylePerWebRequest()). Maybe there could be some magic to make it work under all hosting scenarios.

jonorossi commented 7 years ago

@mario-d-s completely agree the IIS module isn't useful for new web applications. There has been some work around ASP.NET Core and its built-in container in #120. I'd really like to see those with an interest in this to get together (probably via GitHub) to discuss what needs to happen and ultimately put forward those changes.

We should have a release of Windsor out with .NET Core support very soon, so I'll be great to have this type of support come in a following release.

ghost commented 7 years ago

@mario-d-s: +1

PerWebRequest tries to implement dependencies like IHttpModule and then call back into the micro kernel using scopes, I think we should be inverting this. BeginScope/EndScope was the start of that work and was meant to supersede the other implementations but it was based on remoting which was not available in netcoreapp via netstandard(CallContexts) during the migration of Windsor. I do believe in netstandard is bringing this back in v2.0.

Once it compiles everywhere, it would be a doddle to implement extensions in that wrap this idea.

mario-d-s commented 7 years ago

@jonorossi it's great to see that Windsor and Castle.Core are almost completely ready for .NET Core! However, from what I've been reading on the web, there are a couple of differences between OWIN in .NET Core and OWIN in full framework. I have 0 experience with Core myself so I can't really comment.

I just hope any compatibility will also be backported to work with OWIN on the full framework.

@fir3pho3nixx According to at least one Microsoft engineer, CallContext should be avoided in scenario's unrelated to remoting. The source of that claim would be behind this broken link but unfortunately there seems to be no way of retrieving that page anymore (archive.org doesn't have it).

jonorossi commented 7 years ago

@fir3pho3nixx with the port we've left the .NET Framework code for scopes using CallContext (because we are targeting .NET 4.5), however the .NET Standard code will use AsyncLocal.

jonorossi commented 7 years ago

I just hope any compatibility will also be backported to work with OWIN on the full framework.

@mario-d-s this is where I highly encourage you to get involved in the project, we really do need help and features won't get supported unless someone actually wants them and puts in some work. I hope as a collective updates to the per-webrequest lifestyle can support everyone involved.

ghost commented 7 years ago

@mario-d-s: I remember reading about this many moons ago, cannot for the life of me remember why though :)

@jonorossi: I did come across the use of AsyncLocal here. I must have missed that bit in the netcore branch when my internet was borked.

Checking out some middleware implementations would be as simple as creating an extension that goes something like this?


app.Use(new Func<AppFunc, AppFunc>(next => (async env =>
{
    myWindsorContainer.BeginScope();
    await next.Invoke(env);
    myWindsorContainer.EndScope()
})));

You could then wrap it in a tidy extension method.

mario-d-s commented 7 years ago

@jonorossi I would love to get involved, and it's something I will definitely begin to do in the near future when I get around to finally learn Git 😃

@fir3pho3nixx Correct me if I'm wrong, but such middleware would use the default scope (i.e. LifeTimeScoped()). What if you need scopes in other places. Does this support nesting?

ghost commented 7 years ago

Correct me if I'm wrong, but such middleware would use the default scope (i.e. LifeTimeScoped())

@mario-d-s: I could not find the particular extension LifeTimeScoped but I am pretty sure you mean LifestyleScoped. This is basically a shorthand API extension that will apply LifestyleType.Scoped to the config for that registration(using descriptors) which eventually ends up creating a ScopedLifestyleManager(commit: 4e7716e2) in the DefaultKernel. This is significant because when it tries to obtain the current scope for an instance it will return a CallContextLifetimeScope(the class based on remoting I mentioned earlier written by Kozmic himself). The default scope you mention(DefaultLifetimeScope) was only really a thing in the PerWebRequestLifestyleModule and the ThreadScopeAccessor.

What if you need scopes in other places.

@mario-d-s: There is an overload for LifestyleScoped whereby you can specify the type of an accessor. This is a concept which allows you get these things from anywhere but you have to implement an IScopeAccessor. Pretty neat huh?

Does this support nesting?

@mario-d-s: From what I can tell it does.

I think there was significant work at play here to try and clean this up and to make lifestyle management a little easier in general and if Windsor doesn't have it you could "roll your own" in a very easy way, so I am glad you raised this issue. I like the public API for BeginScope/EndScope because of it's simplicity, I am not particularly bound to the implementation of it, I just mentioned the current implementation to add more info to the discussion.

ghost commented 7 years ago

@mario-d-s - I was thinking we start a series of PR's to start moving the DesktopCLR PerWebRequestLifestyleModuleRegistration stuff out of Windsor and into facilities(or something else). It is still there and supports the FEATURE_SYSTEM_WEB . The reason I say this is there has been a significant effort to migrate this windsor to dotnet core and it presents different challenges for Windsor's public API when it comes to lifestyle management. My vote is that we talk about how we would abstract this out whilst implementing an OWIN compatible version at the same time? What do you think?

ghost commented 7 years ago

@jonorossi - I would like to start work on this next. Can you tell me what you would like to see happen here?

My recommendation is we start creating satellite assemblies(Windsor Lifestyle NuGet's) for vendor specific stuff and start breaking it out of the MicroKernel. Our versioning can then be kept in step with vendors and not be tied to core implementations of Windsor. Perhaps we want to start leveraging the facility architecture which would be keeping with the spirit of the original design or we could just create them as extension methods(honestly I prefer this approach).

Look forward to your feedback.

jonorossi commented 7 years ago

A new assembly sounds like a great way to go, that keeps the dependencies of Castle.Windsor.dll small and keeps things self contained. I see no reason the System.Web lifestyle couldn't also move out of the main assembly, which also helps by removing some conditional compilation. The lifestyle enum should be reserved for the really basic built-in lifestyles, and really its use de-emphasised.

Extension methods are definitely a good approach, take a look at the Castle.Windsor.Lifestyles contrib project that did the same thing: https://github.com/castleproject-deprecated/Castle.Windsor.Lifestyles/blob/master/Castle.Windsor.Lifestyles/LifestyleRegistrationExtensions.cs

ghost commented 7 years ago

Yes, this is what I was thinking. Can I go ahead and create a new branch called lifestyles?

Also, do we want a separate assembly for each target implementation? eg.

If you are happy with this as a starting block I can add these new projects and their co-respective NuGet packing requirements into the VS2017 build infrastructure as empty projects for now. I will then commence work on moving PerWebRequest down.

jonorossi commented 7 years ago

Can I go ahead and create a new branch called lifestyles?

Go for it.

Also, do we want a separate assembly for each target implementation?

That probably makes sense as the package dependencies would be vastly different, I'd probably use these names instead:

Unless they'll be more than just a lifestyle, i.e. the WCF integration has a per channel lifestyle but is obviously named a facility as the main part is the Windsor facility.

I will then commence work on moving PerWebRequest down.

Feel free to do what you think works best, but I'd have thought it would be best to leave that one until you've got the others the way you want them.

ghost commented 7 years ago

I have done some more investigation into this. My interest here was how we future proof Castle Windsor and create an implementation that services both dotnet core and desktop clr using OWIN because it is already available on both.

All paths appear to be leading to Rome, or at least in this case Microsoft.Extensions.DependencyInjection. I did know that this could be used on netcore but did not for one second think about using it in a desktop clr scenario. A bit of a duh moment I must admit.

My example is only using the Microsoft.Extensions.DependencyInjection nuget sans any Windsor container implementation. The reason I have done this is because I want to understand how the dependencies and the consumer API works before we dive into Microsoft.Extensions.DependencyInjection.Abstractions, which is where the meat and potatoes of the cross runtime implementation might need to be implemented if we all agree on my suggested approach.

I submit my cobbled together OWIN startup file for net452. Where you can see the same old divergent IDependencyResolver implementation for MVC here and WebApi here. I then went ahead and registered my own service called IAnyService. Following that I registered an MVC and a WebApi controller.

This got me round to thinking about how we still use Windsor's registration API but still get the buttery goodness of integration using MVC and WebAPI in a DesktopClr/NetCore scenario. The answer appears to be by re-implementing Microsoft.Extensions.DependencyInjection.Abstractions. I then had a look at the competition and it appears as though they have beat us to the punch. This could be viewed as a good thing because they have solved the problem for us. We just need a Windsor flavour.

@mario-d-s: Can you weigh in on your intended usage of Castle.Windsor for this issue? How you would like this API to work? I am thinking this kind of extension is perfectly inline with the spirit of registering OWIN middleware(Use* convention). My suggestion here is we have a UseCastleWindsor extension. I would also like to know whether you have any apprehension netstandard being pulled in as a dependency? Assuming you are using this in a DesktopClr scenario, this can slow down CI builds. Are there also any other Service Location concerns you think we are missing whilst registering middleware(seen some stackoverflow's on this)?

@jonorossi: I think the test project for this should boot up my example as a OWIN self host, use the windsor registration API for IAnyService which is ultimately resolved in MVC and WebApi using a HttpClient. Would be super cool to get a dotnet core version of this going too. It is supported.

Let me know what you guys think.

mario-d-s commented 7 years ago

@fir3pho3nixx I think you're pretty much on track for this. I would definitely refer to the AutoFac stuff as an inspiration to get started.

I don't have any additional concerns for the moment. I can't really comment on your question about the netstandard dependency, I assume this is .NET Core stuff which, I've already mentioned, I have no experience with.

By the way, I've already learned a bit of Git in the last couple of weeks. Perhaps I'll be able to contribute myself in a meaningful way here soon, I'll keep you posted.

jonorossi commented 7 years ago

I agree with @mario-d-s, it sounds like you are on the right track.

I think the test project for this should boot up my example as a OWIN self host, use the windsor registration API for IAnyService which is ultimately resolved in MVC and WebApi using a HttpClient. Would be super cool to get a dotnet core version of this going too. It is supported.

I've got no objection to adding some integration tests, it might be worthwhile having a unit test project just for the MSDependencyInjection project though so anyone working on core Windsor doesn't have to worry about all of that and its dependencies.

ghost commented 7 years ago

@mario-d-s - I am sorry if this is not relevant to your issue, which I have already solved BTW. I do have an interest in getting dotnet core going for the Microsoft.Extensions.DependencyInjection stuff. If you feel this is not relevant here I am happy to take a slap on the hand and raise a new issue. I just feel that the implementation is incredibly naive and vulnerable to to memory leaks when it comes to scoping. I submit further detail to @jonorossi below.

@jonorossi - Can you weigh in on this(without pulling your punches)? Microsoft.Extensions.DependencyInjection makes some bold assumptions about naked parent level transients that are resolved in a scoped lifestyle.

This test right here assumes that scoped lifestyles will auto-magically dispose transients without being housed in a parent dependency that is marked as scoped. I got this test from here and copied it into the solution for debugging purposes. This to me is quite simply bad design. It is non deterministic and opens up the gates of hell for prematurely disposed transients or alternatively could create further memory leaks if the developer changing this code makes an assumption that his/her transient object is running within a scope. There simply is no type safety around this design.

I have also been reviewing custom lifestyles over here and was wondering if this was truly the answer. It feels like a sledge hammer.

My expectation for this test would be something like this where by you could find usages of OptionsContainer and immediately know it is scoped. Better or no?

ghost commented 7 years ago

I am tempted to wrap transients using Castle Core mixins with fallback logic. Not sure if this would work, but I think I am going to start looking at this tomorrow night.

jonorossi commented 7 years ago

This test right here assumes that scoped lifestyles will auto-magically dispose transients without being housed in a parent dependency that is marked as scoped. I got this test from here and copied it into the solution for debugging purposes. This to me is quite simply bad design. It is non deterministic and opens up the gates of hell for prematurely disposed transients or alternatively could create further memory leaks if the developer changing this code makes an assumption that his/her transient object is running within a scope. There simply is no type safety around this design.

How does Windsor behave today resolving and disposing transients as the root inside a scope, does the burden cause it to get released on scope disposal?

jonorossi commented 7 years ago

I am tempted to wrap transients using Castle Core mixins with fallback logic. Not sure if this would work, but I think I am going to start looking at this tomorrow night.

The performance penalty would be quite high especially for all transients. How would this work anyway, how would you know the user is finished with the object, I can't see how you'd do reference counting.

The transient really should be released as per Windsor's long standing guidelines.

ghost commented 7 years ago

How does Windsor behave today resolving and disposing transients as the root inside a scope, does the burden cause it to get released on scope disposal?

No it does not, and I don't believe it should. If it is resolved as a child dependency where the parent has a scope it does. I think this is correct behaviour.

The performance penalty would be quite high especially for all transients. How would this work anyway, how would you know the user is finished with the object, I can't see how you'd do reference counting.

Yeah, thinking about it I don't think it would be easy and anything as best would be a lifecycle hack. I will keep digging but I might raise an issue to see what the authors say. Lifestyle side effects like this can lead to all sorts of bad things.

ghost commented 7 years ago

Yeah not sure how to proceed, raised this issue. I think this needs fixing. Parent/Child scoping got violated in Windsor when I implemented this. Let's see what happens next.

ghost commented 7 years ago

The duplicate registrations of RC1ForwardingActivator are also doing my head in.

image

ghost commented 7 years ago

Getting tests going was a little tricky also.

jonorossi commented 7 years ago

Looking at that test code again it feels a little like Windsor's child containers since the resolution of all components is happening against the scope, but then the singleton doesn't get disposed on scope disposal so it doesn't really match. When the code references scope.ServiceProvider is that the exact same instance as the provider variable or something special for that specific scope? Just wondering if nested scopes are a thing. If they are the same, the code would read clearer with just provider.GetService.

If the transient gets disposed at the end of the scope then why shouldn't the singleton also get disposed? Is there a design document/specification for how the MSDependencyInjection abstractions should be implemented and how the lifecycle of components behave with scopes?

ServiceCollectionServiceExtensions.AddSingleton<IFakeSingletonService, FakeService>(collection);
ServiceCollectionServiceExtensions.AddScoped<IFakeScopedService, FakeService>(collection);
ServiceCollectionServiceExtensions.AddTransient<IFakeService, FakeService>(collection);
...

using (var scope = ServiceProviderServiceExtensions.CreateScope(provider))
{
    disposableService = (FakeService) scope.ServiceProvider.GetService<IFakeScopedService>();
    transient1 = (FakeService) scope.ServiceProvider.GetService<IFakeService>();
    transient2 = (FakeService) scope.ServiceProvider.GetService<IFakeService>();
    singleton = (FakeService) scope.ServiceProvider.GetService<IFakeSingletonService>();

    Assert.False(disposableService.Disposed);
    Assert.False(transient1.Disposed);
    Assert.False(transient2.Disposed);
    Assert.False(singleton.Disposed);
}

Assert.True(disposableService.Disposed);
Assert.True(transient1.Disposed);
Assert.True(transient2.Disposed);
Assert.False(singleton.Disposed);
ghost commented 7 years ago

Looking at that test code again it feels a little like Windsor's child containers since the resolution of all components is happening against the scope, but then the singleton doesn't get disposed on scope disposal so it doesn't really match.

We appear to have matching behaviour for singletons. I am thinking the reasoning behind this not getting disposed is because although it is a memory leak it is a small one because of the long lived lifestyle.

When the code references scope.ServiceProvider is that the exact same instance as the provider variable or something special for that specific scope? Just wondering if nested scopes are a thing. If they are the same, the code would read clearer with just provider.GetService

In the case of their recommended test (which uses xunit) I have debugged the IServiceProviderFactory which would suggest that the instances could be different for scope.ServiceProvider but it is the same instance. So for now I am in complete agreement with you.

If the transient gets disposed at the end of the scope then why shouldn't the singleton also get disposed?

They ask you not to used scoped dependencies in a singleton in the docs. So I guess this is just something the user has to remember. Then again thinking about it, if you use a transient in a singleton according this test it will get disposed but the singleton will live on ....

Is there a design document/specification for how the MSDependencyInjection abstractions should be implemented and how the lifecycle of components behave with scopes?

This test suite is what they came back with from GitHub. The docs online point you to an autofac implementation. It is a long old read.

ghost commented 7 years ago

They just got back to me, BeginScope() = new WindsorContainer(). So will go ahead and try this.

ghost commented 7 years ago

Right, after blasting @davidfowl with WTF's he pointed me to https://github.com/aspnet/Mvc/issues/5403.

Apparently there is an implementation that uses an outside-in approach for containers here by bastardising scopes using OWIN middleware. I am definitely not saying this is the answer, it needs to be validated but luckily we are not the only ones. I am going to drop this business of trying to write my own abstraction implementation and join this discussion. I just need time to absorb what is going on here. I resent the fact that registration is fault tolerant, I also dont like the fact that service resolution is done from scopes with overriding lifestyles when it comes to disposables. This is a radical departure from windsor's design. Scopes are something else entirely(they dont nest). Will keep this issue updated.

jonorossi commented 7 years ago

@fir3pho3nixx looking forward to hearing the outcome of your continued research and discussions, I wasn't aware of those non-conforming GitHub issues either.

jonorossi commented 7 years ago

Knowing that at least the ASP.NET Core DependencyInjection implementation is less about a lifestyle and more an adapter, we should instead follow the existing Windsor facilities naming convention used for all existing extensions.

ghost commented 7 years ago

@jonorossi - I have to be honest here, I believe we should drop the whole MsDependencyInjection thing for this issue. It is massive, not to be forgotten though, will raise a new issue. On the bright side I have solved the OWIN implementation on both desktop clr and core. A massive huge shout out to @dotnetjunkie from the CastleProject for coming up with a tidy working OWIN implementation here. You are a legend. From all my time wasted studying this the only change I needed to make was implement naming, and mark things as IsDefault. Both WebAPI and MVC now work. This supports my scoping API argument earlier in this issue. Good stuff.

Still need to write a hammer test and check it with scenarios I believe are valid using dottrace and dotmemory. After that I can submit a PR with tests. Implementing OWIN is very easy and extensible.

Edited

We can move forward and close this issue in roughly a 2 week timeframe. I also recommend we minor patch to Castle.Windsor to 4.1.0 because we are adding new functionality in a backwards compatible way.

OWIN PR next. :)

dotnetjunkie commented 7 years ago

They ask you not to used scoped dependencies in a singleton in the docs. So I guess this is just something the user has to remember.

This is actually something they have "addressed" post-v1. There is now an overload of ServiceCollectionContainerBuilderExtensions.BuildServiceProvider that accepts a boolean validateScopes that prevents scoped instances to be resolved from the root container (and probably prevent them from being injected into singletons).

The problem however with this new feature is that 1. Nobody knowns about its existence and 2. It is something the user must explicitly call and 3. setting this is a breaking change which might cause 3rd party libraries that integrate with the IServiceCollection to break and 4 using it is rather ugly. Example:

public IServiceProvider ConfigureServices(IServiceCollection services)
{
    // your normal stuff here

    // Explictly call BuildServiceProvider and return the IServiceProvider
    return services.BuildServiceProvider(validateScopes: true);
}

Then again thinking about it, if you use a transient in a singleton according this test it will get disposed but the singleton will live on ....

Great catch. By allowing transients to be injected into singletons, they basically use the lifestyle that Autofac calls 'instance per dependency'. In other words, "each consumer is intended to get its own instance of the dependency, and the dependency is expected to live as long as the consumer". Whether or not this is sane is arguable, but if you do this, you simply can't dispose any transient that is injected into a singleton, because disposing the transient means you break its consuming singleton and therefore break the application. Long story short: These two models don't marry.

he pointed me to aspnet/Mvc#5403.

He forgot to point out to other discussions such as this important one: https://github.com/aspnet/DependencyInjection/pull/416 and this one https://github.com/aspnet/DependencyInjection/issues/433. I've been discussing this with Microsoft for about two years now, and have written a two articles on this myself here and here over a year ago.

Great that Fowler points at #5403, but up till now, Microsoft has done nothing to improve the situation for non-conformers.

jonorossi commented 7 years ago

I have to be honest here, I believe we should drop the whole MsDependencyInjection thing for this issue. It is massive, not to be forgotten though, will raise a new issue. On the bright side I have solved the OWIN implementation on both desktop clr and core.

@fir3pho3nixx sounds good to me.

He forgot to point out to other discussions such as this important one. I've been discussing this with Microsoft for about two years now, and have written a two articles on this myself here and here over a year ago. Great that Fowler points at #5403, but up till now, Microsoft has done nothing to improve the situation for non-conformers.

@dotnetjunkie thanks for the links and the great work blazing the trails on this stuff over the last few years. After skim reading a few of those it is coming back to me, I was following along back then in the early DNX days but just checked out completely because it all seemed too hard and I didn't have the time or energy to care at the time.

ghost commented 7 years ago

@jonorossi - Current learnings so far.

OWIN has some constraints:

My PR addresses the DesktopCLR side so far with some peculiar API's namely UseWindsorOwinWebHost and UseWindsorOwinSelfHost (needs renaming though!). I am working on the UseWindsorOwinKestrel implementation at the moment.

This got me round to thinking, we do have an opportunity for creating a migration safe API given everything I know about how Windsor runs across the spectrum.

Considerations for how we name assemblies

Most people would not want to have MVC and WebApi in the same project as a dependency in a DesktopCLR scenario. The reason I say this, is because PerWebRequest does it using a module that requires config after finding a runtime exception. The newer zero-config way unfortunately requires 2 separate projects.

MVC

DependencyResolver.SetResolver(new WebHost.MsMvcDependencyResolver());

requires(latest)

<PackageReference Include="Microsoft.AspNet.Mvc" Version="5.2.3" />

WebApi

GlobalConfiguration.Configuration.DependencyResolver = new MsWebApiDependencyResolver<WebHost.MsWebApiDependencyScope>();

requires(latest)

<PackageReference Include="Microsoft.AspNet.WebApi" Version="5.2.3" />

In both WebHost and SelfHost, the API's above have nothing to do with OWIN for overriding from a Windsor point of view but as such it might be prudent to herd all the environment config sheep(App_Start) into a StartUp.cs file that is OWIN compliant. Also now that we know Startup.cs and middleware is a thing on netcore with completely different dependencies perhaps there is an opportunity for API optimisation.

AspNet was the only thing tying this all together in a fly by the seat of your pants kind of way

I assume you identified Castle.Facilities.SystemWeb as the landing place for PerWebRequest lifestyles, maybe it needs a web bucket. For example we could tweak this to be Castle.Facilities.Web.SystemWeb so we can support Castle.Facilities.Web.Mvc or Castle.Facilities.Web.WebApi as an example. Also in the interest of hiving off functionality that should not be in the microkernel, perhaps we should make it Castle.Facilities.MicroKernel.Web.PerWebRequest? Not sure myself at this point.

Proposed new facility namespace Castle.Facilities.AspNet.*

With this we can create:

This contains a push down of PerRequest API as an extension method of microkernel lifestyles ...

This contains a AddWindsorMvc extension on Owin.IAppBuilder but override's the MVC ControllerFactory.

This contains a AddWindsorWebApi extension on Owin.IAppBuilder but override's the WebApi DependencyResolver with this scope.

This contains a AddWindsorWebApiSelfHost extension on Owin.IAppBuilder but overrides the WebApi DependencyResolver with this scope

This really did not need to be a separate NuGet.

This contains a AddWindsorKestrel extension yet to be verified.

Out of scope for this discussion.

This contains a AddWindsorWebListener extension yet to be verified.

Out of scope for this discussion

jonorossi commented 7 years ago

@fir3pho3nixx I was really hoping others would have jumped in with their opinion, if you are following along silently please do.

I do like the Castle.Facilities.AspNet.* naming compared to adding Web to the namespace to Ms prefixes everywhere. The product name (ASP.NET or ASP.NET Core) makes it much clearer what it is for. I just wonder if all of this is necessary since we don't have any today, and ASP.NET is now sort of legacy since you can run ASP.NET Core on .NET Framework and .NET Core?

It would also be worthwhile thinking how the naming would tie into usage of Windsor in Nancy (or another web framework) on .NET Core using DependencyInjection.

masterpoi commented 7 years ago

What would be the difference between AddWindsorKestrel and AddWindsorWebListener? As i understand it the request scope would be opened and closed in middleware, which afaik behaves the same in both? Since AspNetCore middleware is not Owin middleware, I'd rather suggest something like Castle.Facilities.AspNet.Core and Castle.Facilties.Owin.

ghost commented 7 years ago

I do like the Castle.Facilities.AspNet.* naming compared to adding Web to the namespace to Ms prefixes everywhere. The product name (ASP.NET or ASP.NET Core) makes it much clearer what it is for. I just wonder if all of this is necessary since we don't have any today, and ASP.NET is now sort of legacy since you can run ASP.NET Core on .NET Framework and .NET Core?

@jonorossi - Glad you like the product namespaced idea. As for ASPNET sort of being legacy, there are a surprising amount of companies in the center of London that still have not made any progress on moving over to ASPNET Core(let alone the cloud!). Selfishly I am also tired of picking up projects in the wild with half-baked Windsor solutions in them. I do take your point, though most these integrations appear to be about 2 lines of code. Something worth considering. Perhaps we should look forward not back. That said I will leave the final decision to you. :)

It would also be worthwhile thinking how the naming would tie into usage of Windsor in Nancy (or another web framework) on .NET Core using DependencyInjection.

Nancy yes, DependencyInjection potentially but I would encourage use of the Windsor registration API instead. The Nancy Windsor Bootstrapper could benefit from an update. It appears to be using the same DesktopClr method the Microkernel uses for PerWebRequest lifestyles and only runs under net452. I can factor this into my thinking whilst checking out the core stuff further. Good shout.

What would be the difference between AddWindsorKestrel and AddWindsorWebListener? As i understand it the request scope would be opened and closed in middleware, which afaik behaves the same in both? Since AspNetCore middleware is not Owin middleware, I'd rather suggest something like Castle.Facilities.AspNet.Core and Castle.Facilties.Owin.

@masterpoi - Nothing at all from what I can tell. These are just methods I spit balled out there, they can be revised to only be AddWindsor. Castle.Facilities.Owin is an interesting one because of the WebHost(that can do true PerWebRequest lifestyles hosted in IIS) and then there is the pure middleware logic for SelfHost(which is vanilla CallContextLifetimeScope without handlers and HttpContext.Current == null). I would like to know just for clarity, would Castle.Facilities.Owin only support WebApi?

masterpoi commented 7 years ago

Castle.Facilities.Owin is an interesting one because of the WebHost(that can do true PerWebRequest lifestyles hosted in IIS) and then there is the pure middleware logic for SelfHost(which is vanilla CallContextLifetimeScope without handlers and HttpContext.Current == null). I would like to know just for clarity, would Castle.Facilities.Owin only support WebApi?

(Just some thoughts, my understanding of the details or consequences might be lacking.)

Wouldn't this be something that you could auto-detect? In fact, when reading the OP again, i think putting all this magic under a single LifestylePerWebRequest is the way to go. Decouple the framework from the lifestyle (also with regard to naming). You would want to be able to use PerWebRequest lifestyle outside of the established frameworks, i.e. in custom middleware.

So LifestylePerWebRequest would match (if i understood correctly), on IIS: Traditional PerWebRequest OWIN - IIS: Traditional PerWebRequest OWIN - Selfhost : CallContextLifetimeScope AspNetCore: LifeStyleScoped

In the case of AspNetCore you would call AddWinsor somewhere to register the middleware that manages the scope.

In addition to that you would have the facilities/adapters that provide integration with Mvc, WebApi, Nancy, MsDependencyInjection for Core, and other frameworks.

ghost commented 7 years ago

Just some thoughts, my understanding of the details or consequences might be lacking.

No problem, I am only connecting the dots as I go. Glad to have your input.

Wouldn't this be something that you could auto-detect?

Yes, this was my first thought whilst pursuing this under the SelfHost OWIN implementation on net452 here.

My question here would be this, if you were testing for the presence of an HttpContext.Current in an OWIN Startup how would you build that auto detection mechanism without incurring a performance hit in the middleware along with having to make it threadsafe?

In fact, when reading the OP again, i think putting all this magic under a single LifestylePerWebRequest is the way to go.

My worry here is coupling. The Windsor project already houses a reference to System.Web because of LifestylePerWebRequest in the MicroKernel. This could be improved, for the WPF guys who dont care about this. With everything I have explained so far in this issue we have to try decouple the following packages for DesktopClr on AspNet. 

<package id="Microsoft.AspNet.Mvc" version="5.2.3" targetFramework="net452" />
<package id="Microsoft.AspNet.WebApi" version="5.2.3" targetFramework="net452" />
<package id="Microsoft.AspNet.WebApi.WebHost" version="5.2.3" targetFramework="net452" />
<package id="Microsoft.AspNet.WebApi.SelfHost" version="5.2.3" targetFramework="net452" />

For core we are lucky all of these concepts are merged. It does bring new problems.

Decouple the framework from the lifestyle (also with regard to naming)

I am open to suggestions. How would you achieve this?

You would want to be able to use PerWebRequest lifestyle outside of the established frameworks, i.e. in custom middleware.

So LifestylePerWebRequest would match (if i understood correctly), on
IIS: Traditional PerWebRequest
OWIN - IIS: Traditional PerWebRequest
OWIN - Selfhost : CallContextLifetimeScope
AspNetCore: LifeStyleScoped

I am looking at the runtime level. Here is my view incorporating yours.

My goal here is we agree on how a side effect like System.Web found it's way into the Lifestyle namespace of the MicroKernal in Windsor? This is an important API but it should be scaled out to a new assembly and it's runtime implications considered. BeginScope/refDispose looks like a lovely API to do this with when it comes to LifeStyleScoped because it way more portable across platforms right now. Please see this which I shamelessly plagiarized from @dotnetjunkie. These are really tidy hooks outward from the MicroKernel which would make the assembly Castle.Windsor almost runtime agnostic.

In the case of AspNetCore you would call AddWinsor somewhere to register the middleware that manages the scope.

Yes. I have played with this idea here.

In addition to that you would have the facilities/adapters that provide integration with Mvc, WebApi, Nancy, MsDependencyInjection for Core, and other frameworks.

Other framework perhaps not, I am being selfish here. I have web concerns. Not sure what people would need from the Desktop world. Nobody is looking at this right now.

masterpoi commented 7 years ago

My goal here is we agree on how a side effect like System.Web found it's way into the Lifestyle namespace of the MicroKernal in Windsor? This is an important API but it should be scaled out to a new assembly and it's runtime implications considered. BeginScope/refDispose looks like a lovely API to do this with when it comes to LifeStyleScoped because it way more portable across platforms right now. Please see this which I shamelessly plagiarized from @dotnetjunkie. These are really tidy hooks outward from the MicroKernel which would make the assembly Castle.Windsor almost runtime agnostic.

Yes, this sounds really great. Make everything lifestylescoped and move the reference to System.Web to a seperate assembly for those who need it.

Since everything would become LifestyleScoped, have you not de facto decoupled the framework from the lifestyle? Facilities providing the deeper integration would then configure how to setup LifestyleScoped for the specific framework.

For instance, the Castle.Facilities.AspNetCore does the middleware thing as shown in your example and provide adapters for MsDependencyInjection (which, i guess, should be in another assembly for those cases where you use MsDependencyInjection outside of asp.net)

Castle.Facilities.AspNet.Mvc would provide the ControllerFactory, DependencyResolver, etc...

ghost commented 7 years ago

@masterpoi - Sorry I have been busy with other things, I am just pinging you back to say hi and I am still thinking about this.

Yes, this sounds really great. Make everything lifestylescoped and move the reference to System.Web to a seperate assembly for those who need it.

Glad you agree. lifestylescoped is the intellectual property of castle windsor, and facilities implement this for different platforms.

Since everything would become LifestyleScoped, have you not de facto decoupled the framework from the lifestyle?

Yes. The lifestyle is really down to the hosting environment, they know best.

Facilities providing the deeper integration would then configure how to setup LifestyleScoped for the specific framework.

Correct.

For instance, the Castle.Facilities.AspNetCore does the middleware thing as shown in your example and provide adapters for MsDependencyInjection (which, i guess, should be in another assembly for those cases where you use MsDependencyInjection outside of asp.net)

Maybe we leave MsDependencyInjection out of it for now. I don't think it's scoping is compatible with the MicroKernel because of the way it expects to create and destroy the container the whole time. My proposal is purely runtime based

ghost commented 7 years ago

I think we are nearing the home run on this issue. I have implemented 3 facilities one of which supports OWIN self hosting.

ghost commented 7 years ago

@mario-d-s - if you could please take a look at https://github.com/fir3pho3nixx/Windsor/tree/facilities-desktopclr and tell me if this is good for your use case.

The docs are located here: https://github.com/fir3pho3nixx/Windsor/blob/facilities-desktopclr/docs/aspnetwebapi-facility.md

Just pull it down and run build.cmd, then add the facilities to your intended project(using direct assembly references) and let me know what problems you have.

Any feedback would be much appreciated.

mario-d-s commented 7 years ago

@fir3pho3nixx I just had a look at the docs you've written, but I'm afraid I'm missing the point of this facility entirely. You still register controllers as transient, but using the facility makes them magically "per web request"?

As a matter of fact, we already had custom IHttpControllerActivator and IDependencyResolver implementations as the perfect place to plug Windsor into the ASP.NET framework and start resolving controllers from the container. That is I guess how everyone uses Windsor in an ASP.NET project, I can't think of another way to make it possible.

The OWIN (IIS-hosted) project I'm working on now has transient controllers, resolved through that custom IDependencyResolver implementation and it works as it should, I'm not even sure if the old PerWebRequest lifestyle would still have any value when it comes to controllers...

Aside from that, I guess PerWebRequest lifestyle had another functionality: scope other services (not controllers) to web requests implicitly. I don't believe we use that anywhere in our projects but I don't see how the facility you wrote supports that?

I'm not criticizing here by the way, I just feel like I'm missing something.

I'll see if I find some time this week to pull in your repository, make a build and play around a bit.

ghost commented 7 years ago

@fir3pho3nixx I just had a look at the docs you've written, but I'm afraid I'm missing the point of this facility entirely. You still register controllers as transient, but using the facility makes them magically "per web request"?

Transients would be scoped to the lifestyle of the request based on the resolve/release policy. Transients consumed multiple times as dependents of controllers though would behave as expected and then be tracked as burdens with decomission concerns. Do you believe this is incorrect?

ghost commented 7 years ago

Aside from that, I guess PerWebRequestLifeStyle had another functionality: scope other services (not controllers) to web requests implicitly. I don't believe we use that anywhere in our projects but I don't see how the facility you wrote supports that?

Yes, we moved PerWebRequest to it's own facility SystemWeb. You implicitly get PerWebRequest using the LifeStyleScoped method on the facility. I thought this was what we were saying in this discussion. Feel free to tell me if you thought otherwise.

ghost commented 7 years ago

I'm not criticizing here by the way, I just feel like I'm missing something.

Nope not at all, I really value your feedback :)

mario-d-s commented 7 years ago

Transients would be scoped to the lifestyle of the request based on the resolve/release policy. Transients consumed multiple times as dependents of controllers though would behave as expected and then be tracked as burdens with decomission concerns. Do you believe this is incorrect?

Technically that is correct, but since controllers should be the composition root (comparable to Program.Main() in a console app) I believe it's terrible design to have them as dependencies in other components. Still, you are right on that part and it would be the main difference compared to the transient lifestyle.

Oops, total misinterpretation there. If I understand correctly, using the facility, all transients would instead become per-web-request? But what if you really needed a transient, how can you still register it?

Yes, we moved PerWebRequest to it's own facility SystemWeb. You implicitly get PerWebRequest using the LifeStyleScoped method on the facility. I thought this was what we were saying in this discussion. Feel free to tell me if you thought otherwise.

That is indeed what the discussion was about. However, I thought LifeStyleScoped works when you are using Container.BeginScope() calls. Unless you have a custom scope accessor, but I can't find that in your docs or commits. With the original PerWebRequest lifestyle, one did not need to do BeginScope() or similar calls, it was fully implicit.

ghost commented 7 years ago

Oops, total misinterpretation there. If I understand correctly, using the facility, all transients would instead become per-web-request? But what if you really needed a transient, how can you still register it?

You still have transients, it just they behave differently at the controller level. You have to remember that they will get resolved and released using the activator which only lasts as long as the request. If your dependency graph went 3 levels deep and the same transient service was consumed inside of that twice I would expect them to be 2 completely different instances.

Edited

In fact I think it would be a good idea to add another test that proves this. 👍

ghost commented 7 years ago

However, I thought LifeStyleScoped works when you are using Container.BeginScope() calls. Unless you have a custom scope accessor, but I can't find that in your docs or commits

You can see the BeginScope/EndScope logic here.

For self host it is here