Fix calls to Exception.StackTrace

[paulirwin]

new Exception().StackTrace is an incorrect translation from Java. We should find all calls to Exception.StackTrace as well as the StackTrace class and convert them correctly. From ChatGPT:

In Java, the code new Exception().getStackTrace() creates a new instance of the Exception class, and then calls the getStackTrace() method on that instance to obtain an array of StackTraceElement objects. Each element in the array represents a stack trace element, providing information about the call stack at the point where the exception was created.

Here's an example of how it might be used:

StackTraceElement[] trace = new Exception().getStackTrace();

for (StackTraceElement element : trace) {
    System.out.println(element.getClassName() + " - " + element.getMethodName() + " - " + element.getLineNumber());
}

This code prints out the class name, method name, and line number for each element in the stack trace.

In C#, the equivalent code would use the StackTrace class from the System.Diagnostics namespace. Here's an example:

using System;
using System.Diagnostics;

class Program
{
    static void Main()
    {
        StackTrace trace = new StackTrace();
        StackFrame[] frames = trace.GetFrames();

        foreach (StackFrame frame in frames)
        {
            Console.WriteLine($"{frame.GetMethod().DeclaringType} - {frame.GetMethod().Name} - {frame.GetFileLineNumber()}");
        }
    }
}

This C# code creates a new StackTrace instance and then uses the GetFrames() method to obtain an array of StackFrame objects. Similar to the Java example, it prints out the declaring type, method name, and line number for each frame in the stack trace.

We have a class called StackTraceHelper that we could add the support to convert it to a string. There are also several calls to .printStackTace() that should be reviewed. In .NET, Exception.StackTrace doesn't contain the exception type, so a lot of the tests use Exception.ToString() instead. But we would be better off with a centralized way of dealing with stack traces (in Support) - StackTraceHelper only applies to the test code, but there is code in production that is also not correctly translated. Of course, since it is something we own, moving it to Support is an option.

In short, we want to review all of the code that was using .getStackTrace() or .printStackTrace() in Lucene.

Originally posted by @NightOwl888 in #926 (comment)

[paulirwin]

I reviewed this yesterday, and in addition to the translation errors in the original issue, there is highly inconsistent translation of printStackTrace. I'm going to do a sweep of all usages and have it use a common extension method so that it is translated correctly.

As @NightOwl888 noted:

In .NET, Exception.StackTrace doesn't contain the exception type, so a lot of the tests use Exception.ToString() instead.

In Java, printStackTrace includes the exception type and message before printing the stack trace, so this is effectively the same as printing e.ToString(). There were several cases where this was omitted, or translated into two Console.WriteLine calls, which just begs to have future translation issues (or set bad precedence).

To solve this, I'm moving the TestFramework's printStackTrace extension method overloads into Support's ExceptionExtensions, fixing the capitalization of them to match .NET conventions, and having them print an output that is equivalent to Java's. This will let us translate e.printStackTrace() to e.PrintStackTrace() (or e.printStackTrace(System.out) to e.PrintStackTrace(Console.Out)) which will feel like a very natural and foolproof translation of the code. These methods are set to AggressiveInlining so they should not harm performance. (Most of the usage is in test or console code, anyways.)

Additionally, I'm moving the StackTraceHelper type into Support as noted in the original ticket, with a new method PrintCurrentStackTrace that does what the original issue needs. It uses new StackTrace(skipFrames: 1) with NoInlining so that the PrintCurrentStackTrace method is omitted from the printed stack trace, while NoInlining ensures that we don't miss any frames by having this inlined and then skipping the calling method frame.

[NightOwl888]

One thing of note is that randomizedtesting injects the random seed and other debugging details into the stack trace. I haven't looked into what the most reasonable way to do that would be. But, it might be useful to have a way to accomplish that sort of thing by the test framework since we are not planning on porting randomizedtesting for the release. I think that it would be more useful if we had that injected into the live debugging info rather than just the output from PrintStackTrace(), but I haven't looked into the feasibility of the former and we might have to rely on the latter.

[NightOwl888]

Oh, one more thing, sort of related. There is an AddSuppressed() extension method on Exception, as well. It sort of works like AggregateExcpetion except that it keeps the primary exception on the outside and stores a list of any other exceptions that were thrown in a list in Exeption.Data. Certainly, it would make sense to have those extra exceptions in the stack trace.

In hindsight, I wonder if it would make more sense to use AggregateException. It didn't seem possible before we changed the catch blocks to use extension methods to identify exception types, but now that we are mostly doing that everywhere, it seems like we could just use a standard convention, such as considering the first Exception in the collection as the primary one. It could save some effort with having to work out how to display a stack trace for all of these nested exceptions since AggregateException has a pretty reasonable way to do that already.

Although, we never worked out why the Thread class in Lucene seems to propagate the exception to the calling thread. The javadoc says it doesn't do that by default, although I found that there is a way to configure it to act that way. But, if Lucene has that extra configuration somewhere, I couldn't locate it. ThreadJob simply re-throws an exception that happened on the background thread when Join() is called. It seems to work that way. But it is possible that there is a path forward where AggregateException makes sense rather than just throwing the exception for the first call to Join().

[paulirwin]

Oh, one more thing, sort of related. There is an AddSuppressed() extension method on Exception, as well. It sort of works like AggregateExcpetion except that it keeps the primary exception on the outside and stores a list of any other exceptions that were thrown in a list in Exeption.Data. Certainly, it would make sense to have those extra exceptions in the stack trace.

In hindsight, I wonder if it would make more sense to use AggregateException. It didn't seem possible before we changed the catch blocks to use extension methods to identify exception types, but now that we are mostly doing that everywhere, it seems like we could just use a standard convention, such as considering the first Exception in the collection as the primary one. It could save some effort with having to work out how to display a stack trace for all of these nested exceptions since AggregateException has a pretty reasonable way to do that already.

Although, we never worked out why the Thread class in Lucene seems to propagate the exception to the calling thread. The javadoc says it doesn't do that by default, although I found that there is a way to configure it to act that way. But, if Lucene has that extra configuration somewhere, I couldn't locate it. ThreadJob simply re-throws an exception that happened on the background thread when Join() is called. It seems to work that way. But it is possible that there is a path forward where AggregateException makes sense rather than just throwing the exception for the first call to Join().

This seems like something that could be broken out into its own issue.

[NightOwl888]

Oh, one more thing, sort of related. There is an AddSuppressed() extension method on Exception, as well. It sort of works like AggregateExcpetion except that it keeps the primary exception on the outside and stores a list of any other exceptions that were thrown in a list in Exeption.Data. Certainly, it would make sense to have those extra exceptions in the stack trace.
In hindsight, I wonder if it would make more sense to use AggregateException. It didn't seem possible before we changed the catch blocks to use extension methods to identify exception types, but now that we are mostly doing that everywhere, it seems like we could just use a standard convention, such as considering the first Exception in the collection as the primary one. It could save some effort with having to work out how to display a stack trace for all of these nested exceptions since AggregateException has a pretty reasonable way to do that already.
Although, we never worked out why the Thread class in Lucene seems to propagate the exception to the calling thread. The javadoc says it doesn't do that by default, although I found that there is a way to configure it to act that way. But, if Lucene has that extra configuration somewhere, I couldn't locate it. ThreadJob simply re-throws an exception that happened on the background thread when Join() is called. It seems to work that way. But it is possible that there is a path forward where AggregateException makes sense rather than just throwing the exception for the first call to Join().

This seems like something that could be broken out into its own issue.

Yeah, probably. But it is related to printing out the stack trace, so I thought I would mention it here. Even if it means doing it separately and retrofitting the stack trace printing, as required.

Currently, the AddSuppressed() method just puts those exceptions into a void. They are available for debugging, but only if you know to look for them in Exception.Data. .NET doesn't add them to the stack trace of the outer exception. For the time being, since we don't use AggregateException it would be helpful if the PrintStackTrace() method included the stack traces of the inner exceptions from GetSuppressedAsList().

[paulirwin]

Ah I see what you mean, makes sense. I'll see if it is coherent to include that in this PR or break it out separately.