From 96022f03ce2fd5d334a2a3f66befcadfc671fd42 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 21 Oct 2024 16:52:52 -0700 Subject: [PATCH 1/2] Add [DebuggerDisableUserUnhandledExceptions] closer to user code --- .../Components/src/ComponentBase.cs | 36 ++++++++++++++++--- .../Components/src/RenderTree/Renderer.cs | 2 ++ .../src/Rendering/ComponentState.cs | 7 ++++ .../EndpointHtmlRenderer.Streaming.cs | 4 --- 4 files changed, 41 insertions(+), 8 deletions(-) diff --git a/src/Components/Components/src/ComponentBase.cs b/src/Components/Components/src/ComponentBase.cs index 5de04ae8d70b..a5cf641cae93 100644 --- a/src/Components/Components/src/ComponentBase.cs +++ b/src/Components/Components/src/ComponentBase.cs @@ -1,6 +1,7 @@ // 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 Microsoft.AspNetCore.Components.Rendering; namespace Microsoft.AspNetCore.Components; @@ -271,10 +272,22 @@ public virtual Task SetParametersAsync(ParameterView parameters) } } + // We do not want the debugger to consider NavigationExceptions caught by this method as user-unhandled. + [DebuggerDisableUserUnhandledExceptions] private async Task RunInitAndSetParametersAsync() { - OnInitialized(); - var task = OnInitializedAsync(); + Task task; + + try + { + OnInitialized(); + task = OnInitializedAsync(); + } + catch (Exception ex) when (ex is not NavigationException) + { + Debugger.BreakForUserUnhandledException(ex); + throw; + } if (task.Status != TaskStatus.RanToCompletion && task.Status != TaskStatus.Canceled) { @@ -307,10 +320,23 @@ private async Task RunInitAndSetParametersAsync() await CallOnParametersSetAsync(); } + // We do not want the debugger to consider NavigationExceptions caught by this method as user-unhandled. + [DebuggerDisableUserUnhandledExceptions] private Task CallOnParametersSetAsync() { - OnParametersSet(); - var task = OnParametersSetAsync(); + Task task; + + try + { + OnParametersSet(); + task = OnParametersSetAsync(); + } + catch (Exception ex) when (ex is not NavigationException) + { + Debugger.BreakForUserUnhandledException(ex); + throw; + } + // If no async work is to be performed, i.e. the task has already ran to completion // or was canceled by the time we got to inspect it, avoid going async and re-invoking // StateHasChanged at the culmination of the async work. @@ -326,6 +352,8 @@ private Task CallOnParametersSetAsync() Task.CompletedTask; } + // We do not want the debugger to stop more than once per user-unhandled exception. + [DebuggerDisableUserUnhandledExceptions] private async Task CallStateHasChangedOnAsyncCompletion(Task task) { try diff --git a/src/Components/Components/src/RenderTree/Renderer.cs b/src/Components/Components/src/RenderTree/Renderer.cs index e0fcfd834340..a1f41cfa4678 100644 --- a/src/Components/Components/src/RenderTree/Renderer.cs +++ b/src/Components/Components/src/RenderTree/Renderer.cs @@ -1039,6 +1039,8 @@ async Task ContinueAfterTask(ArrayRange eventHandlerIds, Task afterTaskIg } } + // We do not want the debugger to stop more than once per user-unhandled exception. + [DebuggerDisableUserUnhandledExceptions] private async Task GetErrorHandledTask(Task taskToHandle, ComponentState? owningComponentState) { try diff --git a/src/Components/Components/src/Rendering/ComponentState.cs b/src/Components/Components/src/Rendering/ComponentState.cs index 4d973398f639..3e30105c2bc7 100644 --- a/src/Components/Components/src/Rendering/ComponentState.cs +++ b/src/Components/Components/src/Rendering/ComponentState.cs @@ -87,6 +87,8 @@ public ComponentState(Renderer renderer, int componentId, IComponent component, internal Renderer Renderer => _renderer; + // We do not want the debugger to consider NavigationExceptions caught by this method as user-unhandled. + [DebuggerDisableUserUnhandledExceptions] internal void RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment renderFragment, out Exception? renderFragmentException) { renderFragmentException = null; @@ -106,6 +108,11 @@ internal void RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment re } catch (Exception ex) { + if (ex is not NavigationException) + { + Debugger.BreakForUserUnhandledException(ex); + } + // If an exception occurs in the render fragment delegate, we won't process the diff in any way, so child components, // event handlers, etc., will all be left untouched as if this component didn't re-render at all. The Renderer will // then forcibly clear the descendant subtree by rendering an empty fragment for this component. diff --git a/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Streaming.cs b/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Streaming.cs index f7fd6a8860f8..ca70e155825d 100644 --- a/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Streaming.cs +++ b/src/Components/Endpoints/src/Rendering/EndpointHtmlRenderer.Streaming.cs @@ -77,10 +77,6 @@ public async Task SendStreamingUpdatesAsync(HttpContext httpContext, Task untilT } catch (Exception ex) { - // Rethrowing also informs the debugger that this exception should be considered user-unhandled unlike NavigationExceptions, - // but calling BreakForUserUnhandledException here allows the debugger to break before we modify the HttpContext. - Debugger.BreakForUserUnhandledException(ex); - // Theoretically it might be possible to let the error middleware run, capture the output, // then emit it in a special format so the JS code can display the error page. However // for now we're not going to support that and will simply emit a message. From 252061ee11ef7fb979e03fe4a531bbe7dd710a13 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 22 Oct 2024 02:17:09 -0700 Subject: [PATCH 2/2] Remove [DebuggerDisableUserUnhandledExceptions] from developer exception page middleware --- .../DeveloperExceptionPageMiddlewareImpl.cs | 20 ------------------- 1 file changed, 20 deletions(-) diff --git a/src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs b/src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs index 2f33111ccb3b..cfc26131c1a1 100644 --- a/src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs +++ b/src/Middleware/Diagnostics/src/DeveloperExceptionPage/DeveloperExceptionPageMiddlewareImpl.cs @@ -102,13 +102,8 @@ private static void SetExceptionHandlerFeatures(ErrorContext errorContext) /// /// /// - [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); @@ -127,11 +122,6 @@ 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 aborted requests should be rare in development, and developers - // might be surprised if an IOException propagating through their code was not considered user-unhandled. - // That said, if developers complain, we consider removing the following line. - Debugger.BreakForUserUnhandledException(ex); return; } @@ -141,8 +131,6 @@ public async Task Invoke(HttpContext context) { _logger.ResponseStartedErrorPageMiddleware(); _metrics.RequestException(exceptionName, ExceptionResult.Skipped, handler: null); - - // Rethrowing informs the debugger that this exception should be considered user-unhandled. throw; } @@ -173,16 +161,11 @@ public async Task Invoke(HttpContext context) } catch (Exception ex2) { - // It might make sense to call BreakForUserUnhandledException for ex2 after we do the same for the original exception. - // But for now, considering the rarity of user-defined IDeveloperPageExceptionFilters, we're not for simplicity. - // 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); - - // Rethrowing informs the debugger that this exception should be considered user-unhandled. throw; } @@ -195,9 +178,6 @@ 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 fully handled by an exception filter. - Debugger.BreakForUserUnhandledException(errorContext.Exception); - var httpContext = errorContext.HttpContext; var headers = httpContext.Request.GetTypedHeaders(); var acceptHeader = headers.Accept;