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

[API Proposal]: DebuggerDisableUserUnhandledAttribute#103105

Closed
halter73 opened this issue Jun 5, 2024 · 5 comments · Fixed by #104813
Closed

[API Proposal]: DebuggerDisableUserUnhandledAttribute #103105

halter73 opened this issue Jun 5, 2024 · 5 comments · Fixed by #104813
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@halter73
Copy link
Member

halter73 commented Jun 5, 2024

Background and motivation

In .NET 9, Visual Studio is adding support for catching async user-unhandled exceptions which will be enabled by default. This feature has existed for a long time for synchronous methods, but not for async methods. There are several exceptions in ASP.NET Core that propagate through user code but are expected to be handled by framework code. One example is the NavigationException used to force redirects when calling NavigationManager.NavigateTo() during static Blazor rendering.

Async user unhandled exception

The proposal is to add an attribute that can be added to framework methods that handle exceptions that propagate through user code like Blazors NavigationException and a Debugger method that these attributed framework methods can use to indicate the exception should really be treated as user unhandled. This gives the framework the opportunity to do runtime checks on the exception and run it through dynamic filtering logic before deciding if the exception should really be considered user-unhandled.

API Proposal

namespace System.Diagnostics;

/// <summary>
/// If a .NET Debugger is attached which supports the BreakForException API,
/// this attribute will prevent the debugger from breaking on user-unhandled exceptions
/// when the exception is caught by a method with this attribute, unless BreakForException is called.
/// </summary>
[AttributeUsage(AttributeTargets.Method)]
public sealed class DebuggerDisableUserUnhandledExceptionsAttribute : Attribute
{
    public DebuggerDisableUserUnhandledExceptionsAttribute();
}
namespace System.Diagnostics;

public static class Debugger
{
+    /// <summary>
+    /// If a .NET Debugger is attached which supports the BreakForException'API,
+    /// this method will break into the debugger, showing a virtual call stack from the exception.
+    /// </summary>
+    /// <param name="ex">exception object to show</param>
+    public static void BreakForException(Exception ex);
}

API Usage

[MethodImpl(MethodImplOptions.NoInlining)]
[DebuggerDisableUserUnhandledExceptions]
static async Task InvokeUserCode(Func<Task> userCode)
{
    try
    {
        await userCode();
    }
    catch (Exception ex)
    {
        if (TryHandleWithFilter(ex))
        {
            return; // example case where we don't want to break for user-unhandled exceptions
        }

        Debugger.BreakForException(e); // debugger will stop here and show the exception if attached.
    }
}

Alternative Designs

If other frameworks or libraries do not need this, the VS debugger could special case these ASP.NET Core exceptions.

It might also be easier in some cases for framework and library developers if they could add an attribute to the Exception type itself if it's a type should always be handled by the framework like Blazor's NavigationException.

namespace System.Diagnostics;

[AttributeUsage(AttributeTargets.Class)]
public sealed class DebuggerDisableUserUnhandledAttribute : Attribute
{
    public DebuggerDisableUserUnhandledAttribute();
}
using System.Diagnostics;

namespace Microsoft.AspNetCore.Components;
 
[DebuggerDisableUserUnhandled]
public class NavigationException : Exception
{
    // ....
}

However, this might be too inflexible because it's all or nothing in terms of whether a given exception type should be considered "user unhandled" regardless of what method threw it or what non-user methods are on the stack that could handle it. This would not work as well as the primary proposal when the exception is handled by an exception "filter" or "handler" that's called after the exception is caught and therefore is not on the stack.

Risks

This use case may be too niche to warrant new runtime API. This might also make it more confusing to developers who expect the debugger to break every user-unhandled exception propagating through user code even if the framework gracefully handles it.

@gregg-miskelly

@halter73 halter73 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 5, 2024
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 5, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jun 5, 2024
@terrajobst
Copy link
Member

I assume the intent is that it would control the default behavior of the debugger for "break when an exception of this type is thrown"? IOW, if the exception still goes unhandled, I'd think the debugger would still break at that point?

@terrajobst terrajobst added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 6, 2024
@terrajobst terrajobst added this to the 9.0.0 milestone Jun 6, 2024
@terrajobst
Copy link
Member

(applying labels to ensure it's high on our backlog)

@vcsjones vcsjones added area-System.Diagnostics and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 6, 2024
@dotnet-policy-service dotnet-policy-service bot removed the untriaged New issue has not been triaged by the area owner label Jun 6, 2024
Copy link
Contributor

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

@teo-tsirpanis teo-tsirpanis removed the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jun 6, 2024
@hoyosjs hoyosjs added the enhancement Product code improvement that does NOT require public API changes/additions label Jun 10, 2024
@stephentoub
Copy link
Member

I'm unclear as to what the state of this proposal is. Is this a feature that the VS debugger has already agreed to, and we're just talking about API name? Or is this actually proposing a feature that still needs agreement / design / etc. at the debugger level?

@terrajobst terrajobst removed the blocking Marks issues that we want to fast track in order to unblock other important work label Jun 11, 2024
@halter73 halter73 added the blocking Marks issues that we want to fast track in order to unblock other important work label Jul 2, 2024
@bartonjs
Copy link
Member

bartonjs commented Jul 9, 2024

Video

  • BreakForException => BreakForUserUnhandledException
  • It was asked if the parameter to BreakForUserUnhandledException could be removed, and just use $exception; but it was felt that the parameter was useful.
  • We discussed "DebuggerDisable" or "DisableDebugger", and feel that the prefix "Debugger" gives better alignment to other Debugger-intent attributes in this namespace.
  • Spell out "ex" to "exception" in the parameter.
namespace System.Diagnostics;

[AttributeUsage(AttributeTargets.Method)]
public sealed class DebuggerDisableUserUnhandledExceptionsAttribute : Attribute
{
    public DebuggerDisableUserUnhandledExceptionsAttribute();
}

public partial class Debugger
{
    public static void BreakForUserUnhandledException(Exception exception);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jul 9, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants