ExceptionHandlerMiddleware does not clear IApiVersioningFeature

[mattvperry]

aspnetcore/src/Middleware/Diagnostics/src/ExceptionHandler/ExceptionHandlerMiddleware.cs

Lines 170 to 171 in d09b931

	var routeValuesFeature = context.Features.Get<IRouteValuesFeature>();
	routeValuesFeature?.RouteValues?.Clear();

If someone is trying to use both the ApiVersioning feature and the ExceptionHandler configuration that uses a path name, you will get an ambiguous routing exception on every error because the router uses the selected routes in the IApiVersioningFeature from the original request which contains the original route.

Here is the routing code from ApiVersioning:
https://github.com/microsoft/aspnet-api-versioning/blob/e16b579d240574053ebd8e7ee38c8686beaee174/src/Microsoft.AspNetCore.Mvc.Versioning/Versioning/ApiVersionActionSelector.cs#L165

It seems like the ApiVersioning code lives in a separate repo not under the dotnet umbrella which introduces a dependency problem. I wonder if its possible to introduce an extension point for this middleware that allows implementers to add additional functionality to the ClearHttpContext step of the exception middleware. This way, the ApiVersioning folks could add an implementation which clears their feature.

Bottom line, the exception middleware is already clearing the IRouteValuesFeature in order to accomplish its goal because it is assuming that this is the only code that was involved in selecting the original route. However, any libraries which introduce a custom layer of routing, like ApiVersioning, will fail to be cleared properly before re-entering the pipeline. Adding some kind of extension point such that routing libraries can expose the means for others to clear their state, could clear this issue up.

[davidfowl]

API versioning isn't part of ASP.NET Core and it can't be cleared from this middleware. This issue should be filed on that repository.

It seems like the ApiVersioning code lives in a separate repo not under the dotnet umbrella which introduces a dependency problem. I wonder if its possible to introduce an extension point for this middleware that allows implementers to add additional functionality to the ClearHttpContext step of the exception middleware. This way, the ApiVersioning folks could add an implementation which clears their feature.

Bottom line, the exception middleware is already clearing the IRouteValuesFeature in order to accomplish its goal because it is assuming that this is the only code that was involved in selecting the original route. However, any libraries which introduce a custom layer of routing, like ApiVersioning, will fail to be cleared properly before re-entering the pipeline. Adding some kind of extension point such that routing libraries can expose the means for others to clear their state, could clear this issue up.

That doesn't sound unreasonable, are you going to propose something concrete? We need an API proposal and a couple of examples to go with it.

[Tratcher]

Middleware are supposed to clear their own state on exit. The exception handling middleware clearing the route data was a performance compromise to avoid an extra state machine in the route middleware.

[mattvperry]

API versioning isn't part of ASP.NET Core and it can't be cleared from this middleware. This issue should be filed on that repository.

Yeah, I assumed that since ApiVersioning wasn't part of this monorepo that I might have to file something with those guys.

That doesn't sound unreasonable, are you going to propose something concrete? We need an API proposal and a couple of examples to go with it.

I have a few ideas, not sure if any of them are the right scope though.

Middleware are supposed to clear their own state on exit. The exception handling middleware clearing the route data was a performance compromise to avoid an extra state machine in the route middleware.

This makes sense, however, I don't know that this will necessarily work in this case. The only middleware added by ApiVersioning is done through a startup filter and I believe lives higher than the exception middleware. Therefore the exception middleware is going to intercept the exception and send the request back down the pipeline before ApiVersioning even knows what's happening. At least this is my understanding.

ApiVersioning works by overriding the IActionSelector abstraction from MVC. Part of what the new ActionSelector does is store the selected routes in an IApiVersioningFeature. Problem comes in when the ExceptionHandler sends the request down the pipe again, those first selected routes are still present on the feature.

Options I had considered to alleviate this:

Option 1: Add another delegate to ExceptionHandlerOptions, something like OnReentry (working title)

This would allow consumers of both ApiVersioning and ExceptionHandler to do:

// During startup
app.UseExceptionHandler(options => {
  options.ExceptionHandlingPath = "/error";
  options.OnReentry = context => context.Features.Get<IApiVersioningFeatures>().Clear()
});

// Within ExceptionHandlerMiddleware's ClearHttpContext
_options.OnReentry(context);

If we also get ApiVersioning onboard they might be able to implement OnReentry themselves with an IPostConfigure<ExceptionHandlerOptions>

Option2: Add a Clear method to IActionSelector

This one is certainly a bit more pervasive and may be overkill but if the intended path for library developers to take when modifying the stock MVC routing patterns is to override IActionSelector then it might make sense to expose a method on the selection abstraction which clears any state that could exist.

public ExceptionHandlerMiddleware(..., IActionSelector selector) 
{
  //...
}

public void ClearHttpContext(HttpContext context)
{
  //...

  _actionSelector.Clear();
}

Option 3: Add an entirely new abstraction that represents the clearing of router state.

This one is obviously super heavy and involves a lot of changes. It basically looks like Option 2 but we split up the abstractions of ActionSelection and RouteStateClearing

[Tratcher]

Part of what the new ActionSelector does is store the selected routes in an IApiVersioningFeature. Problem comes in when the ExceptionHandler sends the request down the pipe again, those first selected routes are still present on the feature.

That selection code runs after the exception handler? Could it reset the feature on entry instead of exit? Do they have a scenario for honoring pre-set data for some reason?

[mattvperry]

That selection code runs after the exception handler? Could it reset the feature on entry instead of exit? Do they have a scenario for honoring pre-set data for some reason?

It seems as though the selection code runs twice. Once on the initial request. And then again when the exception handler attempts to route to the ExceptionHandlingPath route.

I don't have an answer to your other questions. Let me start an issue on their repo as well, perhaps those folks could chime in on the feasibility of fixing this without intervention from this side. I had originally assumed that the best solution would be on this side but I could definitely be wrong.

dotnet/aspnet-api-versioning#748

Here's my main point:
ExceptionHandlerMiddleware works by clearing some state and re-triggering the middleware pipeline to route the request to the configured error handling path.
This clearing some state code makes certain assumptions about what state it has to clear in order to make the subsequent request fresh enough to re-route correctly to the configured error handling path.
I am not claiming that the assumptions made are incorrect, they clearly work perfectly when using the standard routing provided by MVC.
However, the problem comes when a library developer tries to add some new routing features to MVC. Which the official Microsoft ApiVersioning library does.

In my opinion, the questions that need to be answered are:
Is it the routing library's responsibility to make sure their extension functions when some middleware decides to rerun the entire pipeline a second time?
Or is "routing state clearing" an essential function of MVC's routing responsibilities that it needs to be individually extensible?
Or is this idea of re-running middleware so specific to this exception handling middleware that the extension point belongs entirely within this middleware?

[commonsensesoftware]

Answering here for thread affinity...

Question 1

Are you using the legacy routing method (e.g. IRouter + IActionSelector)? Endpoint Routing does not use these. We need to level set because if you're using Endpoint Routing many of the proceeding assumptions are incorrect.

Generally speaking, API Versioning does not change the behavior of routing. The whole premise is to use the same routing, but allow otherwise ambiguous matches to be resolved by way of an API version. If that still isn't possible, developers receive the same AmbiguousMatchException and 500 as you would without versioning.

The legacy routing method posed several challenges. The requirements from API Versioning helped push and vet the design of Endpoint Routing. In particular, it was possible for the IActionSelector to be entered multiple times. This most frequently happened with semantically equivalent URLs (ex: values/{id} vs values/{id:int}). This was a problem for API Versioning because it cannot make the correct decision until all possible route candidates have been observed. The routing system, however, only cared about continuing to try until it got a match or exhausted all attempts. This is should really be the only place where things deviate from the standard, expected routing behavior in the old system. This is a non-issue with Endpoint Routing.

API Versioning and IApiVersioningFeature do not require middleware. While it does exist, it might result in a no-op in the future. Today, the one and only thing that the middleware does is add the IApiVersioningFeature to the current HttpContext. This could just as easily been done imperatively, with an extension method, or so other method.

Question 2

What are you hoping/needing to clear? API Versioning makes no attempt to change or handle exceptional behavior. An API that encounters an error shouldn't have an alternate execution path. You might have a custom error response, which is fine and should work as expected. The only variant I've seen where this might be a problem is when mixing API and UI controllers in the same application; especially with SPAs. It makes complete sense for a UI to redirect to an error page, but not an API. If this is your scenario (and you're using legacy routing), I can elaborate more on the API Versioning repo with some possible solutions.

All of the required types are public. If you really must clear the IApiVersioningFeature, you can so like this:

var oldFeature = httpContext.Features.Get<IApiVersioningFeature>();
IApiVersioningFeature newFeature = new ApiVersioningFeature(httpContext);

newFeature.RawRequestedApiVersion = oldFeature.RawRequestedApiVersion;
newFeature.RequestedApiVersion = oldFeature.RawRequestedApiVersion;

// note: this is not the route value, it's the parameter name. this only applies if you're versioning by
// url segment. it's captured by the route constraint of a matching template. you can use whatever
// name you want, so api versioning needs to capture it so it can potentially look up the value later on
newFeature.RouteParameter = oldFeature.RouteParameter;

httpContext.Features.Set(newFeature);

NOTE: This will effectively clear the IApiVersioningFeature.SelectionResult. This might produce unintended side effects. If you're using Endpoint Routing, this will do nothing because it's not used.

[Tratcher]

Thanks @commonsensesoftware. The ExceptionhandlerMiddleware does something uncommon: it catches exceptions, changes the request path, and re-executes the request in-place to generate the error page. This will cause routing to run twice on the same request. The second time through it seems to be using cached ApiVersioning data.

Can you show us where IApiVersioningFeature is first generated and set on the HttpContext? Does that logic re-use an existing value if it's already there?

[mattvperry]

Question 1

Yes, this repro occurred with legacy routing. For unrelated reasons I had turned off endpoint routing. If the answer to all this is that endpoint routing is required for these two features to coexist then I will likely have to revisit that decision.

Question 2

Yes, the SelectionResult is where I was looking and you are completely correct, it is totally possible for someone to reset the ApiVersioningFeature from the outside since everything is public. The problem is that the consumer of these two things doesn't have a place to put that code.

@commonsensesoftware, I disagree with your statement that API controllers shouldn't have an alternate execution path for errors. I 100% agree that an API controller wouldn't use 301 redirects to error pages but that is not what this middleware does. Instead, at a high level, it uses a given route to find the corresponding controller action to execute on error. However, it does all of this within the same request. Now you may argue that one could/should use the alternative configuration for the ExceptionHandlerMiddleware which does not re-route and only uses a RequestDelegate but I find that there are various features of being within a controller action that assist with crafting a custom error which is why I prefer to use the path configuration

[commonsensesoftware]

In no particular order:

• The IApiVersioningFeature is added by the pipeline by the API Versioning middleware once and only once per request
  • The execution order usually doesn't matter for this feature as it's used much later in the pipeline
  • You can disable the automatic middleware configuration with options.RegisterMiddleware = false. You're then responsible for calling app.UseApiVersioning() at the place where you need it. Remember that it's still only initialized once
  • You can also imperatively add the feature on your own as needed
• Endpoint Routing will almost certainly make your life easier and is the preferred/recommend routing mechanism
• On error transfer to another controller seems unnecessarily complex. There are numerous ways to provide common or standardized error responses. If that's how you chose to make it work, so be it. It's your app. My opinion is irrelevant. 😉

If I fully understand the issue correctly, the problem is not that the middleware isn't working correctly, it's that the wrong action is selected (or not selected) because IApiVersioningFeature.SelectionResult contains previous matches that it should no longer be considered because the request has been transferred to a new route path.

The absolute simplest solution should be to just explicitly reset the IApiVersioningFeature yourself for every request. The implementation of ApiVersioningFeature has no initialization logic and is lazy-evaluated. The only cost you'd be paying is a second allocation, which will be negligible in this context.

This should do the trick:

app.UseExceptionHandler("/error");
app.Use((context, next) =>
    {
        // explicitly set/reset the api versioning feature at this point no matter what may have happened upstream
        // this is entered on all happy and unhappy paths
        context.Features.Set<IApiVersioningFeature>(new ApiVersioningFeature(context));
        return next(context);
    });
app.UseRouting();
app.UseMvc();

This would be unnecessary with Endpoint Routing since IApiVersioningFeature.SelectionResult is not considered. It does seem reasonable to clear previous selections without reallocating if the API Versioning middleware is entered again. I'll track that as an enhancement for anyone still using legacy routing.

[Tratcher]

It does seem reasonable to clear previous selections without reallocating if the API Versioning middleware is entered again.

Ah, that would be here:
https://github.com/microsoft/aspnet-api-versioning/blob/e16b579d240574053ebd8e7ee38c8686beaee174/src/Microsoft.AspNetCore.Mvc.Versioning/Versioning/ApiVersioningMiddleware.cs#L16-L22

[mattvperry]

@Tratcher I am not sure that clearing in that middleware would work since it gets injected by a StartupFilter:
https://github.com/microsoft/aspnet-api-versioning/blob/e16b579d240574053ebd8e7ee38c8686beaee174/src/Microsoft.AspNetCore.Mvc.Versioning/Microsoft.Extensions.DependencyInjection/AutoRegisterMiddleware.cs

According to my understanding, this will put the ApiVersioningMiddleware above the ExceptionHandlingMiddleware and therefore reentry would resume from after that middleware.

EDIT:
I missed the suggestion to turn off RegisterMiddleware. If the solution is to add clearing to the ApiVersioningMiddleware and then recommend that users doing legacy routing turn off the auto-registration and do UseApiVersioning() that's acceptable IMO.

[Tratcher]

Great, it sounds like this is now well understood and that any follow-up is in the versioning repo. Thanks everybody.