Implicit call to UseRouting() in WebApplicationBuilder creates ordering issue with pipeline rerunning middleware, e.g. UseExceptionHandler #34146

DamianEdwards · 2021-07-06T23:23:08Z

API propsal:

Background and Motivation

To support middlewares that modify routes in Minimal APIs where UseRouting is called implicitly, we want to allow UseRouting to be called more than once in the app pipeline. E.g.

// visible configuration
app.UseExceptionHandler("/error")
app.MapGet("/error", () => "oh no!");

// functional configuration
app.UseRouting(); // Implicitly added by Minimal API
app.UseExceptionHandler("/error");
app.UseRouting(); // Added by UseExceptionHandler for the error branch only
app.MapGet("/error", () => "oh no!");

To support this, we need to make a change to UseRouting() since it overrides the EndpointRouteBuilder in the Properties bag of the builder: https://github.com/dotnet/aspnetcore/blob/main/src/Http/Routing/src/Builder/EndpointRoutingApplicationBuilderExtensions.cs#L48. Given this would be a breaking behaviour change, we want to add an overload that would use existing EndpointRouteBuilder if one can be found.

Proposed API

namespace Microsoft.AspNetCore.Builder
{
    public static class EndpointRoutingApplicationBuilderExtensions
    {
+       public static IApplicationBuilder UseRouting(this IApplicationBuilder builder, bool overrideEndpointRouteBuilder);
    }

Usage Examples

The usage pattern we expect is in middleware extensions:

public static IApplicationBuilder UseExceptionHandler(this IApplicationBuilder app, ExceptionHandlerOptions options)
{
...
    return app.Use(next =>
    {
...
        var errorBuilder = app.New();
        errorBuilder.UseRouting(overrideEndpointRouteBuilder: false);
        errorBuilder.Run(next);
        options.ExceptionHandler = errorBuilder.Build();
...
    });
}

Alternative Designs

#30549

Risks

Is this the best way to allow recalculation of the endpoint?

Original issue:

WebApplicationBuilder implicitly calls UseRouting() early on in the pipeline which causes issues for middleware added by the application that re-runs the pipeline after changing the path, including UseExceptionHandler. We likely need to revisit this behavior and potentially decide whether we also implicitly add middleware like UseDeveloperExceptionPage and UseExceptionHandler from WebApplicationBuilder to resolve this kinds of ordering issues.

Consider the following app:

using System;
using Microsoft.AspNetCore.Builder;
using Microsoft.Extensions.Hosting;
using Microsoft.AspNetCore.Http;

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();

app.UseExceptionHandler("/error");

app.MapGet("/error", () => "oh no!");
app.MapGet("/", () => "Hello World!");
app.MapGet("/throw", IResult () =>
{
    throw new Exception("bad bad");
});

app.Run();

Requests for "/" and "/error" directly from the browser work fine, but attempting to request "/throw" results in the following exception and a 500 result being returned:

fail: Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware[1]
      An unhandled exception has occurred while executing the request.
      System.Exception: bad bad
         at <Program>$.<>c.<<Main>$>b__0_2() in C:\src\local\EmptyWebExceptionHandling\Program.cs:line 20
         at lambda_method3(Closure , Object , HttpContext )
         at Microsoft.AspNetCore.Http.RequestDelegateFactory.<>c__DisplayClass28_0.<Create>b__0(HttpContext httpContext) in Microsoft.AspNetCore.Http.Extensions.dll:token 0x600013c+0x0
         at Microsoft.AspNetCore.Routing.EndpointMiddleware.Invoke(HttpContext httpContext) in Microsoft.AspNetCore.Routing.dll:token 0x60000a8+0x7b
      --- End of stack trace from previous location ---
         at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.<Invoke>g__Awaited|6_0(ExceptionHandlerMiddleware middleware, HttpContext context, Task task) in Microsoft.AspNetCore.Diagnostics.dll:token 0x60000bf+0x70
fail: Microsoft.AspNetCore.Server.Kestrel[13]
      Connection id "0HMA0QQKH5H54", Request id "0HMA0QQKH5H54:00000003": An unhandled exception was thrown by the application.
      System.InvalidOperationException: The exception handler configured on ExceptionHandlerOptions produced a 404 status response. This InvalidOperationException containing the original exception was thrown since this is often due to a misconfigured ExceptionHandlingPath. If the exception handler is expected to return 404 status responses then set AllowStatusCode404Response to true.
       ---> System.Exception: bad bad
         at <Program>$.<>c.<<Main>$>b__0_2() in C:\src\local\EmptyWebExceptionHandling\Program.cs:line 20
         at lambda_method3(Closure , Object , HttpContext )
         at Microsoft.AspNetCore.Http.RequestDelegateFactory.<>c__DisplayClass28_0.<Create>b__0(HttpContext httpContext) in Microsoft.AspNetCore.Http.Extensions.dll:token 0x600013c+0x0
         at Microsoft.AspNetCore.Routing.EndpointMiddleware.Invoke(HttpContext httpContext) in Microsoft.AspNetCore.Routing.dll:token 0x60000a8+0x7b
      --- End of stack trace from previous location ---
         at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.<Invoke>g__Awaited|6_0(ExceptionHandlerMiddleware middleware, HttpContext context, Task task) in Microsoft.AspNetCore.Diagnostics.dll:token 0x60000bf+0x70
         --- End of inner exception stack trace ---
         at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.HandleException(HttpContext context, ExceptionDispatchInfo edi) in Microsoft.AspNetCore.Diagnostics.dll:token 0x60000bc+0x268
         at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.<Invoke>g__Awaited|6_0(ExceptionHandlerMiddleware middleware, HttpContext context, Task task) in Microsoft.AspNetCore.Diagnostics.dll:token 0x60000bf+0xf2
         at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests[TContext](IHttpApplication`1 application) in Microsoft.AspNetCore.Server.Kestrel.Core.dll:token 0x6000aa6+0x1b8

Proposal:

We add UseRouting() in the error branch. This requires a new overload to UseRouting since the current one replaces the IEndpointRouteBuilder here:

Current proposal: https://github.com/dotnet/aspnetcore/compare/johluo/diagnostic-option3?expand=1

@davidfowl

The text was updated successfully, but these errors were encountered:

ghost · 2021-07-06T23:23:11Z

Thanks for contacting us.

We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

JunTaoLuo · 2021-07-15T02:12:33Z

Had a chat with @halter73 and I think we'll try to add an analyzer that will warn when an incorrect configuration is detected (i.e. UseExceptionHandler configured with a path called after UseRouting or called with no explicit UseRouting).

We also discussed two other options:

Making this work by default. This requires us to re-order the middleware, i.e. call UseExceptionHandler before the implicit call to UseRouting. However there are a few issues: 1) this is unintuitive since it might move the middleware before other middlewares (i.e. configured as MiddlewareA -> ExceptionHandlerMiddleware in code but resolves to ExceptionHandlerMiddleware ->Routing -> MiddlewareA as the request delegate) 2. This error wouldn't occur if the ExceptionHandler is configured with an explicit delegate instead of a path.
Add a warning in the middleware so that it warns of potential misuse if an endpoint is detected (i.e. Routing already ran). This isn't mutually exclusive with the analyzer but given that the analyzer can find the issue at build time instead of first request, the analyzer seems more appealing to me.

ghost · 2021-08-04T17:22:20Z