Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Buffers;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Text;
using System.Text.Encodings.Web;
using Microsoft.AspNetCore.Antiforgery;
Expand Down Expand Up @@ -33,6 +34,9 @@ public Task Render(HttpContext context)
return _renderer.Dispatcher.InvokeAsync(() => RenderComponentCore(context));
}

// We do not want the debugger to consider NavigationExceptions caught by this method as user unhandled.
[MethodImpl(MethodImplOptions.NoInlining)]
[DebuggerDisableUserUnhandledExceptions]
private async Task RenderComponentCore(HttpContext context)
{
context.Response.ContentType = RazorComponentResultExecutor.DefaultContentType;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using System.Text.Encodings.Web;
using Microsoft.AspNetCore.Components.Rendering;
using Microsoft.AspNetCore.Components.Web.HtmlRendering;
Expand Down Expand Up @@ -88,6 +90,9 @@ public ValueTask<IHtmlAsyncContent> PrerenderComponentAsync(
ParameterView parameters)
=> PrerenderComponentAsync(httpContext, componentType, prerenderMode, parameters, waitForQuiescence: true);

// We do not want the debugger to consider NavigationExceptions caught by this method as user unhandled.
[MethodImpl(MethodImplOptions.NoInlining)]
[DebuggerDisableUserUnhandledExceptions]
public async ValueTask<IHtmlAsyncContent> PrerenderComponentAsync(
HttpContext httpContext,
[DynamicallyAccessedMembers(Component)] Type componentType,
Expand Down Expand Up @@ -129,6 +134,9 @@ public async ValueTask<IHtmlAsyncContent> PrerenderComponentAsync(
}
}

// We do not want the debugger to consider NavigationExceptions caught by this method as user unhandled.
[MethodImpl(MethodImplOptions.NoInlining)]
[DebuggerDisableUserUnhandledExceptions]
internal async ValueTask<PrerenderedComponentHtmlContent> RenderEndpointComponent(
HttpContext httpContext,
[DynamicallyAccessedMembers(Component)] Type rootComponentType,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Text;
using System.Text.Encodings.Web;
Expand Down Expand Up @@ -37,6 +39,9 @@ public void InitializeStreamingRenderingFraming(HttpContext httpContext, bool is
}
}

// We do not want the debugger to consider NavigationExceptions caught by this method as user unhandled.
[MethodImpl(MethodImplOptions.NoInlining)]
[DebuggerDisableUserUnhandledExceptions]
public async Task SendStreamingUpdatesAsync(HttpContext httpContext, Task untilTaskCompleted, TextWriter writer)
{
// Important: do not introduce any 'await' statements in this method above the point where we write
Expand Down Expand Up @@ -80,6 +85,10 @@ public async Task SendStreamingUpdatesAsync(HttpContext httpContext, Task untilT
HandleExceptionAfterResponseStarted(_httpContext, writer, ex);
await writer.FlushAsync(); // Important otherwise the client won't receive the error message, as we're about to fail the pipeline
await _httpContext.Response.CompleteAsync();

// We need to inform the debugger that this exception should be considered user unhandled unlike the NavigationException.
// Review: Is this necessary if the method attributed with [DebuggerDisableUserUnhandledExceptions] rethrows like this does?
Debugger.BreakForUserUnhandledException(ex);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gregg-miskelly Is this necessary given that the method rethrows?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a good question. The VS Debugger's current implementation is that we will wind up eventually breaking as long as some other part of non-user code catches that exception. I think that behavior sounds right to me, but I could be convinced in either direction. Do you have an opinion?

CC @asundheim

Copy link

@asundheim asundheim Aug 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably for the best if we break as close to the original user-code throw as possible, in case the HttpContext is altered and the user wants to inspect it in as close to a state as it was when they threw? (in which case, maybe this should be the first line of the catch?)

To the question, it looks like we actually toggle the notifications for the exception off once we get CatchHandlerFound in a non-user frame, regardless if the frame has the Disable attribute, maybe that should be changed on our end? My original thought was that if one frame says we shouldn't break, then we shouldn't make every single frame above it add the attribute if it rethrew through a couple of frames before landing where it needed to be caught, but it looks like that's not the case?

If we don't change it, then this is necessary, otherwise, it becomes this method's callers problem, but we'll not get a chance to break until the caller catches it (implicitly or otherwise)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll move this up to the start of the catch (Exception ex) block since it sounds like it doesn't hurt regardless of how we decide the debugger should handle rethrows. I like that this would avoid mutating the HttpContext before the debugger breaks as you point out.

This would only be problematic if the debugger stopped both for the Debugger.BreakForUserUnhandledException(ex) and later after the rethrow, but it sounds like that would never happen.

My original thought was that if one frame says we shouldn't break, then we shouldn't make every single frame above it add the attribute if it rethrew through a couple of frames before landing where it needed to be caught, but it looks like that's not the case?

My intuition was that [DebuggerDisableUserUnhandledExceptions] would only affect exceptions that were caught and swallowed by the attributed method. I think the method either not catching an exception or rethrowing it is a pretty clear indication that the exception should still be considered "unhandled".

If there's an outer method that is catching and handling these rethrown exceptions, I would expect to be required to use [DebuggerDisableUserUnhandledExceptions] on that method too. Or more likely, I would only attribute the outermost method call that does the catching.

throw;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics.CodeAnalysis;
using System.Diagnostics.Metrics;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Text;
using System.Text.Json;
using System.Text.Json.Serialization;
Expand Down Expand Up @@ -102,8 +103,14 @@ private static void SetExceptionHandlerFeatures(ErrorContext errorContext)
/// </summary>
/// <param name="context"></param>
/// <returns></returns>
[MethodImpl(MethodImplOptions.NoInlining)]
[DebuggerDisableUserUnhandledExceptions]
public async Task Invoke(HttpContext context)
{
// We want to avoid treating exceptions as user unhandled if an exception filter like the DatabaseDeveloperPageExceptionFilter
// handles the exception rather than letting it flow to the default DisplayException method. This is because we don't want to stop the
// debugger if the developer shouldn't be handling the exception and instead just needs to do something like click a link to run a
// database migration.
try
{
await _next(context);
Expand All @@ -122,6 +129,11 @@ public async Task Invoke(HttpContext context)
context.Response.StatusCode = StatusCodes.Status499ClientClosedRequest;
}

// Generally speaking, we do not expect application code to handle things like IOExceptions during a request
// body read due to a client disconnect. But this kind of thing should be rare in development, and developers
// might be surprised if an IOException propagating through user code was not considered user unhandled.
// That said, if developers complain, we consider removing the following line.
Debugger.BreakForUserUnhandledException(ex);
return;
}

Expand All @@ -131,6 +143,8 @@ public async Task Invoke(HttpContext context)
{
_logger.ResponseStartedErrorPageMiddleware();
_metrics.RequestException(exceptionName, ExceptionResult.Skipped, handler: null);

Debugger.BreakForUserUnhandledException(ex);
throw;
}

Expand Down Expand Up @@ -161,11 +175,17 @@ public async Task Invoke(HttpContext context)
}
catch (Exception ex2)
{
// Inform the debugger that the exception filter itself threw an exception.
// REVIEW: Is it okay for the same method to potentially call Debugger.BreakForUserUnhandledException
// multiple times with different exceptions in the same invocation?
Debugger.BreakForUserUnhandledException(ex2);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gregg-miskelly I'm guessing that calling BreakForUserUnhandledException multiple times in the same invocation is fine, but I want to double check this makes sense.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing so would wind up breaking more than once, so I think you don't want to do that

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will definitely break here and show the provided exception, but the debugging experience will be a bit weird since:

  1. I don't think there will be any user-code frames on the exception stack (or the real stack), when I tried it out, you just get a bunch of non-user code frames in the debugger. We also don't have a link between ex2 and the async user frame that threw, so we won't show any [Exception] frames that will give them a chance to inspect the program state or show the source frames where they initially threw when we stop here.
  2. Is the exception caught here related to the user-code exception, besides the fact that we're in the exception page middleware? I'm not sure how likely it is that a developer would care about this particular exception, seeing as they didn't throw it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing so would wind up breaking more than once, so I think you don't want to do that

Yes, but my assumption is that the debugger would only break if the exception propagated through user code. In this case, that would mean the exception was thrown from a user-defined exception filter or a filter that called user-defined code. I think the weirdest thing about the logic in the current PR is that the debugger would break out of order with causality seemingly reversed.

  1. The original user-unhandled exception gets processed by the user-defined exception filter which then throws a secondary exception. The debugger hasn't stopped for the original user-unhandled exception yet because we were still waiting to see if any exception filters handle the exception.
  2. We then call Debugger.BreakForUserUnhandledException(ex2) to break for the secondary exception thrown by the user-defined exception filter.
  3. Then right after that, we call Debugger.BreakForUserUnhandledException(ex) with the original exception the triggered the exception filter.

I think this is pretty bad, and I should not have even opened the PR with this behavior. I think any of the following are better options.

  • Option A: Never call BreakForUserUnhandledException for exceptions thrown from an exception filter since user-defined exception filters are pretty uncommon to begin with and handling this scenario is complicated.
  • Option B: Don't use [DebuggerDisableUserUnhandledExceptions] in the developer exception page middleware at all and live with the debugger breaking before the DatabaseDeveloperPageExceptionFilter gets to run and offer the option to run the database migration by default.
  • Option C: Once we see an exception filter has thrown an uncaught exception, call BreakForUserUnhandledException for the original exception first then the secondary exception from the exception filter second immediately afterwards.

Do either of you have a preference for which option we should take? Or know of a better option?

I don't think there will be any user-code frames on the exception stack (or the real stack), when I tried it out, you just get a bunch of non-user code frames in the debugger. We also don't have a link between ex2 and the async user frame that threw

Maybe this is the disconnect. My impression is that Debugger.BreakForUserUnhandledException will only do anything if both of the following conditions are met:

  1. You have "Just my code" enabled and you have not unchecked "Break when this exception type is user-unhandled"
    Test unhandled exception
    or configured the "Additional Actions" in the exception settings to "Continue when unhandled in user code"
    https://learn.microsoft.com/en-us/visualstudio/debugger/managing-exceptions-with-the-debugger?view=vs-2022#BKMK_UserUnhandled

    P.S. the discoverability of disabling this for all exception types just disabling "Just my code" should probably be improved. There's no way I would have considered adding the "Additional Actions" column to exception settings without reading the docs. Can we just have it on by default? There aren't many columns to begin with. Also, it would be nice if I could enable breaking on user-unhandled exceptions even if "Just my code" is disabled.

  2. The exception actually propagates through user code. If there are no user-code frames on the exception stack, I would expect Debugger.BreakForUserUnhandledException to never do anything regardless of how VS was configured because the user never had the opportunity to handle the exception.

Is the exception caught here related to the user-code exception, besides the fact that we're in the exception page middleware?

We don't know if the secondary exception relates to the original exception. It certainly could be related if something about the original exception triggered different behavior in a user-defined filter.

I'm not sure how likely it is that a developer would care about this particular exception, seeing as they didn't throw it.

The developer exception page middleware has no good way of knowing if either exception was thrown from user code. I thought it would be up to the debugger to figure this out. Even the original exceptions from inner middleware that the developer exception page middleware handles may not come from or propagate through user code. The same goes for any secondary exceptions thrown by exception filters.

Copy link

@gregg-miskelly gregg-miskelly Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do either of you have a preference for which option we should take? Or know of a better option?

I don't have a strong opinion. In option 'A', are you still waiting to find out if there is an exception filter that handles the scenario? If so, that sounds reasonable to me. I certainly don't think we need to go out of the way to support breaking when there is a user-defined exception filter. I definitely agree with you that I think breaking in the "wrong" order is more confusing than only breaking for the triggering exception.

My impression is that Debugger.BreakForUserUnhandledException will only do anything if both of the following conditions are met...

The behavior that you are expecting matches what I think the behavior should be as well.

Copy link
Member Author

@halter73 halter73 Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In option 'A', are you still waiting to find out if there is an exception filter that handles the scenario?

We know that an exception filter must not have gracefully handled the scenario since the filter pipeline threw an exception. We know for sure that there are two unhandled exceptions at that point.

I went with option 'A' even though option 'C' is arguably more correct, because option 'C' would be about 5 more lines and I really doubt many people are writing custom IDeveloperPageExceptionFilter implementations, but I left a comment saying option 'C' might be sensible in case it comes back up.

I could be convinced to do option 'C' right now though if you think it's the best course of action.


// If there's a Exception while generating the error page, re-throw the original exception.
_logger.DisplayErrorPageException(ex2);
}

_metrics.RequestException(exceptionName, ExceptionResult.Unhandled, handler: null);
Debugger.BreakForUserUnhandledException(ex);
throw;
}

Expand All @@ -178,6 +198,9 @@ public async Task Invoke(HttpContext context)
// Assumes the response headers have not been sent. If they have, still attempt to write to the body.
private Task DisplayException(ErrorContext errorContext)
{
// We need to inform the debugger that this exception should be considered user unhandled since it wasn't handled by an exception filter.
Debugger.BreakForUserUnhandledException(errorContext.Exception);

var httpContext = errorContext.HttpContext;
var headers = httpContext.Request.GetTypedHeaders();
var acceptHeader = headers.Accept;
Expand Down