dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.47k stars 10.03k forks source link

AddExceptionHandler/UseExceptionHandler Fails in a non-Obvious Way #51888

Open shawnwildermuth opened 1 year ago

shawnwildermuth commented 1 year ago

Is there an existing issue for this?

Describe the bug

With the new AddExceptionHandler and IExceptionHandler APIs, engaging the exception handler with UseExceptionHandler is confusing. For example, this code fails;

using ExHandler;
using Microsoft.AspNetCore.Diagnostics;

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddExceptionHandler<GlobalExceptionHandler>();

var app = builder.Build();

app.UseExceptionHandler();       // Doesn't work

app.MapGet("/", () => {
  throw new Exception("Bad things happen to good developers");
});

app.Run();

public class GlobalExceptionHandler : IExceptionHandler
{
  public async ValueTask<bool> TryHandleAsync(HttpContext httpContext,
    Exception exception,
    CancellationToken cancellationToken)
  {
    httpContext.Response.ContentType = "text/plain";
    httpContext.Response.StatusCode = 501;
    await httpContext.Response.WriteAsync($"It don't work: {exception.Message}");
    return true;
  }
}

To make this work, you must pass in an empty lambda to the `UseExceptionHandler':

app.UseExceptionHandler(o => { }); // Works

Expected Behavior

Expected the empty call to UseExceptionHandler to work. Could not find documentation that explained this. ,

Steps To Reproduce

Example is at: https://github.com/shawnwildermuth/ExceptionHandlerRepro

Exceptions (if any)

I get an arcane message:

System.InvalidOperationException
  HResult=0x80131509
  Message=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()'.
  Source=Microsoft.AspNetCore.Diagnostics
  StackTrace:
   at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddlewareImpl..ctor(RequestDelegate next, ILoggerFactory loggerFactory, IOptions`1 options, DiagnosticListener diagnosticListener, IEnumerable`1 exceptionHandlers, IMeterFactory meterFactory, IProblemDetailsService problemDetailsService)
   at Microsoft.AspNetCore.Builder.ExceptionHandlerExtensions.<>c__DisplayClass5_0.<SetExceptionHandlerMiddleware>b__0(RequestDelegate next)
   at Microsoft.AspNetCore.Builder.ApplicationBuilder.Build()
   at Microsoft.AspNetCore.Builder.ApplicationBuilder.Build()
   at Microsoft.AspNetCore.Hosting.GenericWebHostService.<StartAsync>d__40.MoveNext()
   at Microsoft.Extensions.Hosting.Internal.Host.<<StartAsync>b__15_1>d.MoveNext()
   at Microsoft.Extensions.Hosting.Internal.Host.<ForeachService>d__18`1.MoveNext()
   at Microsoft.Extensions.Hosting.Internal.Host.<StartAsync>d__15.MoveNext()
   at Microsoft.Extensions.Hosting.HostingAbstractionsHostExtensions.<RunAsync>d__4.MoveNext()
   at Microsoft.Extensions.Hosting.HostingAbstractionsHostExtensions.<RunAsync>d__4.MoveNext()
   at Microsoft.Extensions.Hosting.HostingAbstractionsHostExtensions.Run(IHost host)
   at Program.<Main>$(String[] args) in C:\projects\codingshorts\net8exhandler\test\ExHandler\Program.cs:line 17

.NET Version

8.0.100-rc.2.23502.2

Anything else?

Note the failure is uncommented out, and the one that works is commented out. If this is expected behavior, we need it to be well documented since it is unobvious behavior.

davidfowl commented 1 year ago

Agreed this should be fixed now that we have exception handlers that can be added via DI.

captainsafia commented 1 year ago

PR open against main.

This is a good candidate for servicing to 8.0 as well.

dotnet-policy-service[bot] commented 9 months ago

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

dotnet-policy-service[bot] commented 9 months ago

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

shawnwildermuth commented 9 months ago

This has been fixed.

dse-copsfs commented 7 months ago

The connected PR has been closed.

We still facing this issue in 8.0.3, so we have to use the proposed workaround app.UseExceptionHandler(o => {});

Could you please reopen this issue?

captainsafia commented 7 months ago

Yes, we never ended up addressing this because the ramifications of the breaking change weren't fully enumerated.

I'll reopen this issue but considering the current prioritizations and the fact that there is a workaround, this will likely go in the backlog.

I'm open to reviewing a PR with a fix though!

diegocam commented 7 months ago

Any updates here?

latuconsinafr commented 4 months ago

Still waiting for the update 👀

edproton commented 3 months ago

Still waiting for the update 👀

ghhv commented 2 months ago

fyi, I managed to get this issue on a brand new NET 8.0.8 Blazor Web Application project because I was calling this again by accident in a start-up helper function but without any parameters like so: app.UseExceptionHandler();

I have this prior in the program.cs app.UseExceptionHandler("/Error", createScopeForErrors: true);

The lack of parameters isn't an issue - it was the calling it twice!

Legion-ale commented 2 months ago

For what i understand I can agree with @ghhv saying that the problem is how we configure the webApp, and not in the method itself.

I was having the same problem, and I was thinking I'd solved it with: app.UseExceptionHandler(()=>{}); but, actually, we'd just bypassed the correct exception management.

The real problem seems that you need to register a "default" response in case your IExceptionHandler return false (Even if every exception is correctly managed inside the IExceptionHandler).

Actually adding 'AddProblemDetails()' before your exception handler seems doing the trick:

        builder.Services.AddProblemDetails();
        builder.Services.AddExceptionHandler<GlobalExceptionHandler>();
[...]
        var app = builder.Build();
        app.UseExceptionHandler();

but i'm not sure is the correct behaviour. I leave here microsoft link with the method explanation:

[...] code configures the app to generate a problem details response for all HTTP client and server error responses that don't have body content yet[...]

kjkrum commented 1 month ago

I'm also not seeing this issue when using an exception handler in combination with problem details. The order they're added to the service collection shouldn't matter.

builder.Services.AddExceptionHandler<ExceptionHandler>();
builder.Services.AddProblemDetails(options =>
{
    options.CustomizeProblemDetails = context =>
    {
        ...
    };
});

app.UseStatusCodePages();
app.UseExceptionHandler();

If (and only if) the IExceptionHandler returns false, the exception is available in the customize action as ProblemDetailsContext.Exception. This makes sense, since I think the exception handler returning true means the exception does not propagate.

But letting an exception propagate so it appears in ProblemDetailsContext.Exception (e.g., so you can use it to add information to the problem details) triggers another bug. ProblemDetailsContext.ProblemDetails is always initialized as a 500 regardless of the status code set by the exception handler, and there's no way to leverage the existing defaults to re-initialize it (#47978).