Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The section about IExceptionHandler is misleading. #32311

Open
voroninp opened this issue Apr 12, 2024 · 2 comments
Open

The section about IExceptionHandler is misleading. #32311

voroninp opened this issue Apr 12, 2024 · 2 comments
Assignees
Labels

Comments

@voroninp
Copy link
Contributor

Description

The article creates an impression that it's enough to add custom implementation of IExceptionHandler. However, I had to call UseExceptionHanlder middleware.

This comment is a confirmation that the former does not work without the latter.

However, there are more surprises:

If I call:

app.UseExceptionHandler();

I get the following exception:

System.InvalidOperationException: 'An error occurred when configuring the exception handler middleware. Either the 'ExceptionHandlingPath' or the 'ExceptionHandler' property must be set in 'UseExceptionHandler()'. Alternatively, set one of the aforementioned properties in 'Startup.ConfigureServices' as follows: 'services.AddExceptionHandler(options => { ... });' or configure to generate a 'ProblemDetails' response in 'service.AddProblemDetails()'.'

If I call it like this:

app.UseExceptionHandler(c => {});

custom exception handler is called, but I see two log entries about the errordespite by handler returns true:

"Category": "Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware",

and then from my handler:

"Category": "ExceptionHandler"

I'd appreciate if this WTF behavior was properly explained.

the 'ExceptionHandler' property must be set in 'UseExceptionHandler()'.

Why do I need to call it, if I called builder.Services.AddExceptionHandler<ExceptionHandler>();

Here's the code I use:

using Microsoft.AspNetCore.Diagnostics;

var builder = WebApplication.CreateBuilder(args);
// Add services to the container.

builder.Services.AddExceptionHandler<ExceptionHandler>();
builder.Services.AddControllers();
builder.Logging.AddJsonConsole(c =>
{
    c.UseUtcTimestamp = true;
    c.IncludeScopes = true;
    c.JsonWriterOptions = new()
    {
        Indented = true,
    };
});

var app = builder.Build();

// Configure the HTTP request pipeline.

app.UseExceptionHandler(c => {});

app.UseHttpsRedirection();

app.UseAuthorization();

app.MapControllers();

app.Run();

internal sealed class ExceptionHandler : IExceptionHandler
{
    private readonly ILogger<ExceptionHandler> _logger;

    public ExceptionHandler(ILogger<ExceptionHandler> logger)
    {
        _logger = logger;
    }

    public async ValueTask<bool> TryHandleAsync(HttpContext httpContext, Exception exception, CancellationToken cancellationToken)
    {
        _logger.LogError(exception, "You will see it twice");

        return true;
    }
}

Page URL

https://learn.microsoft.com/en-us/aspnet/core/fundamentals/error-handling?view=aspnetcore-8.0

Content source URL

https://github.com/dotnet/AspNetCore.Docs/blob/main/aspnetcore/fundamentals/error-handling.md

Document ID

38515dfb-91a5-b395-db9d-084bbaf095c8

Article author

@tdykstra

@voroninp
Copy link
Contributor Author

I'd also mention that currently ExceptionHandlerMiddleware logs exception as unhandled despite IExceptionHandler returns true.

There was a request to disable logging

but dev team rejected the proposal.

But this is so wrong regarding the expectations of the developer.

Kestrel logs uncaught (!) exceptions with Error level. - That's fine.

However, ExceptionHandlerMiddleware logs exception anyway, but it does not pass it to Kestrel, if IExceptionHanlder says it handled the exception.

It's better to clarify in documentation, how to disable logging error from the middleware:

None for Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware

@voroninp
Copy link
Contributor Author

voroninp commented Apr 12, 2024

I'd propose @adityamandaleeka to reconsider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: To do
Development

No branches or pull requests

3 participants