This is the first post in a series of posts based on code reviews of systems where ASP.NET Core is involved.
I recently came across code like the following:
public class FaultyMiddleware { public FaultyMiddleware(RequestDelegate next) { _next = next; } public async Task Invoke(HttpContext context) { // saving the context so we don't need to pass around as a parameter this._context = context; DoSomeWork(); await _next(context); } private void DoSomeWork() { // code that calls other private methods } // ... HttpContext _context; RequestDelegate _next; }
The problem here is a misunderstanding of how middleware components work in the ASP.NET Core pipeline. ASP.NET Core uses a single instance of a middleware component to process multiple requests, so it is best to think of the component as a singleton. Saving state into an instance field is going to create problems when there are concurrent requests working through the pipeline.
If there is so much work to do inside a component that you need multiple private methods, a possible solution is to delegate the work to another class and instantiate the class once per request. Something like the following:
public class RequestProcessor { private readonly HttpContext _context; public RequestProcessor(HttpContext context) { _context = context; } public void DoSomeWork() { // ... } }
Now the middleware component has the single responsibility of following the implicit middleware contract so it fits into the ASP.NET Core processing pipeline. Meanwhile, the RequestProcessor, once given a more suitable name, is a class the system can use anytime there is work to do with an HttpContext.