Added support for middleware activation via IMiddlewareFactory #773
Conversation
- IMiddlewareFactory and IMiddleware are new extensiblity points for activating and authoring middleware. Under the covers, middleware is still very much just a function. This just provides a nice way to get a per request activated middleware instance that is created and released via the IMiddlewareFactory. The caveats are that middleware needs to be registered in the container (by default) and that not possible to explicitly pass arguments directly via UseMiddleware. - Added tests
| // activated from the container | ||
| if (args.Length > 0) | ||
| { | ||
| throw new NotSupportedException(Resources.FormatException_UseMiddlewareExplicitArgumentsNotSupported(typeof(IMiddleware))); |
There was a problem hiding this comment.
So you've decide you're OK with throwing? #754 (comment)
There was a problem hiding this comment.
I put it in the PR for feedback. I considered adding another overload but it would be confusing naming.
| <value>No service for type '{0}' has been registered.</value> | ||
| </data> | ||
| <data name="Exception_UseMiddlewareUnableToCreateMiddleware" xml:space="preserve"> | ||
| <value>Unable to create middleware '{0}'.</value> |
There was a problem hiding this comment.
This could be a little more clear about it being the IMiddlewareFactory that's not providing an IMiddleware implementation of type {0}.
| throw new InvalidOperationException(Resources.FormatException_UseMiddlewareNoMiddlewareFactory(typeof(IMiddlewareFactory))); | ||
| } | ||
|
|
||
| var middleware = middlewareFactory.Create(middlewareType); |
There was a problem hiding this comment.
It's kind of interesting how the type winds up just being used as a key and nothing else.
| { | ||
| // The default middleware factory is just an IServiceProvider proxy. | ||
| // This should be registered as a scoped service so that the middleware instances | ||
| // don't end up being singletons. |
There was a problem hiding this comment.
Is there a companion PR in hosting where this is registered?
There was a problem hiding this comment.
Not yet, I'll do that after we agree we like this.
|
🆙 📅 |
|
What does the perf look like for middleware with the various lifetimes? E.g. singleton vs scoped. |
|
What exactly are you asking? What he cost of resolving middleware instances per request is? What will that data change? |
|
If there's a high overhead to resolve middleware per request then it will be hard to recommend this to anyone. |
|
Please read the PR description outlining the reason we're adding this in the first place (non conforming containers). On top of that, mvc controllers are created once per request so are many other objects along the request pipeline. The DI container is also extremely efficient at creating new objects so I'd be extremely suprised if this causes any real problems. People have to do this today if they anyway if they want to use a non conforming container (see the code sample in the PR description). |
|
Were you planning to convert any of our middleware? |
|
Nope. Although maybe we should use it somewhere so we can have something validating this code path. |
|
|
activating and authoring middleware. Under the covers, middleware is still
very much just a function. This just provides a nice way to get a per request
activated middleware instance that is created and released via the IMiddlewareFactory (this critical for non conforming containers, see https://github.com/dotnetjunkie/Missing-Core-DI-Extensions/blob/7efd3c3f1aa6330d303d0a3e2d28fdc43687b9f4/src/SampleApplication.SimpleInjector/Startup.cs#L52-L55 for more details).
The caveats are that middleware needs to be registered in the container (by default)
and that not possible to explicitly pass arguments directly via UseMiddleware.
/cc @Tratcher @halter73 @pakrym