IExceptionHandler for exception handler middleware

[Tratcher]

Background and Motivation

The exception handling middleware is designed to take unexpected, unhandled exceptions, log them, and render a customizable response for the client. This is a last ditch effort to produce something human readable, otherwise the response will receive an empty 500. This is not intended for known cases or control flow. Expected exceptions should be handled at the individual call sites.

Ask: Some exceptions like database exceptions could happen in many places and having to handle those in each place requires a lot of redundant code. How can handling of known exceptions be centralized?

Proposed API

The IExceptionHandler is a new interface that can have multiple instances registered with DI. The existing exception handling middleware will resolve these and loop through them until something matches. If none match, a higher level log (warning?) will be written. Exception handlers are not run if the response has already started. The existing IDeveloperPageExceptionFilter is used similarly with the developer exception page.

Assembly: Microsoft.AspNetCore.Diagnostics.Abstractions

Microsoft.Extensions.DependencyInjection;

public static class ExceptionHandlerServiceCollectionExtensions
{
+  public static IServiceCollection AddExceptionHandler<T>(this IServiceCollection services) where T is IExceptionHandler;
}

namespace Microsoft.AspNetCore.Diagnostics;

+ public interface IExceptionHandler
+ {
+    ValueTask<bool> TryHandleAsync(HttpContext httpContext, Exception exception, CancellationToken cancellationToken);
+ }

Usage Examples

services.AddExceptionHandler<DBExceptionHandler>();

public class DBExceptionHandler : IExceptionHandler
{
  ValueTask<bool> TryHandleAsync(HttpContext httpContext, Exception exception, CancellationToken cancellationToken)
  {
    if (exception is DBException) ...
  };
}

Risks

We don't want to encourage developers to use exceptions as normal control flow, it's extremely in-efficient and poor design. We should document that expectation and avoid using this capability in our own components.

This should not be confused with the MVC IExceptionFilter.
https://learn.microsoft.com/en-us/dotnet/api/system.web.mvc.iexceptionfilter

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[Kahbazi]

@Tratcher Would you accept a PR for this one when the API got approved?

[Tratcher]

Likely. Thanks for offering.

[Kahbazi]

Do we need to pass CancellationToken to IExceptionHandler.TryHandleAsync. I assume it's the same as HttpContext.RequestAborted.

[halter73]

API Review Notes:

• Why not copy IDeveloperPageExceptionFilter's more middleware-like approach that has greater flexibility? It's not clear why any filter would call next() other than to skip itself.
• Why not create an ErrorContext? We don't think we'll ever need to pass anything other than HttpContext and Exception.
• Returning new ValueTask<bool>(false) means unhandled and try the next filter.
• Do we need a CancellationToken or are we fine relying solely on HttpContext.RequestAborted. The token probably always will be RequestAborted but we like the flexibility.
• What do we think about the naming? Handler vs Filter? Without nesting, let's call it a filter.

API Approved as proposed!

Assembly: Microsoft.AspNetCore.Diagnostics.Abstractions

Microsoft.Extensions.DependencyInjection;

public static class ExceptionHandlerServiceCollectionExtensions
{
    // Adds a singleton IExceptionHandler service. You can pull request services from HttpContext.
+    public static IServiceCollection AddExceptionHandler<T>(this IServiceCollection services) where T is IExceptionHandler;
}

namespace Microsoft.AspNetCore.Diagnostics;

+ public interface IExceptionHandler
+ {
+    ValueTask<bool> TryHandleAsync(HttpContext httpContext, Exception exception, CancellationToken cancellationToken);
+ }

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 8 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.

[jez9999]

Background and Motivation

The exception handling middleware is designed to take unexpected, unhandled exceptions, log them, and render a customizable response for the client. This is a last ditch effort to produce something human readable, otherwise the response will receive an empty 500. This is not intended for known cases or control flow. Expected exceptions should be handled at the individual call sites.

I'd like a more generic mechanism that lets me completely replace the default exception handler. I have my own thing (registered with app.UseExceptionHandler) which outputs exceptions in a more detailed, more nicely formatted way. I don't wanna have to configure logs to hide the default exception handler's one, it should be doable from the pipeline. Why not allow the registration of an IOptions<ExceptionHandlerConfig> or something, with a OutputLogs bool? Or will this API allow AddExceptionHandler<Exception> to always replace the default handler?

We don't want to encourage developers to use exceptions as normal control flow, it's extremely in-efficient and poor design. We should document that expectation and avoid using this capability in our own components.

I'd like a bit more clarification on exactly what you mean by "normal control flow". I've read long debates over this and if anything, what won out in terms of official advice from Microsoft was to use exceptions relatively liberally; for an unexpected network disconnection or something, yes, but also for broken user input. I throw an exception if an invalid value is passed into my REST API for example. Is this wrong? Why? It's a one-off thing and bad input isn't generally expected. It works very nicely to curtail any further execution, which is impossible if the input is broken and cannot be used.

[Tratcher]

I'd like a more generic mechanism that lets me completely replace the default exception handler. I have my own thing (registered with app.UseExceptionHandler) which outputs exceptions in a more detailed, more nicely formatted way. I don't wanna have to configure logs to hide the default exception handler's one, it should be doable from the pipeline. Why not allow the registration of an IOptions<ExceptionHandlerConfig> or something, with a OutputLogs bool? Or will this API allow AddExceptionHandler<Exception> to always replace the default handler?

How is that different from using your own middleware?

We've avoided making log levels configurable anywhere in the platform because it doesn't scale up or down. At one extreme you'd need an option for every log statement. At the other you'd need an option for every component which isn't significantly different from configuring the levels via appsettings.json.

Log filters are another way of tuning your log outputs.

I'd like a bit more clarification on exactly what you mean by "normal control flow". I've read long debates over this and if anything, what won out in terms of official advice from Microsoft was to use exceptions relatively liberally; for an unexpected network disconnection or something, yes, but also for broken user input. I throw an exception if an invalid value is passed into my REST API for example. Is this wrong? Why? It's a one-off thing and bad input isn't generally expected. It works very nicely to curtail any further execution, which is impossible if the input is broken and cannot be used.

The guidance is nearly the opposite, avoid exceptions for any expected scenario. E.g. when dealing with user input use APIs like TryParse and provide the user with a direct comment about what input is expected. Types like Socket and Stream have been around forever and don't provide better alternatives, but even there we try to handle those exceptions as early as possible in the caller rather than flowing them through the stack. Why? Exceptions are very expensive, and they're also hard to reason about when thrown from a deeply nested component, you lose too much context to provide a reasonable mitigation. The ExceptionHandlerMiddleware is a fallback of last resort, it can't provide granular, meaningful handling of exceptions from anywhere in your app, only generic handling like "Oops, something went wrong, please try again later.".

[jez9999]

I'd like a more generic mechanism that lets me completely replace the default exception handler. I have my own thing (registered with app.UseExceptionHandler) which outputs exceptions in a more detailed, more nicely formatted way. I don't wanna have to configure logs to hide the default exception handler's one, it should be doable from the pipeline. Why not allow the registration of an IOptions<ExceptionHandlerConfig> or something, with a OutputLogs bool? Or will this API allow AddExceptionHandler<Exception> to always replace the default handler?

How is that different from using your own middleware?

I don't quite understand what you mean - use my own middleware in what way, for what? For handling exceptions? I do, but the default handler still spits out logs that have to be suppressed with an unintuative log level configuration that reads like "don't log exceptions". It feels hacky.

We've avoided making log levels configurable anywhere in the platform because it doesn't scale up or down. At one extreme you'd need an option for every log statement. At the other you'd need an option for every component which isn't significantly different from configuring the levels via appsettings.json.

It's not really configuring log levels, it's more "don't even register the default exception handler because here's my one that's gonna replace it".

The guidance is nearly the opposite, avoid exceptions for any expected scenario. E.g. when dealing with user input use APIs like TryParse and provide the user with a direct comment about what input is expected. Types like Socket and Stream have been around forever and don't provide better alternatives, but even there we try to handle those exceptions as early as possible in the caller rather than flowing them through the stack. Why? Exceptions are very expensive, and they're also hard to reason about when thrown from a deeply nested component, you lose too much context to provide a reasonable mitigation. The ExceptionHandlerMiddleware is a fallback of last resort, it can't provide granular, meaningful handling of exceptions from anywhere in your app, only generic handling like "Oops, something went wrong, please try again later.".

Again, I spent quite some time thinking about this, and reading up on it, and opinion is seriously divided. I didn't find any "clear guidance" anywhere on precisely how to use exceptions. But say I have some code where I'm creating a game from a REST API controller. It calls GameCreator.CreateGame, and that method decides that the game config passed through to it is invalid. My code has var gameData = await _gameCreator.CreateGame(...). I want my code to just take gameData and assume success. An exception on valid input is an extremely convenient way to get my app to return a 400 with an appropriate error message, by throwing a custom exception (I can even specify the HTTP status code in there). I have centralized code for constructing a standard JSON response when there's an error, and I'm not having to call something from lots of different places to do it. If I don't do that, I have to litter my code with if (gameData == null) or whatever "there was an error" return value mechanism one cares to use, then return the appropriate error JSON by calling something to construct it from many different places, which I ultimately decided made my code look much less readable and maintainable.

If the official guidance is not to use exceptions like this, I think it's seriously debatable. I think the exception mechanism allowing this control flow is way too useful to be "last resort" only and if they're so heavyweight, well, .NET should introduce some kind of lightweight exception.

The ExceptionHandlerMiddleware can't provide granular, meaningful handling of exceptions precisely because it's totally generic. I know what my exception types and their contents mean. That's why I want to replace the default handler and handle my exceptions myself.

[Tratcher]

It's not really configuring log levels, it's more "don't even register the default exception handler because here's my one that's gonna replace it".

It's only registered in the template. If you don't want it you can remove this line:

aspnetcore/src/ProjectTemplates/Web.ProjectTemplates/content/StarterWeb-CSharp/Program.cs

Line 125 in 6796d5b

app.UseExceptionHandler("/Home/Error");

Here's the guidance I was referring to:

https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/exception-throwing

❌ DO NOT use exceptions for the normal flow of control, if possible.

Except for system failures and operations with potential race conditions, framework designers should design APIs so users can write code that does not throw exceptions. For example, you can provide a way to check preconditions before calling a member so users can write code that does not throw exceptions.

The member used to check preconditions of another member is often referred to as a tester, and the member that actually does the work is called a doer.

There are cases when the Tester-Doer Pattern can have an unacceptable performance overhead. In such cases, the so-called Try-Parse Pattern should be considered (see Exceptions and Performance for more information).

✔️ CONSIDER the performance implications of throwing exceptions. Throw rates above 100 per second are likely to noticeably impact the performance of most applications.

[jez9999]

It's only registered in the template. If you don't want it you can remove this line:
app.UseExceptionHandler("/Home/Error");

Odd, I have a .NET 6 web project and it's getting registered anyway, and I don't have that line. Are you sure that's not just for using a specific error page? I mean, if you don't have that line, there's still some kind of default exception handling middleware isn't there?

Here's the guidance I was referring to:

https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/exception-throwing

x DO NOT use exceptions for the normal flow of control, if possible.

Yeah, like I said, code littered with if (result == null) { return generateError(); } or whatever and no centralized error handling. Or even more fun, with async code, you need tuple returns instead of out params so the code becomes var thingResult = doThing(); if (!thing.Success) { return generateError(); } Seriously debatable. And then, what is the actual error if it failed? You almost want to wrap it in an object with a message, a call stack, and optional other values that can be specified by extending the object. Not the only place I don't like MS's "design guidelines", frankly. That guidance seems rather confused in itself. "DO report execution failures by throwing exceptions", execution failures? Like not being able to continue because of invalid input? "DO NOT use exceptions for the normal flow of control, if possible." If possible. Meh, what does that mean, really? Maybe if something lends itself to a TryX pattern, but it often doesn't. "CONSIDER the performance implications of throwing exceptions" - but only consider. And I'd never heard of System.Environment.FailFast. Seems to terminate the whole process. Shame there isn't a FastException.

[Tratcher]

Odd, I have a .NET 6 web project and it's getting registered anyway, and I don't have that line. Are you sure that's not just for using a specific error page? I mean, if you don't have that line, there's still some kind of default exception handling middleware isn't there?

If you don't have that line then the only default is the server catching exceptions thrown out of the middleware pipeline, logging, and returning an empty 500. If you handle the exception in your own middleware and don't re-throw it, neither of these components will log it. What's the log look like?

[jez9999]

Here's an example:

fail: Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware[1]
      An unhandled exception has occurred while executing the request.
      Microsoft.Data.SqlClient.SqlException (0x80131904): Cannot insert [...]

My middleware is running via app.UseExceptionHandler. I don't know whether that's meant to stop the default exception handler from logging or not.

[Tratcher]

My middleware is running via app.UseExceptionHandler. I don't know whether that's meant to stop the default exception handler from logging or not.

Oh, you don't have a custom middleware the, you've just plugged into UseExceptionHandler's callback. The middleware will log before invoking your callback. Replacing the middleware is pretty simple, it's just a try/catch around the next middleware call.

[jez9999]

Don't quite understand you, I have:

app.UseExceptionHandler(errorApp => {
	errorApp.Run(async context => {
		[...]
	}
}

Where is the next call?

[Tratcher]

UseExceptionHandler needs to be replaced completely to get the control you want, but it doesn't take much:

app.Use(async (context, next) =>
{
    try
    {
        await next(context);
    }
    catch (Exception ex)
    {
        if (context.Response.HasStarted)
        {
            // Too late to change anything
            throw;
        }

        /* handle exception */
    }
});

[jez9999]

Also @Tratcher I just noticed your risks comment, "We don't want to encourage developers to use exceptions as normal control flow, it's extremely in-efficient and poor design."

In practice, I'm really not sure about this advice. I've read that you have to be throwing 100 exceptions per second to start having a problem. How often is that realistically going to happen, especially for an API that isn't public or heavily-used? And to lose the exception mechanism for a generic error handler in ASP.NET, avoiding the need for a bunch of defensive coding around everything, seems a shame. Please read my issue #47020 for further comments on this.

[davidfowl]

Exceptions are extremely expensive, even more expensive than usual when they are in the ASP.NET Core pipeline which is fully asynchronous and nested (we're looking at ways to make this cheaper but it's still very expensive). In fact, there are teams with high scale services that see performance problems with exceptions and are trying to avoid them happening. There's no way we'd do #47020.

[jez9999]

Is there no way to have a mechanism like exceptions that isn't so expensive? It's just way more convenient to be able to throw and handle the error at the right level (often my centralized exception handling middleware) than pass some error info back up the chain.

[davidfowl]

It isn't something we'd ever recommend; I think it's fine for your custom middleware to continue to do this.

[jez9999]

I find that really confusing guidance.

[davidfowl]

@Tratcher can you file some follow up issues around how we plan to use this with problem details.