Facilities SystemWeb, Mvc and WebApi #351

ghost · 2017-10-08T20:07:41Z

This hopefully addresses #283.

ghost · 2017-10-11T06:57:33Z

Additional considerations for not needing scope accessor in MVC/WebAPI:

http://piers7.blogspot.co.uk/2005/11/threadstatic-callcontext-and_02.html

Apparently this is to do with how ASPNET Queue's threads and might swap them under load after the HttpModule is created. I am not entirely convinced this is the case for MVC/WebAPI under IIS given where I have hooked into the activator/factories.

Glad to be wrong here but I do believe this will be fine for most if not all cases.

ghost · 2017-10-11T07:02:08Z

If this does become a problem under async/await scenario's, then I would recommend we think about moving everything over to use AsyncLocal instead might mean upgrading things again. I will test it again tonight and post my findings.

Looks like we are using the right approach:

https://github.com/StephenClearyArchive/AsyncLocal/blob/master/src/AsyncLocal/AsyncLocal.cs

jonorossi

Forewarning that I've gone to town (as they say) on your pull request including the docs. Apologies if my comments aren't valid, however I spent a bit of time trying to learn this stuff to be able to provide a valid review, so hopefully we can constructively move forward rather than me just sort of eyeballing the code.

It is early days, but I'd be interested to hear how you want to release these changes. e.g. ship the 2 new facilities in a minor release prior to v5, or just wait, etc.

jonorossi · 2017-10-13T07:47:23Z

src/Castle.Facilities.AspNet.SystemWeb/ComponentActivators/WebUserControlComponentActivator.cs

@@ -13,15 +13,15 @@
 // limitations under the License.


I was wondering what you were talking about when you asked if we could drop WebForms support, I think this was probably implemented for use with MonoRail and the WebForms view engine.

I think we should drop this, if someone is upgrading to Windsor v5 and still wants this than they can implement the activator themselves, the code will always be there in history.

jonorossi · 2017-10-13T07:49:12Z

src/Castle.Facilities.AspNet.SystemWeb/Lifestyle/LifestyleRegistrationExtensions.cs

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+namespace Castle.Facilities.AspNet.SystemWeb.Lifestyle


Do we need a Lifestyle namespace (or directory)? The WCF facility has a Lifestyles (plural) directory but is in the Castle.Facilities.WcfIntegration namespace.

This is how it was foldered/represented in the MicroKernel. I stayed with the singular too because it now really is only one lifestyle.

I thought users are currently using Castle.Core.PerWebRequestAttribute and Castle.MicroKernel.Registration.ComponentRegistration.LifestylePerWebRequest() method. The implementation of the second then uses the Castle.MicroKernel.Registration.Lifestyle.LifestyleGroup.PerWebRequest property, not actual users?

Getting rid of this for now.

jonorossi · 2017-10-13T07:50:06Z

src/Castle.Facilities.AspNet.SystemWeb/Lifestyle/PerWebRequestAttribute.cs

@@ -12,21 +12,18 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-#if FEATURE_SYSTEM_WEB
-namespace Castle.Core
+namespace Castle.Facilities.AspNet.SystemWeb.Lifestyle


Same comment above about namespace.

This is how it was foldered/represented in the MicroKernel. I stayed with the singular too because it now really is only one lifestyle.

jonorossi · 2017-10-13T07:50:17Z

src/Castle.Facilities.AspNet.SystemWeb/Lifestyle/PerWebRequestAttribute.cs

@@ -1,4 +1,4 @@
-// Copyright 2004-2012 Castle Project - http://www.castleproject.org/
+// Copyright 2004-2011 Castle Project - http://www.castleproject.org/


Copyright going backwards?

Time warp sinkhole repaired.

jonorossi · 2017-10-13T07:55:14Z

src/Castle.Windsor/Core/LifestyleType.cs

-		/// <summary>
-		///   PerWebRequest components are created once per Http Request
-		/// </summary>
-		PerWebRequest,


Removing this will obviously cause the underlying values for the members after this one to change, should we explicitly set values for all members to avoid any compatibility issues?

Can you please clarify?

I would really like to make this enum interal and hide it all together if I could. Future developers will naturally gravitate toward trying to extend this for lifestyles. What we are really saying is your intent should be established via the component registration without this enum. Also Windsor is choc full when it comes to lifestyles. Implementing platform/framework lifestyle should be done using scoping.

Also I have done all the work to remove it all together from the underlying components. So this is my mind is an intentional breaking change.

I would really like to make this enum interal and hide it all together if I could. Future developers will naturally gravitate toward trying to extend this for lifestyles. What we are really saying is your intent should be established via the component registration without this enum. Also Windsor is choc full when it comes to lifestyles. Implementing platform/framework lifestyle should be done using scoping.

Over time we'll push towards getting lifetimes out of here, however for at least singleton and transient there would be so much code using this enum I'd vote against removing this from the public API even if guidance was to use the extension methods.

Here I was just asking that we change the LifestyleType enum members so they have explicit values to avoid an unnecessary breaking change for Custom, Scoped and Bound changing integer values:

public enum LifestyleType { Undefined = 0, Singleton = 1, Thread = 2, Transient = 3, Pooled = 4, // 5 gets left unassigned Custom = 6, Scoped = 7, Bound = 8 }

Ok, got it. Done.

Addressed in commit: 32ab15e

PS: I prefer the iota in golang.

jonorossi · 2017-10-13T14:06:56Z

docs/aspnetwebapi-facility.md

@@ -0,0 +1,139 @@
+# AspNet WebApi Facility


"ASP.NET Web API"

jonorossi · 2017-10-13T14:20:55Z

docs/aspnetwebapi-facility.md

+		var container = new WindsorContainer();
+
+		container.Register(Component.For<ValuesController>().LifestyleScoped()); // <- `Per Web Request`
+		container.Register(Component.For<IAnyService>().ImplementedBy<AnyService>().LifestyleScoped()); // <- `Per Web Request`


With the change to using scopes for per request how does that work if you also use scopes inside your requests. e.g. if IAnyService is marked scoped and it get resolved while you are in a nested scope inside the "per request" scope won't it get released on disposal of the nested scope not the "per request" scope?

Having a very quick look at the Autofac documentation they appear to "tag" scopes so resolution inside that nested scope will know to step outwards to find the scope with the same tag that it was registered with, their InstancePerRequest is really an InstancePerMatchingLifetimeScope with a tag.

New child scope = new instance for LifestyleScoped. Things resolved after the nested scope has been disposed for the same web request also gives you the same instance you had before outside of the child scope. I caution against the usage of this because if you don't also release then there is no guarantee that it will get disposed. The other reason I think this is rather yukky is because you would have to inject IWindsorContainer to get your hands on the BeginScope method, this is possibly making your class vulnerable to implementing Service Location which just sounds like a bad idea. Microsoft took this one step further and implemented GetService on IDependencyScope while implementing IDispose for the .NET Framework. They made this a first class citizen on the dependency injection abstractions and I don't think it ended very well.

I would have thought some people would want to use some sort of unit of work scope inside the web request?

I think you need to model your problem and use types for this pattern so you fallback on to the compiler. Tagging stuff moves a compilation problem to the run-time. Not a fan.

UnitOfWork is definitely possible. Can post an example on a separate repo if you like?

Yes, the idea of walking up and out to a known scope. Let me work through some of the changes and get back to you on this one. I will create a new repo demonstrating this, and then we can discuss it and decide on refinements.

@jonorossi - I have added a very rough example of something here.

Once you clone it dont forget to run:

git submodule update --init --recursive

Please checkout the HomeController.

Example output:

@jonorossi - ^^ Ping ^^

I'll take a look at this tomorrow.

jonorossi · 2017-10-13T14:40:11Z

src/Castle.Facilities.AspNet.WebApi/ISupportFacility.cs

+	{
+		IKernel Kernel { get; }
+		bool AutoCreateLifestyleScopes { get; }
+	}


I think it would be better to just pass the IKernel where it is needed, and pass AutoCreateLifestyleScopes to the controller activator on creation, you probably shouldn't let users modify this later anyway otherwise things will get screwy. At the moment this interface just makes things strange because it pretends it isn't coupling to the facility implementation.

This field is immutable. It also saves me from passing them as separate parameters down to the activator.

It is called interface segregation. I though this was a valid SOLID principle if you dont want to expose public members on IFacility to the activator.

I don't specifically have a problem with using an interface to expose these to the implementation of the facility even if I think the abstraction is a little pointless. If you didn't want a bunch of config booleans (if more got added), then maybe passing an IKernel and a facility configuration object would be clearer. However because of this interface you are now publicly exposing IKernel on the facility. I think the name of the interface is probably what bothered me the most, reading the code that uses it became a little unclear.

I should probably also warn that I'm definitely no expert in software principles or design, so this change might make perfect sense, it just seemed a little strange for passing two fields of the facility to the facility's implementation.

jonorossi · 2017-10-13T14:47:34Z

src/Castle.Facilities.AspNet.WebApi/AspNetWebApiFacility.cs

+		{
+			if (!isSelfHosted)
+			{
+				controllerActivator = new AspNetWebApiControllerActivator(this);


I hate to keep making comparisons against Autofac, but I know nothing about ASP.NET Web API as I've spent all my time with Nancy, however the Autofac integration does both IIS and self-hosting with an IDependencyResolver, what am I missing?:

http://autofac.readthedocs.io/en/latest/integration/webapi.html#get-the-httpconfiguration

I have had varied results getting this approach to work over the years under web hosts. In theory it should be fine but there have been cases where things changed over time and this simply was not reliable enough. I also wanted a separate extension for the webhost scope implementation.

I could optimise this. Let me check it out again.

jonorossi · 2017-10-13T14:48:34Z

src/Castle.Facilities.AspNet.WebApi/AspNetWebApiFacility.cs

+
+		private void SubscribeBeforeControllerCreatedEvent()
+		{
+			if (beforeControllerResolved != null && controllerActivator != null)


Same comment as the MVC facility about events, it would be good to do something better because it just feels very messy, the control flow is so hard to follow.

This has all been refactored to only call back out to the public API. I should never have used this for scope management and now don't anymore. The callbacks are very useful.

ghost · 2017-10-14T11:46:20Z

Forewarning that I've gone to town (as they say) on your pull request including the docs. Apologies if my comments aren't valid, however I spent a bit of time trying to learn this stuff to be able to provide a valid review, so hopefully we can constructively move forward rather than me just sort of eyeballing the code.

Very welcome indeed. It cleaned a lot of things up and I think we are in a better place because of it. If I may recommend I squish the whole thing down and we check it one more time as a single commit.

It is early days, but I'd be interested to hear how you want to release these changes. e.g. ship the 2 new facilities in a minor release prior to v5, or just wait, etc.

There is definitely a breaking change here with lifestyletype. So v5. I also dont want to release this until we have done the work on the AspNetCore implementation. I would also like to move on to the deprecation story before we release v5.

v5 is going to be a fairly large release in my mind.

ghost · 2017-10-14T12:15:48Z

If you would like to see it squished down we can move over here: #355

jonorossi · 2017-10-24T05:28:03Z

If you would like to see it squished down we can move over here: #355

Why not just squish the underlying branch of this pull request, and we continue here?

jonorossi · 2017-10-24T05:32:41Z

src/Castle.Facilities.AspNet.Mvc/AspNetMvcControllerFactory.cs

+		{
+			if (facilitySupport.AutoCreateLifestyleScopes)
+			{
+				(HttpContext.Current.Items[ContextScopeKey] as IDisposable)?.Dispose();


It might be a good idea to remove the item from the HttpContext so the current state of things is clear if debugging.

Good spot, done for both mvc and webapi.

jonorossi · 2017-10-24T05:38:04Z

src/Castle.Facilities.AspNet.Mvc/AspNetMvcControllerFactory.cs

+
+		public SessionStateBehavior GetControllerSessionBehavior(RequestContext requestContext, string controllerName)
+		{
+			return SessionStateBehavior.Default;


I don't know what this stuff does but do we not care if someone applies an SessionStateAttribute to their controller?

Yes, you are totally right we should follow best practice and set this to disabled.

Just kidding, we might need to pass this as a parameter via the public api. Looks like the IControllerFactory demands it for now.

Additional stuff about what it gives you. You basically end up with a Session State object. It tracks the browser and reconciles http/s requests with a user session which then allows you to store user profile information that is retrievable on each subsequent request using a dictionary basically. I have seen this abused also, projects where people try and cache the entire database in this thing where worker processes very quickly jump to 2GB. You can also easily back this store onto something like redis or sql very easily. It is also the first abuse that raises it's head when you migrate a website running on tin in the basement to the cloud.

Addressed in commit: 9c8aa17

Don't worry I know what session and view state are, and also seem them heavily abused in the wild. I just had no idea what this enum does here, it sort of sounds like the "handler" (i.e. the controller factory) is the one telling ASP.NET what to do as the fallback, and this is per controller.

jonorossi · 2017-10-24T05:39:36Z

src/Castle.Facilities.AspNet.Mvc/AspNetMvcControllerFactory.cs

+
+		private Type FindControllerType(string controllerName)
+		{
+			foreach (var type in controllerAssemblies.SelectMany(x => x.ExportedTypes))


I still think this needs caching like DefaultControllerFactory, otherwise we'll be searching every type in the specified assemblies on every request.

Good point, addressed in commit: ed5c4c1

Most of the linq stuff should be cached.

Rather than searching a list and hitting reflection's Name property, what about a dictionary?

jonorossi · 2017-10-24T05:40:27Z

src/Castle.Facilities.AspNet.Mvc.Tests/AspNetMvcFacilityTestCase.cs

+	using NUnit.Framework;
+
+	[TestFixture]
+    public class AspNetMvcFacilityTestCase


There are formatting problems in many of these unit test files.

Hopefully addressed in: 40213c2

Man! this is a constant battle :)

Have you got the EditorConfig VS extension installed?

jonorossi · 2017-10-24T05:43:12Z

src/Castle.Facilities.AspNet.WebApi/AspNetWebApiDependencyResolver.cs

+
+		public void Dispose()
+		{
+			this.facilitySupport.Kernel.Dispose();


Am I missing something here, you dispose the container?

jonorossi · 2017-10-24T05:48:03Z

There is definitely a breaking change here with lifestyletype. So v5. I also dont want to release this until we have done the work on the AspNetCore implementation. I would also like to move on to the deprecation story before we release v5.

v5 is going to be a fairly large release in my mind.

Great, we'll talk about how we want to do v5 development in another thread, but I think we make master v5 and we can have a release branch for maintenance and to accept PRs that we want to release on the v4 line.

jonorossi · 2017-10-24T05:51:16Z

FYI if you didn't see it, Microsoft has added a new API to .NET Framework 4.7.1 so you can use call context with ASP.NET and join ASP.NET to move your ambient data when it transitions to another thread/call context.

Not something we'd want to use, however it does still show that ASP.NET is thread agile but only outside each execution step.

ASP.NET Execution Step Feature
ASP.NET processes requests in its predefined pipeline which includes 23 events. ASP.NET executes each event handler as an execution step. With this new ExecutionStepInvoker feature, developers will be able to run this execution step inside their code.
Today ASP.NET can’t flow the execution context due to switching between native threads and managed threads. ASP.NET selectively flows only the HttpContext which may not be sufficient for ambient context scenarios. With this feature we enable modules to restore ambient data. The ExecutionStepInvoker is intended for libraries that care about the execution flow of the application (tracing, profiling, diagnostics, transactions, etc.).
We have added a new API to enable this: HttpApplication.OnExecuteRequestStep(Action<HttpContextBase, Action> callback)
(https://blogs.msdn.microsoft.com/dotnet/2017/10/17/announcing-the-net-framework-4-7-1/)

ghost · 2017-10-31T21:07:55Z

FYI if you didn't see it, Microsoft has added a new API to .NET Framework 4.7.1 so you can use call context with ASP.NET and join ASP.NET to move your ambient data when it transients to another thread/call context.

It comes across as: "Hey we have some framework shit for tracking ambient context's, seriously just trust us (and roll on the unicorns)". I cannot actually nail this down to a commit on github, get mostly referencesource links. Can you turn up anything?

ghost · 2017-10-31T21:17:55Z

Great, we'll talk about how we want to do v5 development in another thread, but I think we make master v5 and we can have a release branch for maintenance and to accept PRs that we want to release on the v4 line.

This sounds good to me.

jonorossi · 2017-11-01T01:45:37Z

It comes across as: "Hey we have some framework shit for tracking ambient context's, seriously just trust us (and roll on the unicorns)". I cannot actually nail this down to a commit on github, get mostly referencesource links. Can you turn up anything?

.NET Framework is still closed source, referencesource is all you'll get. I wouldn't worry about it, I don't think we want a dependency on .NET Framework 4.7.1.

jonorossi · 2017-12-07T11:12:19Z

@fir3pho3nixx sorry for being so slack with reviewing this, the first time I looked I thought I misunderstood the example and needed to look at it again with fresh eyes. Looking again I still am missing how the example at https://github.com/fir3pho3nixx/Windsor.NestedScopes handles resolving "web request scoped" components inside a scope inside the web request scope, your example with ServiceA and ServiceB show a UoW pattern doing exactly what I was wondering how you avoid.

How do you register a component for "per web request" like you can now that will stay singleton-like for lifetime of the web request without it creating a new instance in a nested scope.

That is why my comment on Oct 18th was talking about maybe we need "tagged scopes" or something so you can register a component using .LifestyleScoped("tagname") and the .LifestylePerWebRequest extension method would use a predefined tag name.

Having a very quick look at the Autofac documentation they appear to "tag" scopes so resolution inside that nested scope will know to step outwards to find the scope with the same tag that it was registered with, their InstancePerRequest is really an InstancePerMatchingLifetimeScope with a tag.

ghost · 2017-12-07T11:30:45Z

@jonorossi - No problem been quite busy too.

How do you register a component for "per web request" like you can now that will stay singleton-like for lifetime of the web request without it creating a new instance in a nested scope

I would simply say don't nest your scopes. You have to remember, scopes are implicitly per web request just because of how the resolver's/factories work, if you opt for further nesting then you are breaking out into a more granular lifestyle within that request.

I am a bit worried that we are over complicating this for a rare if not altogether not needed use case. Do you think it would be good if we try and have a chat about it on google hangouts to improve my understanding of this and then decide what needs to be done?

jonorossi · 2017-12-07T11:38:59Z

I am a bit worried that we are over complicating this for a rare if not altogether not needed use case. Do you think it would be good if we try and have a chat about it on google hangouts to improve my understanding of this and then decide what needs to be done?

Yep, happy to discuss on Hangouts, I'm online now if now suits you, otherwise try to ping me on Hangouts when I'm online over the next couple of days. This feature/use case might be unnecessary but it appears there are no real workarounds to implement this feature yourself, or maybe I'm just missing something.

ghost · 2018-01-06T21:24:34Z

@jonorossi - I have done some more research and think I need to raise an issue to discuss it more. Can you give me a little time to prepare this? I do believe there are good reasons why you would not want to allow this to happen in it's current form(or at least the way AF does it).

** Edited **

There might be another way of getting this behaviour and it could be done using the existing API's. I need to code up an example to be sure.

** Edited **

Windsor does not support this functionality without following the docs closely and finding the right extension point to implement it.

We need an issue that implements "named scopes using name resolved scoped lifestyles". There are going to be problems when it comes to drafting requirements for this. I can already tell you that late bound components have weird behaviours with burdens depending on whether they implement disposable or not using Transient lifestyles. Please see ref: #373

We need to come up with some test cases for how scoped lifestyles work then how we have to decide how we implement "named scopes". We also need to consider permutations around typed factories vs default microkernel.

** Edited **

We can harmonise the projects after we get through this.

jonorossi · 2018-01-22T16:08:43Z

@fir3pho3nixx I don't know if I'm quite following all the edits, but is this PR now ready for a final review and ready to be merged? Happy to defer a possible named scope feature to later, should we add a warning about using nested scopes and these facilities to the docs?

jonorossi · 2018-01-22T16:10:17Z

appveyor.yml

@@ -42,6 +42,9 @@ on_success:
        nuget push ".\build\Castle.Windsor.${env:APPVEYOR_BUILD_VERSION}.nupkg" -ApiKey $env:NUGET_API_KEY -Source https://api.nuget.org/v3/index.json
        nuget push ".\build\Castle.LoggingFacility.${env:APPVEYOR_BUILD_VERSION}.nupkg" -ApiKey $env:NUGET_API_KEY -Source https://api.nuget.org/v3/index.json
        nuget push ".\build\Castle.EventWiringFacility.${env:APPVEYOR_BUILD_VERSION}.nupkg" -ApiKey $env:NUGET_API_KEY -Source https://api.nuget.org/v3/index.json
+        nuget push ".\build\Castle.AspNet.SystemWebFacility.${env:APPVEYOR_BUILD_VERSION}.nupkg" -ApiKey $env:NUGET_API_KEY -Source https://api.nuget.org/v3/index.json


There are still some remnants of Castle.AspNet.xxxFacility naming in this PR.

jonorossi · 2018-01-22T16:18:20Z

Happy to defer a possible named scope feature to later, should we add a warning about using nested scopes and these facilities to the docs?

Hmmm, I think I was talking crap, the new .LifestylePerWebRequest() uses the WebRequestScopeAccessor so maybe my concern was ill founded and this actually results in the same behaviour as before, which is perfect.

ghost · 2018-01-22T17:42:23Z

@jonorossi - Welcome back! I will fix the rubbish name in the appveyor.yml. Let me prep this for merge.

ghost · 2018-01-22T20:13:28Z

CHANGELOG.md

+
+New Features:
+- Created AspNetMvc facility to support Mvc web applications on desktop clr. (@fir3pho3nixx, #283)
+- Created AspNetWebApi facility to support WebApi web hosted and self hosted applications on desktop clr. (@fir3pho3nixx, #283)


The need appropriate names

ghost · 2018-01-22T20:14:47Z

buildscripts/build.cmd

@@ -47,6 +53,9 @@ dotnet build Castle.Windsor.sln -c %Configuration%
 .\tools\Explicit.NuGet.Versions\build\nev.exe ".\build" "castle.eventwiringfacility"
 .\tools\Explicit.NuGet.Versions\build\nev.exe ".\build" "castle.factorysupportfacility"
 .\tools\Explicit.NuGet.Versions\build\nev.exe ".\build" "castle.loggingfacility"
+.\tools\Explicit.NuGet.Versions\build\nev.exe ".\build" "castle.aspnet.systemwebfacility"
+.\tools\Explicit.NuGet.Versions\build\nev.exe ".\build" "castle.aspnet.mvcfacility"
+.\tools\Explicit.NuGet.Versions\build\nev.exe ".\build" "castle.aspnet.webapifacility"


These need to be renamed ... whoops

ghost · 2018-01-22T20:18:28Z

docs/mvc-tutorial-part-7-lifestyles.md

@@ -16,7 +16,7 @@ That's however as far as we can get for what we want from `ISession`. When it's

 ### The `PerWebRequest` lifestyle

-To change the `ISession` lifestyle to be per web request, we need to specify that in the registration. So we need to change it to the following:
+Please install the `Castle.AspNet.SystemWebFacility` NuGet first. To change the `ISession` lifestyle to be per web request, we need to specify that in the registration. So we need to change it to the following:


Needs appropriate name...

ghost · 2018-01-22T20:18:56Z

docs/systemweb-facility.md

+
+- Assembly reference to System.Web
+- NuGet reference for [Microsoft.Web.Infrastructure](https://www.nuget.org/packages/Microsoft.Web.Infrastructure/)
+- NuGet reference for [Castle.AspNet.SystemWebFacility](https://www.nuget.org/packages/Castle.AspNet.SystemWebFacility/)


Need appropriate name...

ghost · 2018-01-22T20:20:29Z

docs/windsor-for-structuremap-users.md

@@ -173,6 +173,8 @@ HttpContext | PerWebRequest |
 HttpSession | None/Custom | There's no direct equivalent in Windsor for this lifestyle, but implementing one is trivial
 Hybrid | None/Custom | There's no direct equivalent in Windsor for this lifestyle, but implementing one is trivial

+Please note that for `PerWebRequest` in Windsor you will need to install the `Castle.AspNet.SystemWebFacility` package from NuGet. 


Appropriate name ...

ghost · 2018-01-22T20:24:38Z

src/Castle.Windsor/Core/ScopedAttribute.cs

@@ -21,7 +21,7 @@ namespace Castle.Core
 	///   per thread lifestyle.
 	/// </summary>
 	[AttributeUsage(AttributeTargets.Class, AllowMultiple = false)]
-	public sealed class ScopedAttribute : LifestyleAttribute
+	public class ScopedAttribute : LifestyleAttribute
 	{
 		/// <summary>
 		///   Initializes a new instance of the <see cref = "PerThreadAttribute" /> class.


XML comments need updating ...

@mario-d-s

Special thanks to @mario-d-s, @masterpoi and @dotnetjunkie participating in the discussion @ #283. Update logging facility NuGet package description by @jonorossi Add info on using performance counters by @jonorossi

ghost · 2018-01-22T21:17:31Z

Hmmm, I think I was talking crap, the new .LifestylePerWebRequest() uses the WebRequestScopeAccessor so maybe my concern was ill founded and this actually results in the same behaviour as before, which is perfect.

Maybe but I think you were on to something. You have highlighted something that AutoFac solves and Windsor kind of does. "Named Scopes". The worry I have with this it has overriding lifestyle side effects unless we defined proper rules for this, hesitant to implement. AspNet team went with this approach and formalised it in their scoped depedencies. It also earned us the title of "non-conforming" container. Yukk.

Let's watch the chatter and see if anyone asks for it. Until then my answer is:

ghost · 2018-01-22T21:24:03Z

Happy to defer a possible named scope feature to later, should we add a warning about using nested scopes and these facilities to the docs?

Yes, we need a proper explanation here. I will add this tomorrow night. DNM = Do not merge.

** Edited **

Need more time here. Started but not finished.

ghost · 2018-01-24T13:21:19Z

@jonorossi - I think we are ready, if you could give it one last look I would much appreciate it.

…pclr

jonorossi

Looks great. My only comment is the PerWebRequest() extension method (LifestyleRegistrationExtensions) and PerWebRequestAttribute are in a new namespace, if we put them in their previous namespace (the file stays where it, just the namespace changes) it would decrease the amount of breaking changes for users when they upgrade because as long as they've referenced the new assembly their using statements will be sufficient otherwise users will have to hop around their installers to add another using statement. Thoughts?

I'm going to merge this though 🎉

ghost · 2018-01-27T16:19:40Z

Looks great. My only comment is the PerWebRequest() extension method (LifestyleRegistrationExtensions) and PerWebRequestAttribute are in a new namespace, if we put them in their previous namespace (the file stays where it, just the namespace changes) it would decrease the amount of breaking changes for users when they upgrade because as long as they've referenced the new assembly their using statements will be sufficient otherwise users will have to hop around their installers to add another using statement. Thoughts?

No problem, I will fix this in a separate commit on the aspnetcore branch.

ghost mentioned this pull request Oct 8, 2017

Facility aspnet.systemweb #350

Closed

jonorossi requested changes Oct 13, 2017

View reviewed changes

jonorossi mentioned this pull request Oct 13, 2017

OWIN-compatible implementation for "PerWebRequest" lifestyle #283

Closed

ghost mentioned this pull request Oct 17, 2017

ASPNET Facilities Implementation #355

Closed

jonorossi reviewed Oct 24, 2017

View reviewed changes

ghost mentioned this pull request Nov 22, 2017

Can be done differently the implementation of the DependencyScope? #366

Open

ghost mentioned this pull request Jan 17, 2018

Fix XML documentation generation and add Markdown docs to VS solution castleproject/Core#315

Closed

jonorossi reviewed Jan 22, 2018

View reviewed changes

ghost commented Jan 22, 2018

View reviewed changes

Facilities for SystemWeb, Mvc and WebApi

d3dcf64

Special thanks to @mario-d-s, @masterpoi and @dotnetjunkie participating in the discussion @ #283. Update logging facility NuGet package description by @jonorossi Add info on using performance counters by @jonorossi

Updated docs for aspnet facilities

79726fe

Gavin van der Merwe added 3 commits January 24, 2018 20:42

Updated docs for aspnet facilities

cc8ff7f

Merge remote-tracking branch 'upstream/master' into facilities-deskto…

2da71e7

…pclr

Adding lifestyles doc

1434538

jonorossi approved these changes Jan 26, 2018

View reviewed changes

jonorossi merged commit 6293ad8 into castleproject:master Jan 26, 2018

jonorossi added this to the vNext milestone Jan 26, 2018

ghost mentioned this pull request Jan 27, 2018

ASP.NET Core facility #376

Merged

jonorossi modified the milestones: vNext, v5.0 May 31, 2018

ghost deleted the facilities-desktopclr branch June 22, 2018 00:21

		@@ -1,4 +1,4 @@
		// Copyright 2004-2012 Castle Project - http://www.castleproject.org/
		// Copyright 2004-2011 Castle Project - http://www.castleproject.org/

Facilities SystemWeb, Mvc and WebApi #351

Facilities SystemWeb, Mvc and WebApi #351

Conversation

ghost commented Oct 8, 2017

ghost commented Oct 11, 2017

ghost commented Oct 11, 2017 • edited by ghost

jonorossi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Oct 14, 2017 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 14, 2017

ghost commented Oct 14, 2017

jonorossi commented Oct 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Oct 31, 2017 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonorossi commented Oct 24, 2017

jonorossi commented Oct 24, 2017 • edited

ghost commented Oct 31, 2017 • edited by ghost

ghost commented Oct 31, 2017

jonorossi commented Nov 1, 2017

jonorossi commented Dec 7, 2017

ghost commented Dec 7, 2017

jonorossi commented Dec 7, 2017

ghost commented Jan 6, 2018 • edited by ghost

jonorossi commented Jan 22, 2018

Choose a reason for hiding this comment

jonorossi commented Jan 22, 2018

ghost commented Jan 22, 2018 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 11, 2017 •

edited by ghost

ghost Oct 14, 2017 •

edited by ghost

ghost Oct 31, 2017 •

edited by ghost

jonorossi commented Oct 24, 2017 •

edited

ghost commented Oct 31, 2017 •

edited by ghost

ghost commented Jan 6, 2018 •

edited by ghost

ghost commented Jan 22, 2018 •

edited by ghost

ghost commented Jan 22, 2018 •

edited by ghost