Add helper method to handle Debug.Fail #11270

RussKie · 2023-10-14T06:41:56Z

No description provided.

gerhardol

+1
Will maybe want to have some way to skip the handling also if debugger is attached

GitExtUtils/DebugHelpers.cs

RussKie · 2023-10-14T08:06:40Z

Will maybe want to have some way to skip the handling also if debugger is attached

Can you please elaborate?

mstv

👍

GitCommands/Git/AheadBehindDataProvider.cs

gerhardol · 2023-10-14T17:21:03Z

Will maybe want to have some way to skip the handling also if debugger is attached

Can you please elaborate?

When in the debugger, you have to press continue several times for some reason, so it is unclear if you could ignore the exception.
It is not simple to just copy the stacktrace and continue.

Outside the debugger, you get the dialog to connect to the debugger.
If I run my dailies, I would prefer to get an exception so I could copy the stacktrace and investigate at a later time (I do not have msvs at work for instance).

None of these comments is against a merge as is.
I could make a private patch too, not sure what to do for the generic code.

mstv · 2023-10-14T18:34:58Z

I would prefer to get an exception so I could copy the stacktrace and investigate at a later time (I do not have msvs at work for instance).

I could make a private patch too, not sure what to do for the generic code.

I am going to route consumed exceptions to the GitOutputControl in addition to to Trace. The same can be done for these DebugHelpers.

GitExtUtils/DebugHelpers.cs

RussKie · 2023-10-15T02:53:38Z

GitExtUtils/DebugHelpers.cs

+            Debugger.Launch();
+
+            if (!Debugger.IsAttached)
+            {
+                throw new InvalidOperationException(message);
+            }


This will prompt to attach a debugger, and if that's declined an exception will get routed to NBug... HOWEVER, if the code that triggered this was executing on a background thread it'd lead to the termination of the app.

HOWEVER, if the code that triggered this was executing on a background thread it'd lead to the termination of the app.

It terminates the app but after starting the BugReporter, isn't it?

The goal is to not have background threads without exception handling. I assume that there is (almost) no such background operation left since the recent PR for JTF.RunAsync and FileAndForget.

It terminates the app but after starting the BugReporter, isn't it?

Correct.

RussKie · 2023-10-15T02:57:35Z

GitExtUtils/DebugHelpers.cs

+        else if (!condition)
+        {
+            Debugger.Launch();
+
+            if (!Debugger.IsAttached)
+            {
+                throw new InvalidOperationException(message);
+            }
+        }


...the above makes me quite unhappy with this section. While nagging the dev in an invalid situation (guarded by Debug.Fail) is warranted, it's not appropriate for a "check" situaion (i.e., Debug.Assert) - an assert should not bring the app down.

That's the fundamental problem with Debug.Assert.
It is used for making assumptions explicit (good) - with the expectation that they will never fail.
(I see that these sanity checks are skipped in the release build for performance reasons.)
In most cases, it is wrong and unsafe to continue the execution. Sometimes, the code throws other exceptions. In many cases, it is just a lack of error handling in release builds (which is bad IMO).

mstv

LGTM

GitExtUtils/DebugHelpers.cs

mstv · 2023-10-15T10:39:11Z

GitExtUtils/DebugHelpers.cs

+            Debugger.Launch();
+
+            if (!Debugger.IsAttached)
+            {
+                throw new InvalidOperationException(message);
+            }


HOWEVER, if the code that triggered this was executing on a background thread it'd lead to the termination of the app.

It terminates the app but after starting the BugReporter, isn't it?

The goal is to not have background threads without exception handling. I assume that there is (almost) no such background operation left since the recent PR for JTF.RunAsync and FileAndForget.

mstv · 2023-10-15T10:47:45Z

GitExtUtils/DebugHelpers.cs

+        else if (!condition)
+        {
+            Debugger.Launch();
+
+            if (!Debugger.IsAttached)
+            {
+                throw new InvalidOperationException(message);
+            }
+        }


That's the fundamental problem with Debug.Assert.
It is used for making assumptions explicit (good) - with the expectation that they will never fail.
(I see that these sanity checks are skipped in the release build for performance reasons.)
In most cases, it is wrong and unsafe to continue the execution. Sometimes, the code throws other exceptions. In many cases, it is just a lack of error handling in release builds (which is bad IMO).

mstv · 2023-10-19T18:23:14Z

MC resolved

RussKie · 2023-10-19T20:39:27Z

Thank you

ghost assigned RussKie Oct 14, 2023

gerhardol reviewed Oct 14, 2023

View reviewed changes

GitExtUtils/DebugHelpers.cs Outdated Show resolved Hide resolved

RussKie force-pushed the debughelpers branch from 04c6eb4 to 9c3999a Compare October 14, 2023 09:12

mstv approved these changes Oct 14, 2023

View reviewed changes

GitCommands/Git/AheadBehindDataProvider.cs Show resolved Hide resolved

mstv reviewed Oct 14, 2023

View reviewed changes

GitExtUtils/DebugHelpers.cs Outdated Show resolved Hide resolved

RussKie force-pushed the debughelpers branch from 9c3999a to 6730a33 Compare October 15, 2023 02:50

RussKie commented Oct 15, 2023

View reviewed changes

RussKie force-pushed the debughelpers branch 2 times, most recently from db0d497 to 65f5df1 Compare October 15, 2023 03:04

mstv reviewed Oct 15, 2023

View reviewed changes

RussKie force-pushed the debughelpers branch 2 times, most recently from 65f5df1 to 8c2d9cb Compare October 18, 2023 22:50

Add helper method to handle Debug.Assert and Debug.Fail

b3d3102

mstv force-pushed the debughelpers branch from 8c2d9cb to b3d3102 Compare October 19, 2023 18:50

mstv approved these changes Oct 19, 2023

View reviewed changes

mstv merged commit 39655a0 into gitextensions:master Oct 19, 2023

ghost added this to the vNext milestone Oct 19, 2023

mstv mentioned this pull request Oct 19, 2023

Do not launch debugger on Debug.Fail in test run #11287

Merged

RussKie deleted the debughelpers branch November 18, 2023 02:45

Uh oh!

Add helper method to handle Debug.Fail #11270

Add helper method to handle Debug.Fail #11270

Uh oh!

Conversation

RussKie commented Oct 14, 2023

Uh oh!

gerhardol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RussKie commented Oct 14, 2023

Uh oh!

mstv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gerhardol commented Oct 14, 2023

Uh oh!

mstv commented Oct 14, 2023

Uh oh!

Uh oh!

RussKie Oct 15, 2023

Choose a reason for hiding this comment

Uh oh!

mstv Oct 15, 2023

Choose a reason for hiding this comment

Uh oh!

RussKie Oct 15, 2023

Choose a reason for hiding this comment

Uh oh!

RussKie Oct 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mstv Oct 15, 2023

Choose a reason for hiding this comment

Uh oh!

mstv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mstv Oct 15, 2023

Choose a reason for hiding this comment

Uh oh!

mstv Oct 15, 2023

Choose a reason for hiding this comment

Uh oh!

mstv commented Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RussKie commented Oct 19, 2023 via email • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

RussKie Oct 15, 2023 •

edited

Loading

mstv commented Oct 19, 2023 •

edited

Loading

RussKie commented Oct 19, 2023 via email •

edited by ghost

Loading