Clarify inner exception and aggregate exception ToString() by danmoseley · Pull Request #27333 · dotnet/coreclr

danmoseley · 2019-10-21T03:51:30Z

Continuation of the part of #25045 that was deferred from 3.0.

Using this test program - the SSL case is real, albeit the stacks are not --

It's easiest to open in separate tabs and switch between. I think the boxes make it considerably easier to match up which exception goes with which stack, and which exception is nested inside which other exception. Thoughts? Other suggestions?

Note that the non-aggregate non-nested exception case (top example) is not affected.

BTW also made some minor fixes to pretty some exceptions I noticed as I was doing this

aae660d (one bad IL message was missing path)
7b57069 (non absolute path message was missing path)
54336a7 (2 blank lines caused by runtime passing empty string for fusion log: I assume fusion log is not relevant anymore)
Only append the file name if it's not already in the exception message. It looks like the runtime pretty much always puts it in the message if it's available, in which case text line File name: 'c:\some\file' is an unnecessary line.

danmoseley · 2019-10-21T03:56:13Z

            // when it's retrieved.
            var sb = new StringBuilder(256);
            new StackTrace(fNeedFileInfo: true).ToString(System.Diagnostics.StackTrace.TraceFormat.TrailingNewLine, sb);
-            sb.AppendLine(SR.Exception_EndStackTraceFromPreviousThrow);


I removed this line (and thus the string) because elsewhere where remote stack trace and stack trace are combined, they are concatenated without a separator. Eg.,

coreclr/src/System.Private.CoreLib/src/System/Exception.CoreCLR.cs

Line 43 in c37aab5

_remoteStackTraceString += _stackTraceString;

coreclr/src/System.Private.CoreLib/src/System/Exception.CoreCLR.cs

Line 171 in c37aab5

return remoteStackTraceString + stackTraceString;

coreclr/src/System.Private.CoreLib/src/System/Exception.CoreCLR.cs

Line 178 in c37aab5

return remoteStackTraceString + GetStackTrace(this);

Please put it back. Have you looked at what the stack traces look like without it?

jkotas · 2019-10-21T07:08:26Z

+                return "";
+
+            /* Format something like this to clearly separate from the containing exception
+             ________________________________________________________________________________________________


I believe that the ascii art part of the change has significant risk of breaking tools that analyze exception stacktraces, and it may be controversial (e.g. my personal preference would be a more compact format without the ascii art, but I am happy to be overruled.)

If you would like to pursue it, I think it should have its own issue and PR. The issue should be marked as breaking change and the stakeholders should be included to comment on it:

The community projects that are in the business of formatting and understanding exception stacktraces (e.g. Ben.Demystifier)

Our diagnostic and TI teams

breaking tools that analyze exception stacktraces

e.g. some of our own corefx tests are failing because of this problem

I really just wanted to start a discussion. Any suggestions for an alternative? Maybe the arrows? (Is there general agreement the existing format is hard to correlate?)

I'll close this and open an issue to discuss that...

stephentoub · 2019-10-21T11:35:18Z

+            foreach (Exception ex in m_innerExceptions)
            {
-                if (m_innerExceptions[i] == InnerException)
+                if (object.ReferenceEquals(ex, InnerException))


Why is == inappropriate here?

We want to filter out the exact same object - figured it was clearer. Also a subclass could have overridden the equals operator.

Also a subclass could have overridden the equals operator.

That's actually not how == works. Unless the strongly-typed variable being used defines an operator == (and Exception doesn't), this will be emitted with the exact same behavior as ReferenceEquals. It doesn't use Equals. e.g. this will output false:

using System; class Program { static void Main() => Console.WriteLine(new Program() == new Program()); public override bool Equals(object obj) => true; }

I had forgotten this. In the case where this is known to be true (as here), do we prefer to use == for brevity, or O.RE to be explicit about intent? I assume the former as we don't seem to use O.RE that much.

It often ends up being personal preference.

Roslyn compiles ReferenceEquals into the same IL as what you get from ==, so it is just a style difference. Similar to == null vs. is null.

stephentoub · 2019-10-21T11:37:28Z

-                text.Append(m_innerExceptions[i].ToString());
-                text.Append("<---");
-                text.AppendLine();
+                text.Append(InnerExceptionToString(ex));


This helper ends up allocating it's own string builder, formatting to it, getting its string, and then appending that string to this builder. Why not just pass this builder in? If the concern is reusing it in places that don't use a builder, you can make the helper create the builder only if one want passed in, and return a its ToString only if one was.

stephentoub · 2019-10-21T11:38:41Z

            string s = GetType().ToString() + ": " + Message;

-            if (!string.IsNullOrEmpty(_fileName))
+            if (!string.IsNullOrEmpty(_fileName) && !Message.Contains(_fileName))


Is this all that meaningful? While we discourage parsing, the current format is much more parsable for the filename.

Assuming the filename is set and not just in the message. Ok, guess I need feedback on this one.

Apart from reducing noise, another possible advantage of reducing line count is that the parts of the output are closer together in a log produced from concurrent Interleaving outputs eg a build log or test run.

stephentoub · 2019-10-21T11:41:26Z

-                    {
-                        sb.AppendLine();
-                        sb.Append(SR.Exception_EndStackTraceFromPreviousThrow);
-                    }


Why is this being removed?

The idea was that the boundary could be inferred - this was in the previous PR I think.

I don't believe that's easy to do in many/all cases. We explicitly skip this for async because that case generally is easier, but for other cases, you're generally not nearly as linear, and you end up with stack traces that look impossible, which is much more confusing and time consuming to analyze than when they're clearly demarcated.

jkotas · 2019-10-21T16:16:45Z

Some of the changes in the PR like including the filename for bad image format exceptions are good and non-contentious. They can be submitted separately from the more profound changes.

danmoseley added 6 commits October 20, 2019 15:48

Add path to Bad IL exception message

aae660d

Add path to not-absolute-path exception

7b57069

Remove empty lines caused by empty fusion log string

54336a7

Remove 'end stack trace from previous location'

1f7c0e9

Add arrows and indenting

2dc55bd

Change arrows to ASCII box

ba389ee

danmoseley requested review from jkotas and stephentoub October 21, 2019 03:51

Neaten

c37aab5

danmoseley commented Oct 21, 2019

View reviewed changes

Fix chk build

8392980

danmoseley force-pushed the exceptionsv2 branch from d2be58c to 8392980 Compare October 21, 2019 05:21

danmoseley commented Oct 21, 2019

View reviewed changes

Comment thread src/vm/assemblynative.cpp Outdated

danmoseley requested a review from davidsh October 21, 2019 05:26

Better

7a6b228

jkotas reviewed Oct 21, 2019

View reviewed changes

stephentoub reviewed Oct 21, 2019

View reviewed changes

danmoseley closed this Oct 21, 2019

danmoseley mentioned this pull request Jun 30, 2020

AggregateException.ToString() prints terrible message dotnet/runtime#28695

Closed

Conversation

danmoseley commented Oct 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley Oct 21, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephentoub Oct 21, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Oct 21, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas commented Oct 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danmoseley commented Oct 21, 2019 •

edited

Loading

danmoseley Oct 21, 2019 •

edited

Loading

stephentoub Oct 21, 2019 •

edited

Loading

jkotas Oct 21, 2019 •

edited

Loading