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

Do not expand stacktraces when completion exception is rethrown multiple times#31375

Merged
pakrym merged 2 commits intodotnet:masterfrom
pakrym:pakrym/short-stacks
Jul 26, 2018
Merged

Do not expand stacktraces when completion exception is rethrown multiple times#31375
pakrym merged 2 commits intodotnet:masterfrom
pakrym:pakrym/short-stacks

Conversation

@pakrym
Copy link

@pakrym pakrym commented Jul 25, 2018

Fixes #31388

Because of incorrect usage of ExceptionDispatchInfo exception stack trace is expanded every time it's rethrown https://github.com/dotnet/corefx/issues/31371

@pakrym pakrym requested review from ahsonkhan and halter73 July 25, 2018 22:34
return false;
}

if (_exception != s_completedNoException)

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Done

@pakrym pakrym force-pushed the pakrym/short-stacks branch from 18dbabc to 0a32a71 Compare July 25, 2018 22:43
public bool IsCompleted => _isCompleted;

public bool IsFaulted => IsCompleted && _exception != s_completedNoException;
public bool IsFaulted => IsCompleted && _exception != null;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The IsCompleted check now redundant since _exception can't be non-null if not completed.


private Exception _exception;
private bool _isCompleted;
private ExceptionDispatchInfo _exception;
Copy link
Member

Choose a reason for hiding this comment

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

Nit _exceptionInfo

Assert.Equal("Writer exception", invalidOperationException.Message);
Assert.Contains("ThrowTestException", invalidOperationException.StackTrace);

Assert.Single(Regex.Matches(invalidOperationException.StackTrace, "Pipe.GetReadResult"));
Copy link
Member

Choose a reason for hiding this comment

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

Do you also want a test that if the same source exception is passed into multiple pipes it only returns a single entry? aspnet/KestrelHttpServer#2730 (comment)

Copy link
Author

Choose a reason for hiding this comment

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

But we can't guarantee that. Different instances of EDI would still mutate the same exception object.

Copy link
Member

@benaadams benaadams Jul 26, 2018

Choose a reason for hiding this comment

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

void Main()
{
    Exception exception;
    try
    {
        throw new Exception();
    }
    catch (Exception ex)
    {
        exception = ex;
    }

    var edi1 = ExceptionDispatchInfo.Capture(exception);
    var edi2 = ExceptionDispatchInfo.Capture(exception);

    try
    {
        edi1.Throw();
    }
    catch (Exception ex)
    {
        Console.WriteLine("***");
        Console.WriteLine(ex.ToString());
    }
    try
    {
        edi1.Throw();
    }
    catch (Exception ex)
    {
        Console.WriteLine("***");
        Console.WriteLine(ex.ToString());
    }

    try
    {
        edi2.Throw();
    }
    catch (Exception ex)
    {
        Console.WriteLine("***");
        Console.WriteLine(ex.ToString());
    }
    try
    {
        edi2.Throw();
    }
    catch (Exception ex)
    {
        Console.WriteLine("***");
        Console.WriteLine(ex.ToString());
    }
}

Doesn't seem to?

***
System.Exception: Exception of type 'System.Exception' was thrown.
   at UserQuery.Main() in C:\Users\thund\AppData\Local\Temp\LINQPad5\_jffesacy\query_ibgjui.cs:line 36
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at UserQuery.Main() in C:\Users\thund\AppData\Local\Temp\LINQPad5\_jffesacy\query_ibgjui.cs:line 48
***
System.Exception: Exception of type 'System.Exception' was thrown.
   at UserQuery.Main() in C:\Users\thund\AppData\Local\Temp\LINQPad5\_jffesacy\query_ibgjui.cs:line 36
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at UserQuery.Main() in C:\Users\thund\AppData\Local\Temp\LINQPad5\_jffesacy\query_ibgjui.cs:line 57
***
System.Exception: Exception of type 'System.Exception' was thrown.
   at UserQuery.Main() in C:\Users\thund\AppData\Local\Temp\LINQPad5\_jffesacy\query_ibgjui.cs:line 36
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at UserQuery.Main() in C:\Users\thund\AppData\Local\Temp\LINQPad5\_jffesacy\query_ibgjui.cs:line 67
***
System.Exception: Exception of type 'System.Exception' was thrown.
   at UserQuery.Main() in C:\Users\thund\AppData\Local\Temp\LINQPad5\_jffesacy\query_ibgjui.cs:line 36
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at UserQuery.Main() in C:\Users\thund\AppData\Local\Temp\LINQPad5\_jffesacy\query_ibgjui.cs:line 76

Copy link
Author

Choose a reason for hiding this comment

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


{
    Exception exception;
    try
    {
        throw new Exception();
    }
    catch (Exception ex)
    {
        exception = ex;
    }

    var edi1 = ExceptionDispatchInfo.Capture(exception);

    try
    {
        edi1.Throw();
    }
    catch (Exception ex)
    {
        Console.WriteLine("***");
        Console.WriteLine(ex.ToString());
    }

    var edi2 = ExceptionDispatchInfo.Capture(exception);
    try
    {
        edi2.Throw();
    }
    catch (Exception ex)
    {
        Console.WriteLine("***");
        Console.WriteLine(ex.ToString());
    }
}

Yes it does:

***
System.Exception: Exception of type 'System.Exception' was thrown.
   at UserQuery.RunUserAuthoredQuery() in C:\Users\pakrymet\AppData\Local\Temp\LINQPad5\_clqswnhi\query_ztirrw.cs:line 39
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at UserQuery.RunUserAuthoredQuery() in C:\Users\pakrymet\AppData\Local\Temp\LINQPad5\_clqswnhi\query_ztirrw.cs:line 50
***
System.Exception: Exception of type 'System.Exception' was thrown.
   at UserQuery.RunUserAuthoredQuery() in C:\Users\pakrymet\AppData\Local\Temp\LINQPad5\_clqswnhi\query_ztirrw.cs:line 39
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at UserQuery.RunUserAuthoredQuery() in C:\Users\pakrymet\AppData\Local\Temp\LINQPad5\_clqswnhi\query_ztirrw.cs:line 50
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at UserQuery.RunUserAuthoredQuery() in C:\Users\pakrymet\AppData\Local\Temp\LINQPad5\_clqswnhi\query_ztirrw.cs:line 61


Copy link
Member

Choose a reason for hiding this comment

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

So Throw isn't threadsafe as it uses the same exception object

Exception exception;
Exception exception1 = null;
Exception exception2 = null;
try
{
    throw new Exception();
}
catch (Exception ex)
{
    exception = ex;
}

var edi1 = ExceptionDispatchInfo.Capture(exception);

try
{
    edi1.Throw();
}
catch (Exception ex)
{
    Console.WriteLine("***");
    exception1 = ex;
    Console.WriteLine(ex.ToString());
}

var edi2 = ExceptionDispatchInfo.Capture(exception);
try
{
    edi2.Throw();
}
catch (Exception ex)
{
    Console.WriteLine("***");
    exception2 = ex;
    Console.WriteLine(ex.ToString());
}

Console.WriteLine("***");
Console.WriteLine(ReferenceEquals(exception, exception1));
Console.WriteLine(ReferenceEquals(exception, exception2));
***
System.Exception: Exception of type 'System.Exception' was thrown.
   at UserQuery.Main() in C:\Users\thund\AppData\Local\Temp\LINQPad5\_jffesacy\query_tugyst.cs:line 38
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at UserQuery.Main() in C:\Users\thund\AppData\Local\Temp\LINQPad5\_jffesacy\query_tugyst.cs:line 49
***
System.Exception: Exception of type 'System.Exception' was thrown.
   at UserQuery.Main() in C:\Users\thund\AppData\Local\Temp\LINQPad5\_jffesacy\query_tugyst.cs:line 38
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at UserQuery.Main() in C:\Users\thund\AppData\Local\Temp\LINQPad5\_jffesacy\query_tugyst.cs:line 49
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at UserQuery.Main() in C:\Users\thund\AppData\Local\Temp\LINQPad5\_jffesacy\query_tugyst.cs:line 61
***
True
True

So it looks like its equivalent to throw ex; but it keeps the StackTrace at capture and appends from there; whereas throw ex would completely overwrite.

@pakrym
Copy link
Author

pakrym commented Jul 26, 2018

@ahsonkhan ping

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants