Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SWEEP: PrintStackTrace cleanup, #932 #1101

Merged
merged 8 commits into from
Jan 18, 2025
Merged

Conversation

paulirwin
Copy link
Contributor

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a change, please open an issue to discuss the change or find an existing issue.

Clean up usages of Exception.StackTrace and consolidate to new PrintStackTrace/PrintCurrentStackTrace methods.

Fixes #932

Description

There were various inconsistent, and sometimes incorrect, translations of .printStackTrace() to .NET. This moves the PrintStackTrace extension method into Support, makes it print any Suppressed exceptions, and updates all usages where Lucene uses printStackTrace on a caught exception to use this new extension method.

In the case where the Lucene code was doing new Throwable().printStackTrace(...) to print the stack trace at the call site, this has been replaced with the new StackTraceHelper.PrintCurrentStackTrace(TextWriter) method. This method skips the frame of itself in the stack trace, and has NoInlining set to prevent inlining which could cause the top stack frame to be missed.

For suppressed exceptions, it will print them below the string output of the exception if there are any. This PR does not change the structure of suppressed exceptions, i.e. to use AggregateException as discussed in #932, as there didn't seem to be a good seam for doing that at this time.

@paulirwin paulirwin added the notes:bug-fix Contains a fix for a bug label Jan 14, 2025
@paulirwin paulirwin requested a review from NightOwl888 January 14, 2025 05:18
@paulirwin paulirwin marked this pull request as ready for review January 14, 2025 05:24
@paulirwin
Copy link
Contributor Author

FYI, tests failing on .NET FX, I'll get that fixed later today.

@paulirwin paulirwin requested a review from NightOwl888 January 18, 2025 18:02
@NightOwl888 NightOwl888 self-requested a review January 18, 2025 20:04
@paulirwin paulirwin merged commit 5374caa into apache:master Jan 18, 2025
267 checks passed
@paulirwin paulirwin deleted the issue/932 branch January 18, 2025 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes:bug-fix Contains a fix for a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix calls to Exception.StackTrace
2 participants