AggregateException.ToString() when wrapped in another exception loses data

[hannasm]

When AggregateException is wrapped in another exception, the ToString() method no longer populates the complete list of exception details.

Full MSTest code:

using Microsoft.VisualStudio.TestTools.UnitTesting;
using System;

namespace ExceptionAggregateTest
{
    [TestClass]
    public class ExceptionHandlingTests
    {
        public Exception[] CollectExceptions() {
          var result = new Exception[2];
          try {
            throw new Exception("Exception 1");
          } catch (Exception eError) {
            result[0] = eError;
          }
          try {
            throw new Exception("Exception 2");
          } catch (Exception eError) {
            result[1] = eError;
          }
          return result;
        }
        public AggregateException CreateAggregate(Exception[] inner) {
          try {
            throw new AggregateException(inner);
          } catch (AggregateException eError) {
            return eError;
          }
        }

        public Exception CreateOuter(AggregateException inner) {
          try {
            throw new Exception("outer", inner);
          } catch (Exception eError) {
            return eError;
          }
        }

        [TestMethod]
        public void Test003() {
          var ex = CreateAggregate(CollectExceptions());
          Console.Error.Write(ex.ToString());
          Assert.Fail();
        }
        [TestMethod]
        public void Test004() {
          var ex = CreateOuter(CreateAggregate(CollectExceptions()));
          Console.Error.Write(ex.ToString());
          Assert.Fail();
        }
    }
}

Test003 (which does not wrap AggregateException) displays:

System.AggregateException: One or more errors occurred. (Exception 1) (Exception 2) ---> System.Exception: Exception 1
at ExceptionAggregateTest.ExceptionHandlingTests.CollectExceptions() in /home/hannasm/tmp_exc_test/ExceptionHandlingTests.cs:line 16
--- End of inner exception stack trace ---
at ExceptionAggregateTest.ExceptionHandlingTests.CreateAggregate(Exception[] inner) in /home/hannasm/tmp_exc_test/ExceptionHandlingTests.cs:line 29
---> (Inner Exception #0) System.Exception: Exception 1
at ExceptionAggregateTest.ExceptionHandlingTests.CollectExceptions() in /home/hannasm/tmp_exc_test/ExceptionHandlingTests.cs:line 16<---

---> (Inner Exception dotnet/corefx#1) System.Exception: Exception 2
at ExceptionAggregateTest.ExceptionHandlingTests.CollectExceptions() in /home/hannasm/tmp_exc_test/ExceptionHandlingTests.cs:line 21<---

Test004 however, (which wraps AggregateException in an Exception) is missing the details on "Exception 2"

System.Exception: outer ---> System.AggregateException: One or more errors occurred. (Exception 1) (Exception 2) ---> System.Exception: Exception 1
at ExceptionAggregateTest.ExceptionHandlingTests.CollectExceptions() in /home/hannasm/tmp_exc_test/ExceptionHandlingTests.cs:line 16
--- End of inner exception stack trace ---
at ExceptionAggregateTest.ExceptionHandlingTests.CreateAggregate(Exception[] inner) in /home/hannasm/tmp_exc_test/ExceptionHandlingTests.cs:line 29
--- End of inner exception stack trace ---
at ExceptionAggregateTest.ExceptionHandlingTests.CreateOuter(AggregateException inner) in /home/hannasm/tmp_exc_test/ExceptionHandlingTests.cs:line 37

I would like to see the output from Test004 include all the same stack trace details that are included in Test003. (Since there is a slightly different chain of exceptions it will be slightly different output)

This stack overflow post explains the issue pretty well: https://stackoverflow.com/a/34433686/68042

Also, it looks like mstest from my console is doing something separately weird with how it displays aggregate exceptions. Any tips on where to open a ticket about that?

[joshfree]

@hannasm I believe you can open MSTest questions and issues @ https://github.com/Microsoft/testfx

[danmoseley]

As described in the SO link,

The problem is that the ToString() on Exception will not call the ToString() of the innerException, but the private ToString(bool,bool) method on the exception class itself.
So the overridden ToString() method of the AggregateException is not called.

This is a problem with Exception itself. The same "information loss" occurs whenever the inner exception writes something in its ToString() that isn't included in its Message (or inner exceptions).

Eg.,

            var fle = new FileLoadException("<msg>", "<filename>");
            Console.WriteLine(fle.ToString()) ;
            Console.WriteLine("####");
            var ex = new Exception("<exmsg>", fle);
            Console.WriteLine(ex.ToString());

gives

System.IO.FileLoadException: <msg>
File name: '<filename>'
####
System.Exception: <exmsg> ---> System.IO.FileLoadException: <msg>
   --- End of inner exception stack trace ---

Here you lost the filename (although it's likely not noticed in this case as the default message contains it). On SqlException, the ToString() has a lot more information that's not in the message, and one would expect it to not be lost.

[danmoseley]

On Exception we have

        public override string ToString()
        {
            return ToString(true, true);
        }

        private string ToString(bool needFileLineInfo, bool needMessage)
        { ... }

       // CoreCLR only
        // InternalToString is called by the runtime to get the exception text.
        internal string InternalToString()
        {
            // Get the current stack trace string. 
            return ToString(true, true);
        }

I guess the runtime uses InternalToString() to bypass any overridden implementations of ToString() that might not be so useful. I am not sure of why (@jkotas?) but it looks like some reasons include Watson reports and asserts. (The booleans seem to exist because when AppX::IsAppXProcess() the runtime wants a different format.)

But when someone calls ToString() we know they are not the runtime, and they presumably expect ToString() on the inner exceptions as well. (I'm assuming we don't think the super duper ToString() needs to be exposed to customers - since we only exposed it indirectly via calling through a wrapper exception)

One way to fix this would be to add another parameter useBaseToString that is set to true only when called by InternalToString() to indicate it wants it special behavior, and false when called by regular ToString()

[danmoseley]

cc @stephentoub

Having looked at so many test failure callstacks in logs, I am pretty motivated to fix any case I found where the output is incomplete..

[jkotas]

I am not sure of why

The current shape of this code was influenced by code-access-security (e.g. the overriden ToString could have been malicious), some kind of Watson feature (it was done for Silverlight - I am not sure whether it is still relevant) , and bug-for-bug .NET Framework compatibility bar.

One way to fix this would be to add another parameter

I would rather look at whether we can fix this by deleting code.

[danmoseley]

Makes sense.

Looking at places that call through METHOD__EXCEPTION__INTERNAL_TO_STRING, they are all likely not concerned whether it calls ToString() directly instead.

The one htat seems watson related is
https://github.com/dotnet/coreclr/blob/58d9cf157f54e8fd61eaaf56b3f8045075d171cd/src/classlibnative/bcltype/system.cpp#L464

@leculver how do does Watson rely on the exception text (the result of ToString() or in this case InternalToString() on Exception)? Right now it uses the latter, which is oblivious to ToString() overrides, possibly a good thing if you simply must get the callstack that way and the ToString() hypothetically does not return it, but otherwise not necessarily a good thing as it can miss some interesting information.

[leculver]

This is all if I remember correctly... The extra failfast argument here with an exception message was for Silverlight (and maybe phone?) so that customers could get this message directly/more easily.

For desktop CLR I don't think this entire codepath exists, and bucketing seems fine without it. also for desktop, Watson generally inspects the shape of exception objects through functions like these.

Right now, Watson is fairly broken on .Net Core for a variety of reasons. I don't think anyone in the Watson space is going to notice if we strip out all of this code around adding an extra bucket parameter for .Net Core. It's on my TODO list to make .Net Core Watson work better, and using this text string wouldn't be a part of that anyway.

So in short: +1 to Jan's recommendation of "can we just strip out most of this code and make things better?"

[danmoseley]

As noted in the linked issue, the same issue exists with StackTrace. Exception.ToString() ignores any override of that property and instead calls the base implementation (GetStackTrace). I think that needs the same fix.