UseMiddlewareFromWindsor cannot pass arguments into the middleware

[xhafan]

This issue is related to https://github.com/fir3pho3nixx/Windsor/tree/aspnet-core-windsor-final branch

UseMiddlewareFromWindsor extension method (https://github.com/fir3pho3nixx/Windsor/blob/fa3617bf19e4f53430e018d543ababa00388e005/src/Castle.Facilities.AspNetCore/WindsorRegistrationExtensions.cs#L90) cannot pass arguments into the middleware.

I had to come up with a modified version with a new parameter argumentsAsAnonymousType:

public static void UseMiddlewareFromWindsor<T>(this IApplicationBuilder app, IWindsorContainer container, object argumentsAsAnonymousType)
	where T : class, IMiddleware
{
	container.Register(Component.For<T>());
	app.Use(async (context, next) =>
	{
		var resolve = container.Resolve<T>(argumentsAsAnonymousType);
		try
		{
			await resolve.InvokeAsync(context, async (ctx) => await next());
		}
		finally
		{
			container.Release(resolve);
		}
	});
}

Example of usage:

app.UseMiddlewareFromWindsor<TransactionScopeUnitOfWorkMiddleware>(_windsorContainer, new { isolationLevel = IsolationLevel.ReadCommitted });

In this example it passes read committed isolation level into the TransactionScopeUnitOfWorkMiddleware.

Should I create a pull request? Or do you think this can be added by you? Cheers.

[ghost]

Like it, except we should probably change the last parameter of UseMiddlewareFromWindsor from object argumentsAsAnonymousType to object argumentsAsAnonymousType = null.

Here is the revised pseudo method:

public static void UseMiddlewareFromWindsor<T>(this IApplicationBuilder app, IWindsorContainer container, object argumentsAsAnonymousType == null /*Already used in prod sorry, no breaking changes*/)
	where T : class, IMiddleware
{
	container.Register(Component.For<T>());
	app.Use(async (context, next) =>
	{
		T resolve = default(T);
		if (argumentsAsAnonymousType == null) { // <- We need to make sure we pick the right overload for the interface IWindsorContainer
			resolve = container.Resolve<T>();
		} else {
			resolve = container.Resolve<T>(argumentsAsAnonymousType);
		}
		try
		{
			await resolve.InvokeAsync(context, async (ctx) => await next());
		}
		finally
		{
			container.Release(resolve);
		}
	});
}

If you are happy with this, I will do it with tests, docs and performance tests.

[ghost]

/cc @generik0

[xhafan]

Like it a lot 👍 ta

[ghost]

I am a bit worried about this interface @xhafan, it overrides lifestyles from a singleton point of view.

The problem is UseMiddlewareFromWindsor is called from the Configure method in Startup.cs. This is single threaded. If you override dependencies here, you are inferring a singleton pattern. How does this work for scoped or transient lifestyles?

If your answer is to use typed factories it is important to note there is a slight memory leak which has not been completely verified/fixed yet. #410

For Example:

• IMiddleware which consumes a scoped dependency might throw an exception
• IMiddleware which consumes transient dependency might actually get a singleton instead. This is a captive dependency. Please see: https://github.com/fir3pho3nixx/Windsor/blob/aspnet-core-windsor-final/docs/aspnetcore-facility.md#captive-dependencies

Think we need to look at these things in detail before we expose another public API for resolution on middleware from the IWindsorContainer. Thoughts?

[xhafan]

If you override dependencies here, you are inferring a singleton pattern. How does this work for scoped or transient lifestyles?

@fir3pho3nixx, the original UseMiddlewareFromWindsor method registers the middleware as a singleton. So why inferring a singleton pattern from overriding dependencies is an issue?

Btw I can see another issue - the middleware is registered a singleton, and passing an argument into Resolve method makes an impression that the middleware is not a singleton.
How about adding object dependenciesAsAnonymousType = null as a parameter:

public static void UseMiddlewareFromWindsor<T>(this IApplicationBuilder app, IWindsorContainer container, object dependenciesAsAnonymousType = null)
	where T : class, IMiddleware
{
	var componentRegistration = Component.For<T>();
	if (dependenciesAsAnonymousType != null)
	{
		componentRegistration.DependsOn(dependenciesAsAnonymousType);
	}
	container.Register(componentRegistration);
	app.Use(async (context, next) =>
	{
		var resolve = container.Resolve<T>();
		try
		{
			await resolve.InvokeAsync(context, async (ctx) => await next());
		}
		finally
		{
			container.Release(resolve);
		}
	});
}

The example of the usage is the same:

app.UseMiddlewareFromWindsor<TransactionScopeUnitOfWorkMiddleware>(_windsorContainer, new { isolationLevel = IsolationLevel.Serializable });

IMiddleware which consumes a scoped dependency might throw an exception
IMiddleware which consumes transient dependency might actually get a singleton instead. This is a captive dependency

I just need to pass data - not resolvable dependencies - into the middleware. I thought that by using dependenciesAsAnonymousType parameter (or previously argumentsAsAnonymousType) you pass data (string, int, struct, etc) into the component constructor, not resolvable dependencies. If you would like to pass resolvable dependencies into a middleware, you would just add those dependencies as a middleware's constructor parameters? And those would need to be singletons anyway.

[xhafan]

I'm a complete moron - I actually don't need to use UseMiddlewareFromWindsor as I don't have any resolvable dependencies as middleware's constructor parameters. I can just use standard UseMiddleware method and pass the parameters this way. Sorry for wasting your time.

[ghost]

@fir3pho3nixx, the original UseMiddlewareFromWindsor method registers the middleware as a singleton. So why inferring a singleton pattern from overriding dependencies is an issue?

After you raised this issue, and it got me round to thinking this could be improved. Sorry if you took this as me rejecting your idea. If you check the PR I flagged this public API as rubbish in the PR here anyway. I do think we should try and remove the container as a parameter. It also does not feel like we have an implementation which expresses lifestyle concerns properly.

Btw I can see another issue - the middleware is registered a singleton, and passing an argument into Resolve method makes an impression that the middleware is not a singleton.

Correct! We need to figure this out a little bit more, this is what I originally meant. We already addressed this issue with cross wiring using a CMC via facility. Will investigate how we achieve this for middleware in the meantime.

I just need to pass data - not resolvable dependencies - into the middleware

Yep, I am just thinking of the consequences of clashing lifestyles for exposing IWindsorContainer.Resolve(object) via middleware. Glad you raised this issue :)

Sorry for wasting your time.

Don't think you did anything of the sort :)

[ghost]

Can we re-open this please?

[xhafan]

Re-opened, I'm glad it at least triggered something useful.

[ghost]

Re-opened, I'm glad it at least triggered something useful.

Great. :)

Please tell me about your usage? Interested to know some details.

You did mention IsolationLevel.ReadCommitted in the description and alluded to a Unit of work pattern here.

If I had to guess, you are trying to implement ACID(using read committed transactions down to your Sql Db) for each request using middleware right?

[ghost]

@xhafan

I think the final results are in for this PR

The problems we are trying to address here from what I can see are:

• Middleware does not have first class support when it comes to lifestyle concerns(always singleton).
• Middleware seems to go off in it's own direction and forsakes the ComponentRegistration API.
• Middleware does not provide any facility for overriding Ctor params.
• Middleware has never been tested for Scoped/Transients with Disposables.

Consider the enhanced code for middleware:

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());
	
	// ... 
}

Once I found a way of glueing the IApplicationBuilder to the facility, this then allowed me to hoist middleware registration up to component registrations. This unlocked all the good stuff one would expect to have when registering middleware with Windsor.

• Middleware now supports lifestyles (Singleton, Transient, Scoped with disposal concerns).
• Middleware now has a public API for supplying dependencies via the DependsOn extension(which is actually a ServiceOverride underneath).

This does mean one massive breaking change which I was rather hoping to avoid but I still think this has left us in a much better place.

I don't think I will be supporting the IWindsorContainer.Resolve(object) overload but I would be interested to hear how you get on with the changes I am suggesting(which give us this capability and even more).

If you are happy with these changes I will merge and close out this issue.

Thanks for participating as an early adopter! :)

[ghost]

Closed via https://github.com/fir3pho3nixx/Windsor/pull/25

[xhafan]

Please tell me about your usage? Interested to know some details.

You did mention IsolationLevel.ReadCommitted in the description and alluded to a Unit of work pattern here.

If I had to guess, you are trying to implement ACID(using read committed transactions down to your Sql Db) for each request using middleware right?

Yes, I have a transaction scope unit of work middleware, which wraps ASP.NET Core request into a transaction scope. As I mentioned previously, I don't have any dependencies, so I can just use standard app.UseMiddleware<TransactionScopeUnitOfWorkMiddleware>() and don't have to use any Windsor extension methods.

Consider the enhanced code for middleware:

public void Configure(IApplicationBuilder app, IHostingEnvironment env, ILoggerFactory loggerFactory)
{
// For making component registrations of middleware easier
Container.GetFacility().RegistersMiddlewareInto(app);

// Add custom middleware, do this if your middleware uses DI from Windsor
Container.Register(Component.For().DependsOn(Dependency.OnValue(loggerFactory)).LifestyleScoped().AsMiddleware());

// ...
}

I tried to use this new way of registering a middleware using AsMiddleware() and followed this tutorial, but the latest version (tried both branches aspnet-core-windsor-final and aspnet-core-windsor-final-middleware) does not work for me - I get an exception Scope was not available. Did you forget to call container.BeginScope().

Middleware now supports lifestyles (Singleton, Transient, Scoped with disposal concerns).

I'm a little bit lost here. My impression was that a middleware instance is a singleton, so I'm not sure allowing a middleware to be transient or scoped would be useful. I wanted to try it with my TransactionScopeUnitOfWorkMiddleware, say give it a transient lifestyle, but was out of luck trying it because of the aforementioned error.

[generik0]

Hi.
I normally use a factory for dB work (that uses castle typefactory). Both sessions and uow. This means that the parent class can be Singleton but the uow unique for each call.
Have a look her (I hope you find it relevant, I have used similar implementation for EF6 and. EF7)

https://github.com/generik0/Smooth.IoC.Dapper.Repository.UnitOfWork

[xhafan]

@fir3pho3nixx, I wanted to try previous commits to discover what is the last commit which works for me for aspnet-core-windsor-final branch. But I'm not able to find the history of commits I can try. For instance, looking at the branch commits, the commit from Apr 26 does not contain the extension method AddWindsor (services.AddWindsor(_windsorContainer), and the next commit from June 26 does not work for me, and I cannot even find the commit which works for me from June 14. Could you please point me to a right direction? Ta.

[xhafan]

I normally use a factory for dB work (that uses castle typefactory). Both sessions and uow. This means that the parent class can be Singleton but the uow unique for each call.
Have a look her (I hope you find it relevant, I have used similar implementation for EF6 and. EF7)

https://github.com/generik0/Smooth.IoC.Dapper.Repository.UnitOfWork

@generik0, are you saying there is a benefit resolving unit of work via a factory instead of resolving it from a container?

[generik0]

@xhafan I am saying that a factory is resolvable from the container.
And a factory can create uow using the container, without exposing the container.

So you ctor injects a dbfactory
this can create a session/context and transactions for you.
This is one of the awsom things about Castle. The type factory reimplementation:

https://github.com/castleproject/Windsor/blob/master/docs/typed-factory-facility-interface-based.md

Or look at https://github.com/generik0/Smooth.IoC.Dapper.Repository.UnitOfWork

[xhafan]

I tried to use this new way of registering a middleware using AsMiddleware() and followed this tutorial, but the latest version (tried both branches aspnet-core-windsor-final and aspnet-core-windsor-final-middleware) does not work for me - I get an exception Scope was not available. Did you forget to call container.BeginScope().

I found out why it gives me an error Scope was not available. Did you forget to call container.BeginScope(). . I blindly copy&pasted LifestyleScoped().AsMiddleware() from the tutorial. I now correctly changed that to .LifestyleSingleton().AsMiddleware(), and now I'm getting a different error: System.InvalidOperationException: 'Unable to resolve service for type 'some service' while attempting to activate 'some controller'.' :

It looks like the ASP.NET Core build in DI container is not able to resolve a controller's dependencies.
@fir3pho3nixx, could you please quickly review my Startup.cs if it follows the tutorial correctly? Ta.

[xhafan]

@xhafan I am saying that a factory is resolvable from the container.
And a factory can create uow using the container, without exposing the container.

So you ctor injects a dbfactory
this can create a session/context and transactions for you.
This is one of the awsom things about Castle. The type factory reimplementation:

https://github.com/castleproject/Windsor/blob/master/docs/typed-factory-facility-interface-based.md

Or look at https://github.com/generik0/Smooth.IoC.Dapper.Repository.UnitOfWork

@generik0, ok I think I get it. So the point is to inject the unit of work factory into my middleware, and use it to create unit of work instances, instead of using the container instance directly. Right?

[generik0]

@xhafan check :-)

[xhafan]

@generik0 thanks for the advice

[ghost]

@xhafan

I'm a little bit lost here. My impression was that a middleware instance is a singleton, so I'm not sure allowing a middleware to be transient or scoped would be useful.

This is simply an extension of middleware that wraps resolve/release inside other middleware. One *could* technically could create scoped middleware this way. It supports newing up your outer transaction scope in your ctor, and disposing it from an IDisposable implementation. You could equally do it as vanilla middleware without using a class which would make my example slightly contrived but there is something to this. I will explain at the end.

I tried to use this new way of registering a middleware using AsMiddleware() and followed this tutorial, but the latest version (tried both branches aspnet-core-windsor-final and aspnet-core-windsor-final-middleware) does not work for me - I get an exception Scope was not available. Did you forget to call container.BeginScope().

Yes, scopes were fun. I found massive discrepancies between middleware being registered and called somehow the scope was not there. I tried to combat this by ensuring the scope was provisioned in the middleware first here. Looks like we have a bug, nothing should be resolving without a scope first. I will check this again and get back to you.

I'm a little bit lost here. My impression was that a middleware instance is a singleton, so I'm not sure allowing a middleware to be transient or scoped would be useful. I wanted to try it with my TransactionScopeUnitOfWorkMiddleware, say give it a transient lifestyle, but was out of luck trying it because of the aforementioned error.

Confirmed will fix. Was simply hoping for middleware that would simply new up an outer TransactionScope in the ctor and then get rid of it by implementing IDisposable using a scoped lifestyle. Man, did
a can of worms explode in my face :)

• Windsor components that are CrossWired and get resolved as a dependency from another CrossWired component do not dispose properly and end up leaking memory if they are consumed in Windsor middleware.

This is probably why I have not merged the PR yet. We use a 2 container solution in this facility. What we attempted here was to let ASP.NET do it's thing(which mean's let it have it's own native container, ServiceProvider) and then we adapt the IWindsorContainer to it by using the ASP.NET Core activators for controllers, taghelpers and viewcomponents and by allowing Cross Wiring(double registrations in both Windsor and the IServiceCollection which all are guaranteed to resolve from Windsor). The problem with trying to do this is it has created a tug of war when it comes to managing and releasing resources(middleware blew the top off of this during perf testing). ServiceProvider always wants to track and release and it does it greedily. The only way we can bring this back to Windsor is by implementing our own providers/factories and having another crack at making these tests pass. Won't happen for V1 but definitely might need to for V2 when we start to look at hosting support.

It looks like the ASP.NET Core build in DI container is not able to resolve a controller's dependencies.
@fir3pho3nixx, could you please quickly review my Startup.cs if it follows the tutorial correctly? Ta.

I cannot see your QueryExecutorInstaller. Is CoreDdd.Queries.IQueryExecutor CrossWired?

Please implement CrossWiring here. We should support based on descriptors for component registration.

container.Register(
	Classes
		.FromAssemblyContaining<GetEmailQueryHandler>()
		.BasedOn(typeof(IQueryHandler<>))
		.WithService.FirstInterface()
		.Configure(x => x.LifestyleTransient().CrossWired()));

[ghost]

@xhafan Just to be clear. I really think you should hold off on upgrading until I have fixed this. We should at least remove the CrossWiring dependency from your IQueryHandler registration. Give me some time to look into this.

[ghost]

@xhafan Can you please take a look at the latest cut? You also should not need to cross wire anything.

I am going to drop this into production tomorrow. If I find anything I will let you know.

[xhafan]

@fir3pho3nixx, the latest cut works for me, thanks.

[ghost]

Awesome. We did not notice any memory spikes either. Closing for now. Please raise new issues when you find them.