Skip to content

Preserve exception stack traces #248

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

Closed
wants to merge 1 commit into from

Conversation

0xced
Copy link

@0xced 0xced commented Mar 16, 2022

@MattB-msft
Copy link
Member

@0xced Is there some reason you feel that is necessary? We did that intentionally to limit the stack trace that was exported.
is there something your trying to do with the dump?

@0xced
Copy link
Author

0xced commented Mar 18, 2022

We did that intentionally to limit the stack trace that was exported.

This seems weird to me. Losing some part of the original stack trace will just make it harder to diagnose issues. Many analyzers suggest to rethrow exceptions with throw; instead of throw ex;.

  • .NET source code analysis: CA2200
  • Meziantou.Analyzer: MA0027
  • ExceptionAnalyzer: EA004

The best practices for exceptions also recommends the same:

private static void TransferFunds(Account from, Account to, decimal amount)
{
    string withdrawalTrxID = from.Withdrawal(amount);
    try
    {
        to.Deposit(amount);
    }
    catch
    {
        from.RollbackTransaction(withdrawalTrxID);
        throw;
    }
}

This example illustrates the use of throw to re-throw the original exception, which can make it easier for callers to see the real cause of the problem without having to examine the InnerException property.

@0xced
Copy link
Author

0xced commented Sep 8, 2022

Would you mind reconsidering this pull request?

@MattB-msft
Copy link
Member

@0xced , We have decided to go back though and relax a few places where we limit the stack, the areas you highlighted here are some of those areas we are refactoring to pass the source though.

We will close this PR when we ship this update.

MattB-msft added a commit that referenced this pull request Nov 4, 2022
Fix by Git user matkov accepted for crash during create when an invalid timespan is passed from a configuration file to the ServiceClient Constructor. See #326 for details.
Fix by GIT user 0xced accepted for bug in Integrated user security setting,  Code will now properly honor integrated security when attempting to authenticate. See #324 for details.
Fixed a number of places where Async Calls did not have a .ConfigureAwait specification. Thanks to user 0xced for prompting this fix.
Fixed a misleading warning message "Client ID not supplied, using SDK Sample Client ID for this connection" that would be incorrectly shown when ExternalAuthentication type was used. Fixes #314, Thanks for your report!
Fixed a possible thread contention issue when managing connection context across multiple concurrent threads in the same client.  Git issue: #304 Thanks for your report !
Fixed a memory leak when caused by a logger not being cleaned up when a the creation of the ServiceClient fails. Git issue: #322
Fixed issue where calling the IOrganizationService* interfaces after the client has been disposed would throw a NullReferenceException. Taking this action will now throw an ObjectDisposedException
Removed a set of exception catch and rethrows to preserve the full callstack to caller. Thanks to Git user 0xceed for prompting ( and sticking with getting us to ) this fix. See: #248 for details.
@0xced 0xced deleted the fix-exception-stack-traces branch March 30, 2023 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants