ASP.NET Core Facility (now production ready BUT... ) #394

ghost · 2018-04-15T18:00:11Z

This is the final PR for the ASP.NET Core Facility. The previous design did not support the @Inject Razor directive. The problem was a difficult one of timing registrations in the right places and introducing the notion of Cross Wiring.

Cross wiring is provided as an extension method to component registration which create's late bound registrations in the IServiceCollection(which are basically factories) that are backed on to a Windsor Container.Resolve. This removes the burden of Castle Windsor users of having to maintain 2 sets of registration logic and moves them toward the Windsor registration API. It also makes things like the @Inject Razor directive work.

@generik0 and I also checked to make sure that we have fixed all the problems with generics, I think there was a gremlin in the previous implementation. It got magically fixed when I re-wrote the generic implementation with additional unit tests.

This code has also endured a significant bashing to make sure it does not have any memory leaks by using the performance tests in this repo. I am satisfied that for scoped lifestyles(Controller, ViewComponents, TagHelpers and cross wired @Inject consumers in the view) there are no memory leaks(roughly 300MB foot print) and it serves 250+ requests/second. This is great for anyone using this in docker.

We need to start thinking about releasing V5. @jonorossi did you have a timescale in mind?

jonorossi

Great work guys, I've added some minor comments, some for me to understand the design decisions better.

jonorossi · 2018-04-20T12:06:47Z

docs/aspnetcore-facility.md

@@ -51,7 +91,7 @@ Here is a complete example:
 ```csharp
 public class Startup
 {
-	private readonly WindsorContainer container = new WindsorContainer();
+	public static readonly WindsorContainer container = new WindsorContainer();


Is having this public in the example a good idea?

This should be private. Agreed.

jonorossi · 2018-04-20T12:09:53Z

src/Castle.Facilities.AspNetCore/Castle.Facilities.AspNetCore.csproj

@@ -17,6 +17,10 @@
 		<RootNamespace>Castle.Facilities.AspNetCore</RootNamespace>
 	</PropertyGroup>

+	<ItemGroup>
+	  <None Remove="Resolvers\FrameworkConfigurationDependencyResolver.cs~RF4e009575.TMP" />


Looks like some junk added to the project file.

Good spot, will remove.

jonorossi · 2018-04-20T12:13:59Z

src/Castle.Facilities.AspNetCore/CastleWindsorRegistrationExtensions.cs

+			InstallFrameworkIntegration(services, container);
+
+			var options = new CastleWindsorRegistrationOptions(container, services);
+			(configure ?? ((o) => { }))(options);


This reads really poorly and would invoke that lambda even though it does nothing, could you change this to an if statement with a null check.

I will do. It is just lambda parameter based null coalescence to avoid null reference exceptions. Is this bad? Can I use null coalescence for invalid arguments?

Can I use null coalescence for invalid arguments?

Yes, definitely, those look great to handle the throw exception case.

It is just lambda parameter based null coalescence to avoid null reference exceptions. Is this bad?

Other than readability my concern is that doing it this way actually runs more code at runtime and that empty lambda gets emitted as a compiler named method.

configure?.Invoke(options) is much better, and it happens that it is the exact same IL with the other syntax.

jonorossi · 2018-04-20T12:20:23Z

src/Castle.Facilities.AspNetCore/CastleWindsorRegistrationExtensions.cs

 		{
-			var assembly = typeof(TTypeAssembly).Assembly;
-			UseCastleWindsor(container, assembly);
+			registration.LifeStyle.Is(lifestyleType);


The comment says only 3 lifestyle types are supported, should this check nothing else is passed in?

Yes we should design by contract. Do you know of any way we could integrate the other types? I personally have never needed them in a web scenario.

Do you know of any way we could integrate the other types?

Someone could create any sort of custom lifestyle type, not sure you'll be able to handle that with this facility, probably just worth throwing on unsupported lifestyle until we need them and have a proper use case.

jonorossi · 2018-04-20T12:20:41Z

src/Castle.Facilities.AspNetCore/CastleWindsorRegistrationExtensions.cs

+				});
+			}
+
+			if (lifestyleType == LifestyleType.Scoped)


Probably should be an else if.

Or do we throw LifestyleNotSupportedException at https://github.com/fir3pho3nixx/Windsor/blob/aspnet-core-windsor-final/src/Castle.Facilities.AspNetCore/CastleWindsorRegistrationExtensions.cs#L186?

We need to figure out #394 (comment) before we decide this.

Yep, throw that exception in the else statement 👍.

jonorossi · 2018-04-20T12:24:08Z

src/Castle.Facilities.AspNetCore/Resolvers/FrameworkDependencyResolver.cs

+		}
+	}
+
+	public static class GenericTypeExtensions


Is it a good idea making GenericTypeExtensions and ServiceCollectionExtensions public? Are you using them in unit tests too?

Is it a good idea making GenericTypeExtensions and ServiceCollectionExtensions public?

No, none at all. Purely muscle memory kicking in here.

Are you using them in unit tests too?

No, I will move this over ...

GenericTypeExtensions is integration tested by:

https://github.com/fir3pho3nixx/Windsor/blob/aspnet-core-windsor-final/src/Castle.Facilities.AspNetCore.Tests/AspNetCoreFacilityTestCase.cs

https://github.com/fir3pho3nixx/Windsor/blob/aspnet-core-windsor-final/src/Castle.Facilities.AspNetCore.Tests/Resolvers/ServiceCollectionExtensionsTestCase.cs

No, I will move this over ...

No problem. I wasn't worried about them being unit tested, more wondering if they were public because the unit tests also used them.

jonorossi · 2018-04-20T12:38:15Z

docs/aspnetcore-facility.md

+
+```csharp
+// Add custom middleware, do this if your middleware uses DI from Windsor
+app.UseCastleWindsorMiddleware<CustomMiddleware>(container);


UseCastleWindsorMiddleware sounds like it is adding some special Windsor middleware to the pipeline, I was wondering if it should be named something like UseMiddlewareViaWindsor<TMiddleware> or UseMiddlewareFromWindsor<TMiddleware>?

I know this was there from the first cut but now is the best time to make the change. I was wondering what Autofac named theirs and I can't find how they do this same sort of thing. @fir3pho3nixx you are the expert here so I'll let you decide whether you want to rename it.

@jonorossi I really like this. It makes it discoverable via code completion.

I love UseMiddlewareViaWindsor<TMiddleware>, the responsible choice is definitely UseMiddlewareFromWindsor<TMiddleware>.

I am really happy with how we are refining the UX here.

Great, especially that it'll be more discoverable.

Were you planning to name it UseMiddlewareFromWindsor or UseMiddlewareFromCastleWindsor. I was wondering if the "Castle" bit should be dropped from AddCastleWindsor and UseCastleWindsor since it doesn't really add anything useful?

You will be pleased to know, UseCastleWindsor is not a thing anymore. We now only need AddWindsor :). Done.

Oh and UseMiddlewareFromWindsor is the new name for the middleware extension.

jonorossi · 2018-04-20T12:39:13Z

docs/aspnetcore-facility.md

+
+```csharp
+container.Register(Component.For<IUserService>().ImplementedBy<AspNetUserService>()
+	.CrossWired(container, services, LifestyleType.Scoped));


It sucks that you have to specify the container in CrossWired but I know why.

Yup, this sucked hard.

Any suggestions?

Other than being a little ugly, one problem with this approach is that CrossWired registers the service in the ASP.NET Core container before it is even registered in Windsor.

What about using a ComponentModel construction contributor (IContributeComponentModelConstruction) that does the cross wiring after the ComponentModel is built. The WCF facility has one (WcfBehaviorInspector) to set up a custom activator, as do most facilities including typed factory, startable, event wiring, synchronize, and built in things like mixins.

Other than being a little ugly, one problem with this approach is that CrossWired registers the service in the ASP.NET Core container before it is even registered in Windsor.

This raises a really important question about how people use Windsor. I hung this extension off ComponentRegistration because I assumed it would be called within the context of Container.Register(where I might be getting it wrong). The cross wiring here is merely registering late bound components with factories(backed on to Container.Resolve) in the IServiceCollection. My assumption was eventually the service will be registered in both places.

What about using a ComponentModel construction contributor

Yes, this would be hard. What CrossWiring is really trying to solve, is making types registered through Windsor available to framework code buried behind IServiceCollection/ServiceProvider logic like the MVC Razor @Inject directive. This is a registration time consideration. I am worried that construction contribution does not consider the registration requirements and anything you can do during resolve to implement this might already be too late.

That said, I still think we should wrangle with this one a little more to tidy it up. I am just not sure what that would be yet.

If you have any more questions about cross wiring, please fire away.

Other than being a little ugly

Yes, it is nasty.

Yes, this would be hard. What CrossWiring is really trying to solve, is making types registered through Windsor available to framework code buried behind IServiceCollection/ServiceProvider logic like the MVC Razor @Inject directive. This is a registration time consideration. I am worried that construction contribution does not consider the registration requirements and anything you can do during resolve to implement this might already be too late.

Not sure I follow why this would be hard, I understand that you are doing late bound components but ComponentModel construction contributors are run directly after construction of the ComponentModel (before container.Register() returns), not at resolve time, see the docs.

The WCF facility uses ComponentRegistration to set up services, and after they are registered in the container it'll do a bunch of other stuff. The Startable facility is just the same, you don't have to implement IStartable and can use the ComponentRegistration methods and its CMCC adds a start/stop concern if a new component is startable.

Ok, let’s try it this way round. What would it contribute to resolves from IServiceCollection/IServiceProvider?

jonorossi · 2018-04-20T12:41:56Z

docs/aspnetcore-facility.md

+{
+	opts.RegisterControllers(typeof(HomeController).Assembly, LifestyleType.Transient);
+	opts.RegisterTagHelpers(typeof(EmailTagHelper).Assembly, LifestyleType.Transient);
+	opts.RegisterViewComponents(typeof(AddressComponent).Assembly, LifestyleType.Transient);


I'm sure you thought about it or used a pattern from somewhere, but could you explain why the class that you perform actions on is named "options".

Yeah, this is what it means from an HTTPS point of view.

public static void Main(string[] args) { BuildWebHost(args).Run(); } public static IWebHost BuildWebHost(string[] args) => WebHost.CreateDefaultBuilder(args) .UseStartup<Startup>() .UseKestrel(options => { options.Listen(IPAddress.Loopback, 5000); options.Listen(IPAddress.Loopback, 5001, listenOptions => { listenOptions.UseHttps("testCert.pfx", "testPassword"); }); }).Build();

It is a convention.

We can clean this up to make options more stateful but it would mean adding more code.

I'll leave it up to you to choose but it just felt a little strange that it was actually doing something rather than just holding info on what the user wanted until calling AddCastleWindsor just like like the above does start listening until calling UseKestrel or actually probably in WebHost.Run.

Couldn't you change AddApplicationComponentsToWindsor so rather than looking at boolean members like ControllersAlreadyRegistered, have members typed List<Tuple<Assembly, LifestyleType>>? It should be about the same amount of code and that way it has the register code once inside AddApplicationComponentsToWindsor and it'll either do the stored list or use options.EntryAssembly.

Do you know what, this was not that bad. I added it in. It did bloat the AddApplicationComponentsToWindsor a little but not as bad as I originally thought.

jonorossi · 2018-04-20T12:43:25Z

src/Castle.Facilities.AspNetCore/CastleWindsorRegistrationExtensions.cs

+			}
+		}
+
+		internal bool ControllersAlreadyRegistered => controllersAlreadyRegistered;


Why not just define internal auto properties with a private setter?

Yep can do.

These are gone now, but yes, the new implementation works this way.

ghost · 2018-04-20T23:22:36Z

@jonorossi good to have you back! 😀

jonorossi · 2018-04-23T12:45:10Z

src/Castle.Facilities.AspNetCore/Exceptions/LifestyleNotSupportedException.cs

+	/// <summary>
+	/// For when an unsupported <see cref="LifestyleType"/> get passed to the <see cref="CastleWindsorRegistrationExtensions.CrossWired{TService}(Castle.MicroKernel.Registration.ComponentRegistration{TService},Castle.Windsor.IWindsorContainer,Microsoft.Extensions.DependencyInjection.IServiceCollection,Castle.Core.LifestyleType)"/> extension method.
+	/// </summary>
+	public class LifestyleNotSupportedException : Exception


Is a custom exception really necessary for a situation no one is going to specifically catch, I thought you were just going to use a NotSupportedException or even better a ArgumentException.

Thought a custom exception with a namespace and a custom message would make the stack trace a little cleaner. Will change it back.

jonorossi · 2018-04-23T13:16:42Z

src/Castle.Facilities.AspNetCore/CastleWindsorRegistrationExtensions.cs

+	/// <summary>
+	/// For overriding default registration and lifestyles behaviour
+	/// </summary>
+	public class CastleWindsorRegistrationOptions


Should these classes also drop the "Castle" bit?

Yup. Definitely.

jonorossi · 2018-04-23T13:18:29Z

src/Castle.Facilities.AspNetCore/CastleWindsorRegistrationExtensions.cs

+
+			foreach (var controllerRegistration in options.TagHelperRegistrations)
+			{
+				container.Register(Classes.FromAssemblyInThisApplication(controllerRegistration.Item1).BasedOn<Controller>().Configure(x => x.LifeStyle.Is(controllerRegistration.Item2)));


Should this and view components be based on a different class other than Controller, I think you had this right before you recent changes.

And here come some tests. The history of this code is something that vexed me.

generik0 · 2018-04-25T06:45:10Z

Oh yes, and in here use a factory to resolve the binder?

Or do you think I could make my own resolver instead to the provider?

public class AuthorEntityBinderProvider : IModelBinderProvider
    {
        public IModelBinder GetBinder(ModelBinderProviderContext context)
        {
            if (context == null)
            {
                throw new ArgumentNullException(nameof(context));
            }

            if (context.Metadata.ModelType == typeof(Author))
            {
                return new BinderTypeModelBinder(typeof(AuthorEntityBinder));
            }

            return null;
        }
    }

ghost · 2018-04-25T07:30:25Z

@generik0, let me check and get back to you. My suspicion is that this does not get run through the ServiceProvider so CrossWiring might not be an option. The same problem would exist for filters.

generik0 · 2018-04-29T11:09:13Z

@fir3pho3nixx update about providers. Found this link, hence i think the answer is simple and as suspected. I need to do a cross wire of sorts and let then container resolve the provider.
In the provider, use factories for anything that needs to be resolver transient or scope

https://stackoverflow.com/questions/44598683/how-to-use-simple-injector-to-inject-a-class-into-my-custom-model-binder-provide?utm_medium=organic&utm_source=google_rich_qa&utm_campaign=google_rich_qa

ghost · 2018-05-20T11:12:27Z

@fir3pho3nixx update about providers. Found this link, hence i think the answer is simple and as suspected. I need to do a cross wire of sorts and let then container resolve the provider.
In the provider, use factories for anything that needs to be resolver transient or scope

Looks like the answer is to use service location. In our implementation you should not need any cross wiring, register it in a vanilla way with Windsor. Should work. I would expect the same thing would need to happen for filters.

@generik0

After the chat on issue #120, I did manage to fix a whole heap of problems with our implementation.

Using the BuilderServiceProvider method in the resolvers was in fact new'ing up a new container. This was definitely causing singletons to double up because aspnet/hosting was already creating one. After debugging the Startup.ConfigureServices method all the way down to hosting, I noticed the safest way forward here was to return it from the AddWindsor extension with an optional factory method where the user can create it themselves if they choose.
Disposables for cross wired component's were all over the place. In some cases things were getting disposed twice and in other cases not at all. This must have caused a few headaches on your end. I added a significant amount of unit testing around this to fix it.
We still have to disable scope validation for the ServiceProvider, unfortunate but if we are to do this properly then we would have to start managing 2 sets of scopes. I am hesitant to add this in just yet until I can research the problem slightly better.

@jonorossi

I think we have bottomed out the torn lifestyle problem. There are some quirky looking unit test's in here but they validate my observations from running it in https://github.com/fir3pho3nixx/Windsor.AspNetCore. There ain't no free lunches up in here that is for sure :) I think I am ready to start finishing the work of improving the cross wire extension to make it a lot less sucky. Should get to it next week.

ghost · 2018-05-20T18:28:47Z

docs/aspnetcore-facility.md

+This method also adds a sub resolver for dealing with the resolution of ASP.NET Core framework types. An example might be something like an 
+[ILoggerFactory](https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.logging.iloggerfactory?view=aspnetcore-2.0). 
+It is important to note that this extension also injects custom middleware for the management of implicitly scoped lifestyles. It will also 
+dispose the implicit scope once the request is completed.


Might need to clarify that these are windsor scopes.

ghost · 2018-05-20T18:48:36Z

src/Castle.Facilities.AspNetCore/WindsorRegistrationExtensions.cs

+			}
+			else if (lifestyleType == LifestyleType.Scoped)
+			{
+				registration.Activator<CrossWiredComponentActivator>();


Scoped cross wired components are released by implicit scopes within the ServiceProvider not the WindsorContainer.

ghost · 2018-05-20T18:49:41Z

src/Castle.Facilities.AspNetCore/WindsorRegistrationExtensions.cs

+			}
+			else if (lifestyleType == LifestyleType.Singleton)
+			{
+				registration.Activator<CrossWiredComponentActivator>();


Singleton cross wired components are released by disposing the ServiceProvider not the WindsorContainer.

ghost · 2018-05-26T15:03:02Z

@jonorossi - I have fixed the cross wiring API to be much cleaner. The component model contributor with custom activator turned out to hold the answer all along. Thanks.

Could you please take another look?

ghost · 2018-06-01T17:35:52Z

@generik0 are you happy with this cut? We are dog fooding and are running this in prod. @jonorossi has a lot of traffic on his hands. I don’t expect he will humanly be able to get to this ... won’t merge unless you say it is ok.

generik0 · 2018-06-01T19:06:40Z

Hi @fir3pho3nixx.
I have been killing myself the last three weeks with a major release @work.
But i beleive I will have time to look at it in the weekend :-)

ghost · 2018-06-03T12:27:08Z

@generik0 thanks for having a look, I think we are nearly there. I am going to start squishing these commit's down as soon as I get some free time.

generik0 · 2018-06-03T13:19:53Z

No thank you @fir3pho3nixx
Really great work. Looks nice and clean. As clean as can be taking into considerations the limitations posed by aspnetcore / MS DI. Really great work.
I am integrating the framework as we speak. I will report back if i find any isses.

Thanks again, good work! Regards, G.

ghost · 2018-06-04T23:28:29Z

@jonorossi

Could you please kindly take another look. Think we are done here for now. Can we merge?

jonorossi · 2018-06-06T14:46:10Z

src/Castle.Facilities.AspNetCore/Contributors/CrossWiringComponentModelContributor.cs

+
+		public void ProcessModel(IKernel kernel, ComponentModel model)
+		{
+			if (model.Configuration.Attributes.Get("aspnet-core-cross-wired") == "yes")


I know some of the facilities don't but it would be good to use a constant for aspnet-core-cross-wired, consider even making a public readonly field like the typed factory.

Just to confirm, we want a facility implementation of the abstract facility right?

There is no need for an IFacility implementing class, just find a sensible place for the constant, probably WindsorRegistrationExtensions.

jonorossi · 2018-06-06T14:48:01Z

src/Castle.Facilities.AspNetCore/WindsorRegistrationExtensions.cs

+		public static ComponentRegistration CrossWired(this ComponentRegistration registration)
+		{
+			registration.Activator<CrossWiredComponentActivator>();
+			registration.Attribute("aspnet-core-cross-wired").Eq("yes");


Just in case you didn't already know, there is an AddAttributeDescriptor method that does the same thing. You could also use bool.TrueString as your constant if you like.

componentRegistration .AddAttributeDescriptor(TypedFactoryFacility.IsFactoryKey, bool.TrueString);

jonorossi · 2018-06-06T14:54:09Z

src/Castle.Facilities.AspNetCore/Activators/CrossWiredComponentActivator.cs

+			{
+				foreach (var concern in steps.Where(x => x.GetType() != typeof(DisposalConcern)))
+				{
+					concern.Apply(Model, instance);


I assume you weren't able to just remove the DisposalConcern from ComponentModel.Lifecycle in CrossWiringComponentModelContributor?

Will LateBoundDecommissionConcerns also cause the same problem? I guess those can't really be used with this facility as it needs to know the component type upfront to cross wire it.

I deliberately left late bound components out of this because I have concerns with how it behaves with singletons and disposables.

We do defer disposal responsibility back to the "ServiceProvider" for some lifestyle concerns. We should at the very least include late bound components.

Let me get back to you on this one. Worthy of being checked, my gut feel is this takes us into typed factory land.

~~We are cool with perf testing here: https://github.com/fir3pho3nixx/Windsor.AspNetCore/commit/fd3afefb5a77f164acd9ab78908a2cb6f7262ad4~~

Fuck sake. We have a memory leak.

Just watched it grow from 220mb to 350mb using typed factories. Che cristo.

@jonorossi it is not fair I eat this on this PR. I have been quite vocal about typed factories. They are rubbish for many reasons. Is it fair to say I do not support typed factories in this PR? Can I sort that next? Not cool that I get held to account for ex-emeritus.

** Where are you guys anyway, help now right or I will re-write your shit **

We have a memory leak.

it is not fair I eat this on this PR. I have been quite vocal about typed factories. They are rubbish for many reasons. Is it fair to say I do not support typed factories in this PR? Can I sort that next? Not cool that I get held to account for ex-emeritus.

** Where are you guys anyway, help now right or I will re-write your shit **

I'm not sure exactly why you are getting so aggressive here, I get that this pull request has gone on a while. Just a polite reminder the only active committers are those listed on the team page, there should be no expectation of others contributing.

That aside, we can definitely merge this not supporting typed factories, we can even release this without supporting typed factories if you want, remember we get to choose the direction and releasing software with known defects or limitations isn't the end of the world.

Thanks @jonorossi. I still believe we need to support typed factories but I know from our issue tracker that comes with even longer tail.

As long as we know what the problems are before we release I am good.

Aside, we had our first use case for a cross wire today and I dropped this PR in and it just worked! Very happy with the current state of this.

The memory leak is small, but it is there. I updated the docs to reflect this.

Please see: https://github.com/fir3pho3nixx/Windsor/pull/25#pullrequestreview-131406376

I assume you weren't able to just remove the DisposalConcern from ComponentModel.Lifecycle in CrossWiringComponentModelContributor?

This has now also been done.

jonorossi

@fir3pho3nixx just a few more comments 😄.

ghost · 2018-06-07T19:16:45Z

@fir3pho3nixx just a few more comments 😄.

I know, I am simultaneously being pulled up by some good guys all over the place. I really would not have it any other way. Thanks.

ghost · 2018-06-07T21:16:03Z

src/Castle.Facilities.AspNetCore/WindsorRegistrationExtensions.cs

+		/// <param name="services">ASP.NET Core service collection from Microsoft.Extensions.DependencyInjection</param>
+		/// <param name="container">Windsor container which IServiceCollection resolves from using factory methods</param>
+		public static void CrossWiresInto(this IWindsorContainer container, IServiceCollection services)
+		{


@generik0 also thought we needed to throw a helpful exception here in case somebody used the CrossWire extension without calling this.

jonorossi · 2018-06-11T13:54:54Z

src/Castle.Facilities.AspNetCore/WindsorRegistrationExtensions.cs

+
+	public static class WindsorRegistrationExtensions
+	{
+		private static bool crossWiresIntoWasCalledBeforeRegistration = false;


Doesn't this have to be called for each container, so this being static wouldn't work for a second container in the process?

Awesome, I need a way of tacking a reference to the IServiceCollection on the container. I was hunting through the internals of the IKernel but then I just got grumpy and tired.

Is there anyway this can be done? That way we don’t smash container life cycles.

Also, there can be only 1 service provider when it comes to the hosting runtime, but I don’t see why we can’t have multiple Windsor containers.

You OK with me adding a general purpose key/value store to the kernel to solve this?

You OK with me adding a general purpose key/value store to the kernel to solve this?

Not really. This is why most facilities will have a facility instance which can hold whatever extra variables it needs and set up the facility in the container, I don't think we want to pollute the container with a generic KV store when we've got a pattern that works well.

I guess if you didn't want that you could have another component model contributor for this facility to check the configuration when you register, and check kernel.ComponentModelBuilder.Contributors.Any(c => c.GetType() == typeof(CrossWiringComponentModelContributor)), however you'd then be better off just always installing the CrossWiringComponentModelContributor and allowing services to be null, and CrossWiringComponentModelContributor will throw when you call Register not when you call CrossWired since it doesn't yet know which container the ComponentRegistration is going to be registered in. Even with a KV store on the container you don't know the container until Register is called, so this has to change.

Ok in this case we should try housing the ServiceCollection reference in a facility first. I am almost 100% certain this will work.

We can use the contributor as a fallback position if we don’t like the way it looks.

Thanks for explaining.

@generik0

I will submit a review. Can you give it one last look. I will highlight the latest changes.

jonorossi

@fir3pho3nixx another minor comment, about crossWiresIntoWasCalledBeforeRegistration.

Once that is done I'll let you do the honours of merging this pull request to master. Feel free to do that when you are happy.

ghost · 2018-06-12T22:50:17Z

src/Castle.Facilities.AspNetCore.Tests/WindsorRegistrationExtensionsAcceptanceTestCase.cs

+			using (var container = new WindsorContainer())
+			{
+				Assert.Throws<InvalidOperationException>(() => { container.Register(Component.For<AnyComponent>().CrossWired()); });
+			}


@generik0

Your suggestion of a helpful exception was implemented here. Thanks!

ghost · 2018-06-12T22:59:02Z

src/Castle.Facilities.AspNetCore/AspNetCoreFacility.cs

+	{
+		internal const string IsCrossWiredIntoServiceCollectionKey = "windsor-registration-is-also-registered-in-service-collection";
+
+		public static readonly AsyncLocal<AspNetCoreFacility> Current = new AsyncLocal<AspNetCoreFacility>();


@jonorossi

Hope you don't mind the ambient state here. Most of the registration, happens in a single thread, so I think we are good. I am a bit worried about doing this as I wont be dog fooding this on net45.

I don't think it makes sense to add the AspNetCoreFacility if and only if you are doing cross-wiring, otherwise it throws if you aren't doing cross-wiring.

I still think this static stuff is completely unnecessary and that the ComponentRegistration should be as stateless as possible, and that the "cross wiring not configured" exception should be throw by CrossWiringComponentModelContributor.ProcessModel after the user calls Register on a container instance, at this point the CMC has access to the IKernel and easy access to AspNetCoreFacility.Services of the facility instance.

I don't think it makes sense to add the AspNetCoreFacility if and only if you are doing cross-wiring, otherwise it throws if you aren't doing cross-wiring.

I hear you. We can definitely wind this back to how it was. No problem. Before we do I just want to make sure I understand this better.

I still think this static stuff is completely unnecessary and that the ComponentRegistration should be as stateless as possible, and that the "cross wiring not configured" exception should be throw by CrossWiringComponentModelContributor.ProcessModel after the user calls Register on a container instance, at this point the CMC has access to the IKernel and easy access to AspNetCoreFacility.Services of the facility instance.

For cross wiring, the CMC needs to be installed in the first place and secondly requires a reference to the IServiceCollection. If the CMC was not installed in the first place how would throwing the helpful exception from inside it help?

Thanks for the tip on the ProcessModel. This would clean up the static reference, I still cannot see how it guard a CrossWire registration from happening unless the CMC was installed first. Does this make sense?

We can definitely wind this back to how it was. No problem. Before we do I just want to make sure I understand this better.

I actually meant to take it further, users always add the facility, and the facility always adds the CWCMC and when it comes across a component that is set for cross wiring it sets it up, or throws if the container isn't configured correctly, the exception will wind up the stack through container.Register.

I actually meant to take it further

Awesome. I like where this is going.

The original CrossWire extension could either extend the IServiceCollection and take the IWindsorContainer as a parameter or vice versa. This was awful. Introducing the CWCMC really went far to solve this problem. It did move the validation problem somewhere else though.

users always add the facility

What we are validating against here is whether someone added the facility or not. Now we have a facility with a configuration callback.

and the facility always adds the CWCMC and when it comes across a component that is set for cross wiring it sets it up

Correct, happy path.

or throws if the container isn't configured correctly

Let's validate against the configuration callback. Not whether the facility is registered.

the exception will wind up the stack through container.Register

That is exactly what we are trying to do. Like it. Thanks for the input.

Summary

We will move the validation down into the facility configuration extension. ie. if you call AddFacility without doing the config callback for f => f.CrossWiresInto(IServiceCollection) we throw helpfully. @generik0 I think this seems perfectly reasonable. We also have the documentation covered. What do you think?

@jonorossi We OK to go ahead?

We will move the validation down into the facility configuration extension. ie. if you call AddFacility without doing the config callback for f => f.CrossWiresInto(IServiceCollection) we throw helpfully. @generik0 I think this seems perfectly reasonable. We also have the documentation covered. What do you think?

It seems like you are agreeing with what I wrote but what I wrote isn't what you are doing, so I'm a little confused.

I thought you were going to make it so you always add the facility even if you don't use cross wiring, then the ComponentRegistration extension methods don't need to check any configuration because they aren't "attached" to a container yet, and only when you call Register with the ComponentRegistration does the container's configuration (i.e. that is has a IServiceCollection set) get checked?

I thought you were going to make it so you always add the facility even if you don't use cross wiring

Correct. Once we established that, I was merely asking @generik0 if we move validation around the facility being present or not down a level of checking whether the IServiceCollection is assigned or not using a CWCMC as you suggested here. Just making sure we are all on the same page.

then the ComponentRegistration extension methods don't need to check any configuration because they aren't "attached" to a container yet.

Yes. You gave me a back reference to the IKernel via the CWCMC here. Facility scan for AspNetCoreFacility and check whether IServiceCollection(non-static) is null. Done. Remove all the validation from ComponentRegistration. Get rid of AsyncLocal.

Have I missed anything?

@fir3pho3nixx yes, I had wrongly misinterpreted your "We OK to go ahead?" with the changes were made and are we ready to go ahead and merge.

Awesome let me get this going. Will implement it tonight. Thanks ;)

ghost · 2018-06-12T23:20:04Z

docs/aspnetcore-facility.md

+```
+
+After rigourous testing we noticed a small memory leak, we are into typed factories here: https://github.com/castleproject/Windsor/issues/410
+


@jonorossi && @generik0

Small memory leak here. Just thought we would call that out. The process creeps up at a glacial pace. We are talking Kb's every hour or so(framework??). Either way, if people are picking this up and using typed factories they should at least have a warning of the stuff we know about.

ghost · 2018-06-12T23:41:32Z

@jonorossi && @generik0

I am not touching this merge button until you guys have had a look.

@anyonewatching

Sorry I am delaying this, but this needs to be right(or as right as it can be) before we release.

generik0 · 2018-06-13T05:30:46Z

@fir3pho3nixx yes I am watch/following all the good work.
I will add the changes tonight and report back.
You have written there is a breaking change. Do I need to do any special changes in my registration?
I will look at you test project for guidens if you don’t have time to answer :-)

generik0 · 2018-06-13T05:33:46Z

@fir3pho3nixx if there is a leak with type factories, should we be using it?
Is it because of the tracking and objects not getting disposed?
Seriously, if there is a memory leak, I think the previous solution was better :-/

ghost · 2018-06-13T07:21:38Z

Do I need to do any special changes in my registration?

Very small change to one line of code on startup.

Container.AddFacility<AspNetCoreFacility>(f => f.CrossWiresInto(services));

The rest should all be the same.

@fir3pho3nixx if there is a leak with type factories, should we be using it? Is it because of the tracking and objects not getting disposed?

It is small but I do notice it creeping up a few Kb's every hour or so. I need a longer running test to be sure. I will be diving into typed factories next to understand this better.

Seriously, if there is a memory leak, I think the previous solution was better :-/

It was always there, we just never tested in anger with late bound component registrations(or typed factories).

jonorossi

Added a few more comments.

jonorossi · 2018-06-13T07:12:01Z

docs/aspnetcore-facility.md

+Container.Register(Component.For<LateBoundTransientDisposable>().UsingFactoryMethod(() => new LateBoundTransientDisposable()).LifestyleTransient().CrossWired());
+```
+
+After rigourous testing we noticed a small memory leak, we are looking into typed factory issues here: https://github.com/castleproject/Windsor/issues/410


If you want this note seen, probably best to format it like existing warnings.

:warning: **Facilities must have public default constructor:** When facilities are instantiated by Windsor, they must have public default constructor. Otherwise an exception will be thrown.

jonorossi · 2018-06-13T07:24:56Z

src/Castle.Facilities.AspNetCore/AspNetCoreFacility.cs

+	{
+		internal const string IsCrossWiredIntoServiceCollectionKey = "windsor-registration-is-also-registered-in-service-collection";
+
+		public static readonly AsyncLocal<AspNetCoreFacility> Current = new AsyncLocal<AspNetCoreFacility>();


I don't think it makes sense to add the AspNetCoreFacility if and only if you are doing cross-wiring, otherwise it throws if you aren't doing cross-wiring.

I still think this static stuff is completely unnecessary and that the ComponentRegistration should be as stateless as possible, and that the "cross wiring not configured" exception should be throw by CrossWiringComponentModelContributor.ProcessModel after the user calls Register on a container instance, at this point the CMC has access to the IKernel and easy access to AspNetCoreFacility.Services of the facility instance.

ghost · 2018-06-14T20:38:39Z

Welcome @xhafan! :)

~~This PR is now blocked by #411~~

generik0 · 2018-06-19T11:10:21Z

Hi Guys.
Looks like we need to test the new generic-host also:
generic-host

ghost · 2018-06-20T08:21:55Z

@generik0 are you sure? It looks to me like this is used in non-http hosting scenarios. They did this to decouple the http request pipeline. After reading this I imagine one would use this for long running tasks or ‘queue workers’.

ghost · 2018-06-20T08:30:30Z

If we can converge with the DI for both scenarios that would be good. Although cautiously I think we need to raise a new issue to track this.

Want to create one?

generik0 · 2018-06-20T12:49:08Z

@fir3pho3nixx you are correct. Its a new feature request, maybe a new facility.
Check: Autofac Generic Host Example
This example uses the AutofacServiceProviderFactory.

This may however lead to a new facility in castle that the aspnetcore also should use.?

ghost · 2018-06-20T20:48:47Z

@generik0 this sounds correct to me. Let me finish up this PR tonight and then we head over to the issue to flesh out the design a little more.

Even if we end up going the long way round it might end up informing us that we don’t even need cross wiring anymore in this facility ... hoping! ;)

ghost · 2018-06-21T00:38:53Z

src/Castle.Facilities.AspNetCore.Tests/WindsorRegistrationExtensionsTestCase.cs

+			testContext.ApplicationBuilder.UseMiddlewareFromWindsor<AnyMiddleware>(testContext.WindsorContainer);
+
+			Assert.DoesNotThrow(() => { testContext.WindsorContainer.Resolve<AnyMiddleware>(); });
+		}


Found more sucky API's.

@jonorossi Do you have any tips here?

Would be great of we could get rid of the IWindsorContainer parameter here. It is a bit late so I will have to pick this up ~~tomorrow~~ ~~later~~ after more discussion on #411 .

Not knowing the ASP.NET Core stuff, I guess you'd have to store the container reference and get access via IApplicationBuilder.

@jonorossi

Correct. I managed to take advantage of the facility to do this. Here is some sample code for new usage:

public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerFactory loggerFactory) { // For making component registrations of middleware easier Container.GetFacility<AspNetCoreFacility>().RegistersMiddlewareInto(app); // Add custom middleware, do this if your middleware uses DI from Windsor Container.Register(Component.For<CustomMiddleware>().DependsOn(Dependency.OnValue<ILoggerFactory>(loggerFactory)).LifestyleScoped().AsMiddleware()); // ... }

This now means we have first class support for Middleware registration(including lifestyles) using component registrations as opposed to the roll your own approach which coerced everything to singleton.

I will present this change as a separate commit once I heard back from @xhafan on issue #411 to make the review a little easier and squish once we are all happy with the proposed change.

Man, sorting out the memory management was an absolute bitch. These were pre-existing problems where CrossWired components were consuming each other (https://github.com/fir3pho3nixx/Windsor/pull/25).

@Inject

Provides integration with ASP.NET Core with first class support for Controllers, TagHelpers, ViewComponents and Middleware(needing Windsor). You can expose your Windsor registrations to third party libraries that use IServiceCollection by registering them with the `CrossWired` component registration extension. This will for example make things like @Inject work. Docs: - https://github.com/castleproject/Windsor/blob/master/docs/aspnetcore-facility.md Thanks to: - @dotnetjunkie - @generik0 - @hikalkan - @ploeh

ghost · 2018-07-02T23:45:49Z

LGTM, thanks to everyone that participated. Truly thank you! :)

stakx · 2018-07-03T05:02:15Z

Sorry for not reviewing @fir3pho3nixx, GitHub never notified my of that request, and while I take a look at this PR every now and then I missed this one... sorry!

ghost · 2018-07-04T07:38:05Z

Don’t worry bud! You are pretty busy with the core work. Not a problem ;)

This was referenced Apr 18, 2018

Reinstate Windsor danielpalme/IocPerformance#81

Closed

Can we retire this project in favour of Castle.Facilities.AspNetCore? volosoft/castle-windsor-ms-adapter#28

Open

Can't find Castle.Facilities.AspNetCore nuget package in nuget gallery #393

Closed

jonorossi requested changes Apr 20, 2018

View reviewed changes

jonorossi reviewed Apr 23, 2018

View reviewed changes

ghost mentioned this pull request May 14, 2018

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

Closed

ghost commented May 20, 2018

View reviewed changes

jonorossi reviewed Jun 6, 2018

View reviewed changes

jonorossi requested changes Jun 6, 2018

View reviewed changes

ghost commented Jun 7, 2018

View reviewed changes

ghost changed the title ~~ASP.NET Core Facility (now production ready)~~ ASP.NET Core Facility (now NOT production ready) Jun 10, 2018

ghost mentioned this pull request Jun 10, 2018

Typed factories need bug fixing #410

Open

jonorossi reviewed Jun 11, 2018

View reviewed changes

jonorossi approved these changes Jun 11, 2018

View reviewed changes

ghost changed the title ~~ASP.NET Core Facility (now NOT production ready)~~ ASP.NET Core Facility (now production ready BUT... ) Jun 12, 2018

ghost commented Jun 12, 2018

View reviewed changes

jonorossi requested changes Jun 13, 2018

View reviewed changes

ghost commented Jun 21, 2018

View reviewed changes

ghost mentioned this pull request Jun 21, 2018

UseMiddlewareFromWindsor cannot pass arguments into the middleware #411

Closed

ghost requested a review from stakx June 23, 2018 23:02

ghost removed the request for review from stakx July 1, 2018 20:11

ghost merged commit 588f786 into castleproject:master Jul 2, 2018

anythingapplied mentioned this pull request Jul 7, 2018

Logic to allow other valid recipe formats 0ptera/ReStack#3

Merged

ghost mentioned this pull request Jul 12, 2018

Netstandard 2.0 support NancyFx/Nancy.Bootstrappers.Windsor#54

Open

This pull request was closed.

		```

		After rigourous testing we noticed a small memory leak, we are into typed factories here: https://github.com/castleproject/Windsor/issues/410

ASP.NET Core Facility (now production ready BUT... ) #394

ASP.NET Core Facility (now production ready BUT... ) #394

Conversation

ghost commented Apr 15, 2018 • edited by ghost Loading

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

ghost Apr 20, 2018 • edited by ghost Loading

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 Apr 20, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 20, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Apr 20, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

ghost Apr 20, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 20, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 22, 2018 • edited by ghost Loading

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 Apr 22, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Apr 23, 2018 • edited by ghost Loading

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 Apr 22, 2018 • edited Loading

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 Apr 20, 2018

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

generik0 commented Apr 25, 2018

ghost commented Apr 25, 2018

generik0 commented Apr 29, 2018

ghost commented May 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 26, 2018

ghost commented Jun 1, 2018

generik0 commented Jun 1, 2018

ghost commented Jun 3, 2018

ghost commented Apr 15, 2018 •

edited by ghost

Loading

ghost Apr 20, 2018 •

edited by ghost

Loading

ghost Apr 20, 2018 •

edited by ghost

Loading

ghost Apr 20, 2018 •

edited by ghost

Loading

ghost Apr 20, 2018 •

edited by ghost

Loading

ghost Apr 20, 2018 •

edited by ghost

Loading

ghost Apr 20, 2018 •

edited by ghost

Loading

ghost Apr 22, 2018 •

edited by ghost

Loading

ghost Apr 22, 2018 •

edited by ghost

Loading

ghost Apr 23, 2018 •

edited by ghost

Loading

jonorossi Apr 22, 2018 •

edited

Loading

ghost commented Jun 4, 2018 •

edited by ghost

Loading

ghost Jun 7, 2018 •

edited by ghost

Loading

ghost Jun 10, 2018 •

edited by ghost

Loading

ghost Jun 10, 2018 •

edited by ghost

Loading