Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Show nested exceptions more clearly #25045

Closed
danmoseley wants to merge 5 commits into from
Closed

Show nested exceptions more clearly #25045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4cb6fba
Change ArgumentException message to one line
danmoseley Jun 2, 2019
3572733
Extract common code to Exception
danmoseley Jun 2, 2019
c32b26a
Prefix inner exception string
danmoseley Jun 9, 2019
b0dd864
Attempt to clean up AE
danmoseley Jun 9, 2019
85fd408
Remove EDI boundary?
danmoseley Jun 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions src/System.Private.CoreLib/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,6 @@
<data name="AggregateException_DeserializationFailure" xml:space="preserve">
<value>The serialization stream contains no inner exceptions.</value>
</data>
<data name="AggregateException_InnerException" xml:space="preserve">
<value>(Inner Exception #{0}) </value>
<comment>This text is prepended to each inner exception description during aggregate exception formatting</comment>
</data>
<data name="AppDomain_AppBaseNotSet" xml:space="preserve">
<value>The ApplicationBase must be set before retrieving this property.</value>
</data>
Expand Down Expand Up @@ -668,7 +664,7 @@
<value>Insufficient memory to continue the execution of the program.</value>
</data>
<data name="Arg_ParamName_Name" xml:space="preserve">
<value>Parameter name: {0}</value>
<value>(parameter '{0}')</value>
</data>
<data name="Arg_ParmArraySize" xml:space="preserve">
<value>Must specify one or more parameters.</value>
Expand Down Expand Up @@ -2224,12 +2220,6 @@
<data name="EventSource_VarArgsParameterMismatch" xml:space="preserve">
<value>The parameters to the Event method do not match the parameters to the WriteEvent method. This may cause the event to be displayed incorrectly.</value>
</data>
<data name="Exception_EndOfInnerExceptionStack" xml:space="preserve">
Copy link
Copy Markdown
Member

@jkotas jkotas Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not unusual for folks to have parser of the Exception.ToString message - to reformat it or to identify root cause. Here is an example found by a quick github search:

https://github.com/pieceofsummer/Hangfire.StackTrace/blob/45ced0e988bee23f86d71c2fa60997b9210f677a/src/Hangfire.StackTrace/FailedStateRenderer.cs#L146

This change is likely going to break these ... that may be ok, but we do not have much time to react to feedback in 3.0.

Copy link
Copy Markdown

@redknightlois redknightlois Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cant we just enable that through configuration. In that way, people can start preparing for it, like it was done with tiered JIT.

Copy link
Copy Markdown
Member Author

@danmoseley danmoseley Jun 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have used AppContext switches less than a dozen times in .NET Core so far. If it is a temporary thing we take out after 3.0 then I do not object. I can use a switch to hide most of this just for a few weeks. Of course that means that few if anyone "benefits" from this in 3.0.

Another option is that I keep the changes to put messages on their own lines, and to avoid duplicating the first inner exception in the AggregateException, but defer the change to indent and defer removing the two dividers.

thoughts?

Copy link
Copy Markdown
Member Author

@danmoseley danmoseley Jun 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved part into #25185 for 3.0, and will pick this up again when master is open for .NET 5.

<value>--- End of inner exception stack trace ---</value>
</data>
<data name="Exception_EndStackTraceFromPreviousThrow" xml:space="preserve">
<value>--- End of stack trace from previous location where exception was thrown ---</value>
</data>
<data name="Exception_WasThrown" xml:space="preserve">
<value>Exception of type '{0}' was thrown.</value>
</data>
Expand Down
22 changes: 5 additions & 17 deletions src/System.Private.CoreLib/shared/System/AggregateException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,26 +440,14 @@ public override string Message
}
}

/// <summary>
/// Creates and returns a string representation of the current <see cref="AggregateException"/>.
/// </summary>
/// <returns>A string representation of the current exception.</returns>
public override string ToString()
protected internal override string InnerExceptionToString(Exception? inner = null)
{
StringBuilder text = new StringBuilder();
text.Append(base.ToString());

for (int i = 0; i < m_innerExceptions.Count; i++)
var sb = new StringBuilder();
foreach(Exception ex in m_innerExceptions)
{
text.AppendLine();
text.Append("---> ");
text.AppendFormat(CultureInfo.InvariantCulture, SR.AggregateException_InnerException, i);
text.Append(m_innerExceptions[i].ToString());
text.Append("<---");
text.AppendLine();
sb.Append(base.InnerExceptionToString(ex));
Copy link
Copy Markdown
Member

@stephentoub stephentoub Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having removed all of the separators, what does an AggregateException now look like?

Copy link
Copy Markdown
Member Author

@danmoseley danmoseley Jun 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See example linked in top post. I can also add one with an AggE wrapping an AggE.

}

return text.ToString();
return sb.ToString();
}

/// <summary>
Expand Down
7 changes: 3 additions & 4 deletions src/System.Private.CoreLib/shared/System/ArgumentException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,10 @@ public override string Message
string s = base.Message;
if (!string.IsNullOrEmpty(_paramName))
{
string resourceString = SR.Format(SR.Arg_ParamName_Name, _paramName);
return s + Environment.NewLine + resourceString;
s += " " + SR.Format(SR.Arg_ParamName_Name, _paramName);
}
else
return s;

return s;
}
}

Expand Down
3 changes: 1 addition & 2 deletions src/System.Private.CoreLib/shared/System/BadImageFormatException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,7 @@ public override string ToString()
if (_fileName != null && _fileName.Length != 0)
s += Environment.NewLine + SR.Format(SR.IO_FileName_Name, _fileName);

if (InnerException != null)
s = s + " ---> " + InnerException.ToString();
s += InnerExceptionToString();

if (StackTrace != null)
s += Environment.NewLine + StackTrace;
Expand Down
7 changes: 0 additions & 7 deletions src/System.Private.CoreLib/shared/System/Diagnostics/StackTrace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,13 +319,6 @@ internal string ToString(TraceFormat traceFormat)
sb.AppendFormat(CultureInfo.InvariantCulture, inFileLineNum, fileName, sf.GetFileLineNumber());
}
}

// Skip EDI boundary for async
if (sf.IsLastFrameFromForeignExceptionStackTrace && !isAsync)
{
sb.Append(Environment.NewLine);
sb.Append(SR.Exception_EndStackTraceFromPreviousThrow);
}
}
}

Expand Down
22 changes: 17 additions & 5 deletions src/System.Private.CoreLib/shared/System/Exception.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

using System.Collections;
using System.Runtime.Serialization;
using System.Text;

namespace System
{
Expand Down Expand Up @@ -149,11 +150,7 @@ public override string ToString()
s += ": " + message;
}

if (_innerException != null)
{
s = s + " ---> " + _innerException.ToString() + Environment.NewLine +
" " + SR.Exception_EndOfInnerExceptionStack;
}
s += InnerExceptionToString();

string? stackTrace = StackTrace;
if (stackTrace != null)
Expand All @@ -164,6 +161,21 @@ public override string ToString()
return s;
}

// Returns the indented inner exception's ToString() prefixed with a newline.
// If there is no inner exception, returns empty string.
protected internal virtual string InnerExceptionToString(Exception? inner = null)
{
inner ??= _innerException;
if (inner == null)
return "";

var sb = new StringBuilder(inner.ToString());
sb.Replace("\r\n", "\n");
sb.Replace("\n", Environment.NewLine + " ");

return Environment.NewLine + " ---> " + sb.ToString();
}

protected event EventHandler<SafeSerializationEventArgs> SerializeObjectState
{
add { throw new PlatformNotSupportedException(SR.PlatformNotSupported_SecureBinarySerialization); }
Expand Down
3 changes: 1 addition & 2 deletions src/System.Private.CoreLib/shared/System/IO/FileLoadException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,7 @@ public override string ToString()
if (FileName != null && FileName.Length != 0)
s += Environment.NewLine + SR.Format(SR.IO_FileName_Name, FileName);

if (InnerException != null)
s = s + " ---> " + InnerException.ToString();
s += InnerExceptionToString();

if (StackTrace != null)
s += Environment.NewLine + StackTrace;
Expand Down
3 changes: 1 addition & 2 deletions src/System.Private.CoreLib/shared/System/IO/FileNotFoundException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,7 @@ public override string ToString()
if (FileName != null && FileName.Length != 0)
s += Environment.NewLine + SR.Format(SR.IO_FileName_Name, FileName);

if (InnerException != null)
s = s + " ---> " + InnerException.ToString();
s += InnerExceptionToString();

if (StackTrace != null)
s += Environment.NewLine + StackTrace;
Expand Down
6 changes: 1 addition & 5 deletions src/System.Private.CoreLib/shared/System/Runtime/InteropServices/COMException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,7 @@ public override string ToString()
s.Append(": ").Append(message);
}

Exception? innerException = InnerException;
if (innerException != null)
{
s.Append(" ---> ").Append(innerException.ToString());
}
s.Append(InnerExceptionToString());

string? stackTrace = StackTrace;
if (stackTrace != null)
Expand Down
6 changes: 1 addition & 5 deletions src/System.Private.CoreLib/shared/System/Runtime/InteropServices/ExternalException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,7 @@ public override string ToString()
s = s + ": " + message;
}

Exception? innerException = InnerException;
if (innerException != null)
{
s = s + " ---> " + innerException.ToString();
}
s += InnerExceptionToString();

if (StackTrace != null)
s += Environment.NewLine + StackTrace;
Expand Down