More fault handling cleanup by jasonmalinowski · Pull Request #59097 · dotnet/roslyn

jasonmalinowski · 2022-01-27T04:02:28Z

Commit-by-commit recommended. For the addition of severities, they were chosen based on the rough criteria described for each option:

roslyn/src/Compilers/Core/Portable/InternalUtilities/FatalError.cs

Lines 260 to 286 in 0b5e47b

    
           /// <summary> 
        
           /// The severity of the error, see the enum members for a description of when to use each. This is metadata that's included 
        
           /// in a non-fatal fault report, which we can take advantage of on the backend to automatically triage bugs. For example, 
        
           /// a critical severity issue we can open with a lower bug count compared to a low priority one. 
        
           /// </summary> 
        
           internal enum ErrorSeverity 
        
           { 
        
               /// <summary> 
        
               /// Something failed, but the user is unlikely to notice. Especially useful for background things that we can silently recover 
        
               /// from, like bugs in caching systems. 
        
               /// </summary> 
        
               Diagnostic, 
        
               /// <summary> 
        
               /// Something failed, and the user might notice, but they're still likely able to carry on. For example, if the user 
        
               /// asked for some information from the IDE (find references, completion, etc.) and we were able to give partial results. 
        
               /// </summary> 
        
               General, 
        
               /// <summary> 
        
               /// Something failed, and the user likely noticed. For example, the user pressed a button to do an action, and 
        
               /// we threw an exception so we completely failed to do that in an unrecoverable way. This may also be used 
        
               /// for back-end systems where a failure is going to result in a highly broken experience, for example if parsing a file 
        
               /// catastrophically failed. 
        
               /// </summary> 
        
               Critical 
        
           }

JoeRobich · 2022-02-03T22:37:29Z

        /// <returns><see langword="false"/> to avoid catching the exception.</returns>
        [DebuggerHidden]
-        public static bool ReportAndPropagateUnlessCanceled(Exception exception, CancellationToken contextCancellationToken, ErrorSeverity severity)
+        public static bool ReportAndPropagateUnlessCanceled(Exception exception, ErrorSeverity severity, CancellationToken contextCancellationToken)


Did 'Change Signature' make this easy?

It did! Although sometimes it did nothing at all, and my effort to debug that kept resulting in the debugger breaking. 😄

Everything has to be categorized from now on.

Otherwise these are using the "regular" error reporting which will send non-fatal events.

RikkiGibson · 2022-02-09T21:45:18Z

Could you please elaborate on how severities were chosen in general?

jasonmalinowski · 2022-02-09T21:52:39Z

@RikkiGibson Oh, forgot to update the PR for that. PR description updated now.

sharwell · 2022-02-10T18:46:01Z

                        Visit(m);
                    }
-                    catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e))
+                    catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e, ErrorSeverity.General))


ErrorSeverity.General should be the default. This coding pattern is unnecessarily burdensome.

ghost added the Area-IDE label Jan 27, 2022

jasonmalinowski self-assigned this Jan 27, 2022

jasonmalinowski force-pushed the more-fault-handling-cleanup branch 2 times, most recently from a958bba to 8197c61 Compare January 28, 2022 20:48

This was referenced Jan 28, 2022

Transient failure in test AsyncIteratorWithCustomCode #34207

Closed

Test failure in Microsoft.CodeAnalysis.CSharp.Emit.UnitTests.dll #57560

Closed

jasonmalinowski force-pushed the more-fault-handling-cleanup branch from 8197c61 to 275c552 Compare February 3, 2022 19:24

jasonmalinowski marked this pull request as ready for review February 3, 2022 22:08

jasonmalinowski requested review from a team as code owners February 3, 2022 22:08

jasonmalinowski requested a review from a team February 3, 2022 22:08

JoeRobich reviewed Feb 3, 2022

View reviewed changes

jasonmalinowski force-pushed the more-fault-handling-cleanup branch from d196377 to e609476 Compare February 4, 2022 20:43

runfoapp Bot mentioned this pull request Feb 4, 2022

[Flaky Test] VisualBasic.BasicAddMissingReference.InvokeSomeFixesInVisualBasicThenVerifyReferences #57980

Closed

jasonmalinowski marked this pull request as draft February 7, 2022 20:23

This comment was marked as outdated.

Sign in to view

jasonmalinowski force-pushed the more-fault-handling-cleanup branch from e609476 to 5827aea Compare February 7, 2022 20:39

jasonmalinowski added 3 commits February 8, 2022 13:08

Remove the "uncategorized" error severity

268cc75

Everything has to be categorized from now on.

Switch integration test handlers to explicitly call FailFast

17ec9c3

Otherwise these are using the "regular" error reporting which will send non-fatal events.

Reorder parameters to put the cancellation tokens last

0b5e47b

jasonmalinowski force-pushed the more-fault-handling-cleanup branch from 5827aea to 0b5e47b Compare February 8, 2022 21:15

jasonmalinowski marked this pull request as ready for review February 8, 2022 23:20

JoeRobich approved these changes Feb 9, 2022

View reviewed changes

jmarolf approved these changes Feb 9, 2022

View reviewed changes

sharwell suggested changes Feb 10, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More fault handling cleanup#59097

More fault handling cleanup#59097
jasonmalinowski wants to merge 3 commits into
dotnet:mainfrom
jasonmalinowski:more-fault-handling-cleanup

jasonmalinowski commented Jan 27, 2022 •

edited

Loading

Uh oh!

JoeRobich Feb 3, 2022

Uh oh!

jasonmalinowski Feb 3, 2022

Uh oh!

This comment was marked as outdated.

RikkiGibson commented Feb 9, 2022

Uh oh!

jasonmalinowski commented Feb 9, 2022

Uh oh!

sharwell Feb 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	/// <summary>
	/// The severity of the error, see the enum members for a description of when to use each. This is metadata that's included
	/// in a non-fatal fault report, which we can take advantage of on the backend to automatically triage bugs. For example,
	/// a critical severity issue we can open with a lower bug count compared to a low priority one.
	/// </summary>
	internal enum ErrorSeverity
	{
	/// <summary>
	/// Something failed, but the user is unlikely to notice. Especially useful for background things that we can silently recover
	/// from, like bugs in caching systems.
	/// </summary>
	Diagnostic,

	/// <summary>
	/// Something failed, and the user might notice, but they're still likely able to carry on. For example, if the user
	/// asked for some information from the IDE (find references, completion, etc.) and we were able to give partial results.
	/// </summary>
	General,

	/// <summary>
	/// Something failed, and the user likely noticed. For example, the user pressed a button to do an action, and
	/// we threw an exception so we completely failed to do that in an unrecoverable way. This may also be used
	/// for back-end systems where a failure is going to result in a highly broken experience, for example if parsing a file
	/// catastrophically failed.
	/// </summary>
	Critical
	}

Conversation

jasonmalinowski commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoeRobich Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Feb 3, 2022

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

RikkiGibson commented Feb 9, 2022

Uh oh!

jasonmalinowski commented Feb 9, 2022

Uh oh!

sharwell Feb 10, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jasonmalinowski commented Jan 27, 2022 •

edited

Loading