OWIN-compatible implementation for "PerWebRequest" lifestyle #283
Comments
|
@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. |
|
@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
changed the title
|
@jonorossi it's great to see that Windsor and Castle.Core are almost completely ready for .NET Core! 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). |
|
@fir3pho3nixx with the port we've left the .NET Framework code for scopes using |
@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. |
|
@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. |
|
@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. |
@mario-d-s: I could not find the particular extension
@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
@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. |
|
@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? |
|
@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. |
|
A new assembly sounds like a great way to go, that keeps the dependencies of Extension methods are definitely a good approach, take a look at the |
|
Yes, this is what I was thinking. Can I go ahead and create a new branch called 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 |
Go for it.
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.
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. |
|
I have done some more investigation into this. My interest here was how we All paths appear to be leading to Rome, or at least in this case My example is only using the I submit my cobbled together OWIN startup file for 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 @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( @jonorossi: I think the test project for this should boot up my example as a OWIN self host, use the windsor registration API for Let me know what you guys think. |
|
@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 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. |
|
I agree with @mario-d-s, it sounds like you are on the right track.
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. |
|
@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 @jonorossi - Can you weigh in on this(without pulling your punches)? This test right here assumes that scoped lifestyles will auto-magically dispose transients without being housed in a parent dependency that is marked as 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? |
|
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. |
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? |
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. |
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.
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 |
|
The duplicate registrations of
|
|
Getting tests going was a little tricky also. |
|
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 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? |
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.
In the case of their recommended test (which uses xunit) I have debugged the
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 ....
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. |
|
They just got back to me, BeginScope() = new WindsorContainer(). So will go ahead and try this. |
|
Right, after blasting @davidfowl with WTF's he pointed me to aspnet/Mvc#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. |
Yes, we moved PerWebRequest to it's own facility SystemWeb. You implicitly get PerWebRequest using the |
Nope not at all, I really value your feedback :) |
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?
That is indeed what the discussion was about. However, I thought |
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. 👍 |
You are effectively getting this if you register your controllers as either transient or scoped. The difference is that with scope's you need to opt in using |
|
PS: Thanks for this, I will also update the docs to describe the lifestyles better. If there is anything you would like to ask or perhaps is not clear let me know and I will make some changes where ever I can to make the UX for this a little easier. |
From the viewpoint of resolving components, that totally makes sense. Of course, dependencies of a controller will be per-web-request implicitly (the same way transient dependencies of a singleton are effectively singleton in that context). But in the following situation:
How do you register them so that M and N both get the same instance of X per request, but each get a different instance of Y per request? I don't see how your proposed registration API makes that clear, since you are registering everything as transient?
I guess I'm confused over how scopes work in Windsor, I've never really used them either. But I know the docs and the docs say basically:
AFAIK you haven't implemented the |
Make X scoped. |
Should I update the docs to Scoped? Maybe add an example and test for what you are mentioning above? |
Don't need the scope accessor. It falls back on to CallContext's. I did need it for SystemWeb facility though. |
|
To make scopes a little clearer, you use a scope accessor when you want to tie the scope to some external thing. In the case of SystemWeb we use a HttpModule for this which is tied to EndRequests for scope disposal. So it make sense to use an accessor.
In SelfHost the scope is managed in an instance of an IDepdencyScope, so no need for a scope accessor. Hope this makes sense. |
|
@mario-d-s - I am just making sure we are covering the bases here, please refer to this reply to @masterpoi in this discussion thread where I clarified how scopes would work. To summarise this was my internal view:
@masterpoi - A review from you is also most welcome. |
|
@fir3pho3nixx I've been digging into the code in Windsor to find how this whole idea of scopes works, and it's finally beginning to click! Can you confirm I'm getting it please:
If my reasonig is right, my only issue is that as a registration API, If possible, and if you agree, I would suggest you keep the The registration API can then be |
|
There is one extra improvement I'd like to see, or at least a decision I'd like to understand better: you've chosen to still use the CallContext to store scopes into. I'm not super clear on how this works exactly, but is there a reason you can't bind it to some context that is closer to the request? I see you're already using it to store the scope, so what's the difference with the scope binding itself to the context? |
Correct
Correct again
In a self host this is done for you but in the web host scenario you definitely would have to opt in. In fact I actually try and wrap the exception in the facility here to give you a more relevant self help message.
Correct
A scope for me is the more general use case with some really extensible API's for how they are accessed. I also thought they had been around for a while. If you check the first commit for the ScopedLifestyleManager you will notice it goes all the way back 2011 which was over 5 years ago. I am also not sure I follow you when you say people will just have to remember. I my hope is that the documentation is clearly stating this, which I pointed out with a link earlier :)
Why the scope accessor here? You can already do this with vanilla Windsor if you want to manage your own activator/dependency resolver. This facility is trying to help manage that for you, so you dont have to spent time googling the right way to achieve that. Also what other kind of scoped access would people want to support in an OWIN scenario? Are you perhaps thinking along the lines of middleware?
2 things here:
|
This was designed in 2011 by @kkozmic but here is a link to CallContext. |
|
@mario-d-s - Your feedback has been great, thank you. Please see commit's where I have taken your input on board. My only criticism for this issue is that you did not follow the discussion fully and I had to explain how scopes worked again. In future it would be great if you limit the conscious streaming in terms of questions/challenges, and commit to comments like this one where you pledged to commit code once you learned git. Please remember this is an open source project. If you raise issues, you should be prepared to get involved and ultimately try commit code for a solution. Again I thank you for you're feedback but @jonorossi and I will take it from here. |
|
@fir3pho3nixx I understand, I realize this was not the best place to seek further understanding about the topics I've asked about. I still genuinely intend to get involved more, but this particular issue was too complicated for me either way. I've followed the discussions from the sideline though. I'm glad I was at least of some help. The update you made to the docs on your facility makes it much clearer by the way. Thanks for all the help! |
|
@fir3pho3nixx I still think your proposed solutions is great. I took a quick look at the docs you wrote. Look good. Just a quick thought i had, typically i'd not make my controllers scoped, but the database session / uow only. Making the controllers scoped might be a bit confusing as an example. In the MVC case, the AddControllerAssembly method, i assume would scan the assembly for controllers? (And that WithLifestyleScopedPerWebRequest has nothing to do with the scanning?) Maybe name it ScanControllerAssembly instead and put WithLifestyleScopedPerWebRequest in front ? |
Not all. Your UoW pattern hopefully breaks things down to a factory type which yields instance based resources which are disposable at some point. Could you post a Gist link to your high level UoW pattern? Maybe something like this(interfaces only :))?
No. I intentionally avoided ComponentRegistration scenario's. I would really love it if people started using IWindsorInstallers with conventional registration properly. The facilities should become the application of scopes.
Yes. For now.
LifestyleScopedControllerSourceAssembly. New name welcome. I guess this is where we can work some magic, to expose new ComponentRegistration extensions and see if we can do something to sweeten the registration part. It would make these facilities very handy. Thoughts/Gist? |
|
@fir3pho3nixx I've reviewed #351. |
Yes, typically a factory method that provides a uow something. I.e. ravendb var ds = new DocumentStore() { /* settings */ };
container.Register(Component.For<IDocumentSession>().LifestyleScoped().UsingFactoryMethod(() => ds.OpenSession()));In my mind everything that doesn't keep state should either be transient or maybe singleton for perf. Since controllers classes typically don't keep state, i tend to make them transient. (You typically won't need multiple controllers per request, so making them transient means in practice they have the same lifetime as making them scoped). The uow / db session may be shared between multiple services that the controller uses, so that really needs to be scoped to the request. As i said, it might be confusing to some, but it might only be confusing to me ;)
Agreed, but then I guess i'm missing something. What does AddControllerAssembly do? |
|
AddControllerAssembly merely supplies assembly to source types from when creating controllers for MVC. It has been converted to a fluent interface, you can now have many. Not sure about the UoW scenario, will have to get back to you. |
|
@masterpoi - I have done more digging here. Your example: var ds = new DocumentStore() { /* settings */ };
container.Register(Component.For<IDocumentSession>().LifestyleScoped().UsingFactoryMethod(() => ds.OpenSession()));I believe we can support this but please let's explore this a bit more.
If you are using something like You mentioned either transient or singleton lifestyle would be good:
I agree with you completely. If the paradigm is a layered to hell architecture like in some of the awful implementations I have seen round London recently then things change ever so subtly depending on where they are resolved. Transient. This one is tricky. You would always expect a new instance. However, if you mark a controller as transient and the facility only instantiates 1 when it engages the ASP.NET request pipeline via ControllerFactory or whatever the abstraction is. In this case you essentially have a "per web request" component right? Transient by the very definition is It get's more complicated when you move over to Imagine this example: Posts have comments, facebook right? You cannot add a comment without knowing what the original the post id was. The net effect of transient being marked on the CommentController vs the CommentRepository can yield different behaviours when it comes to state purely because of the way it is consumed. If CommentRepository was an in What we are trying to say here, is that Let's consider disposables for transients given the above. This where it can get a little crazy especially if you are using TypedFactories that them self re-engage the resolution mechanism of Windsor by using the (k) -> k.Resolve<> feature pattern. Burdens (tracking of disposables from what I can tell) simply wont allow this in their current state because they don't care about release policies or late bound factory components. What this means in plain English is that typed factories will hurt you bad, when it comes to what you "think is possible" and then you drop a "typed factory" in and it does not work(ref: #373). Moving on, so "unit of work". Please explain what your context is? I would not use this word lightly. In fact I find it quite offensive given the implementations I have seen in the wild because most people do not get what this That said, I have investigated using AutoFac with Instance per matching lifetime scope. I honestly don't associate this with "unit of work" because it quite simply does not match the problem domain we are trying to solve in windsor. The taxonomy is completely wrong for a container. We can offer named scopes, similar such. When it is resolved that name needs to be supplied to the resolve mechanism. This would create named scoped slices that could conform to lifestyle variants that would eventually support a unit of work(which we should have to care about in Windsor). The problem I have with all of this, it is supported today in Windsor. You just need to structure you code correctly. Please share what you are trying to do and with further discussion hopefully we can figure this out :) |
|
@masterpoi - thanks for you input so far, it is a great help. |
What do you mean with support? I'm sorry if i let you believe otherwise, but i don't have any fundamental issues with how things are now. And again, the solution you are working on looks great. The rest of my remarks can be summarized as "Let the architect choose the right lifestyles depending on his architecture." ;)
Yes exactly.
This is true if CommentRepository was also registered as Transient. But if it needs to keep state I would register it scoped. In general Repositories would need to be Scoped because they depend on the UOW. I don't see how registering the controller transient has any impact here. If the controller would somehow need to be resolved again in the chain i think there is something really wrong in the design. Given that the controller is the interface between the framework (MVC) and the "user code", it should be managed by the framework. And hence, as said before, in this case Transient implies PerWebRequest.
Agreed that scoped can fix this. But I really think you should consider every service in your dependency chain and give it the appropriate lifestyle when registering. Making the lifestyle of the dependencies hang of the controller lifestyle makes it harder to reason about them in those cases where you need the dependencies out of the scope of the controller. Making higher up dependencies transient gives flexibility for lower dependencies to be something more strict like scoped or singleton. The reverse is not true. If the controller is scoped, every dependency is de facto also scoped. So yes, making the controllers scoped fixes the issue in your example. But i feel like it is better to link the lifestyle to the specific component demanding it.
Do you mean making Controllers transient would make it hard/impossible for Windsor to known when to dispose it's IDisposable dependencies? Yes, i can see this might become in issue in some designs, and in that case, the architect/designer has the choice to make the controllers Scoped for this reason. But i really consider that to be his job.
In this case i use uow to refer to the transaction boundary around database operations in a single process (i.e. inside a single (micro)service). I.e. the DbContext in EF, the DocumentSession in RavenDb / Marten, NServiceBus messagehandler.
Agreed. |
Add some public API's to solve problems we think might be an issue. Don't ever say sorry, I do enough of that on project already :)
Read this. Feel free to rant but don't use names.
Interesting, can you elaborate on this. We have several problems with typed factories in this space at the moment. There is an issue where some big brains are going to duke this out. I am going to back off and see what happens. Not linking for cleanliness and separation of problems. Please give me your account.
No, deeper, chained resolves via vanilla windsor kernel that dependends on typed factories(using LateBoundComponents) with disposables at the end are a pain in the arse. It only crops up with N+2 resolution chains where a typed factory is in the middle. Hopefully we will have an answer soon. It is not often I get a reflective opinion that leans towards my own. Thanks for your input. We are nearing the home run on this issue. Closing this out as I have prepped the final PR. Release is imminent. |
ghost
closed this as 
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.The text was updated successfully, but these errors were encountered: