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 into from
Jul 26, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a32a71
Do not expand stacktraces when completion exception is rethrown multi…
pakrym Jul 25, 2018
1c0a6f3
fb
pakrym Jul 25, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions src/System.IO.Pipelines/src/System/IO/Pipelines/PipeCompletion.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,25 @@ namespace System.IO.Pipelines
internal struct PipeCompletion
{
private static readonly ArrayPool<PipeCompletionCallback> s_completionCallbackPool = ArrayPool<PipeCompletionCallback>.Shared;
private static readonly Exception s_completedNoException = new Exception();

private const int InitialCallbacksSize = 1;

private Exception _exception;
private bool _isCompleted;
private ExceptionDispatchInfo _exceptionInfo;

private PipeCompletionCallback[] _callbacks;
private int _callbackCount;

public bool IsCompleted => _exception != null;
public bool IsCompleted => _isCompleted;

public bool IsFaulted => IsCompleted && _exception != s_completedNoException;
public bool IsFaulted => _exceptionInfo != null;

public PipeCompletionCallbacks TryComplete(Exception exception = null)
{
if (_exception == null)
_isCompleted = true;
if (exception != null)
{
// Set the exception object to the exception passed in or a sentinel value
_exception = exception ?? s_completedNoException;
_exceptionInfo = ExceptionDispatchInfo.Capture(exception);
}
return GetCallbacks();
}
Expand Down Expand Up @@ -68,12 +68,12 @@ public PipeCompletionCallbacks AddCallback(Action<Exception, object> callback, o
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public bool IsCompletedOrThrow()
{
if (_exception == null)
if (!_isCompleted)
{
return false;
}

if (_exception != s_completedNoException)
Copy link

@ahsonkhan ahsonkhan Jul 25, 2018

Choose a reason for hiding this comment

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

Remove s_completedNoException now that it is no longer used.
https://github.com/dotnet/corefx/pull/31375/files#diff-ff9ac9c14f9484dcfcaa2332b6353541L16

Copy link
Author

@pakrym pakrym Jul 25, 2018

Choose a reason for hiding this comment

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

Done

if (_exceptionInfo != null)
{
ThrowLatchedException();
}
Expand All @@ -91,7 +91,7 @@ private PipeCompletionCallbacks GetCallbacks()

var callbacks = new PipeCompletionCallbacks(s_completionCallbackPool,
_callbackCount,
_exception == s_completedNoException ? null : _exception,
_exceptionInfo?.SourceException,
_callbacks);

_callbacks = null;
Expand All @@ -103,13 +103,14 @@ public void Reset()
{
Debug.Assert(IsCompleted);
Debug.Assert(_callbacks == null);
_exception = null;
_isCompleted = false;
_exceptionInfo = null;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private void ThrowLatchedException()
{
ExceptionDispatchInfo.Capture(_exception).Throw();
_exceptionInfo.Throw();
}

public override string ToString()
Expand Down
5 changes: 5 additions & 0 deletions src/System.IO.Pipelines/tests/PipeReaderWriterFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Text;
using System.Text.RegularExpressions;
using System.Threading;
using System.Threading.Tasks;
using Xunit;
Expand Down Expand Up @@ -238,6 +239,8 @@ void ThrowTestException()
invalidOperationException = await Assert.ThrowsAsync<InvalidOperationException>(async () => await _pipe.Writer.FlushAsync());
Assert.Equal("Reader exception", invalidOperationException.Message);
Assert.Contains("ThrowTestException", invalidOperationException.StackTrace);

Assert.Single(Regex.Matches(invalidOperationException.StackTrace, "Pipe.GetFlushResult"));
}

[Fact]
Expand Down Expand Up @@ -304,6 +307,8 @@ void ThrowTestException()
invalidOperationException = await Assert.ThrowsAsync<InvalidOperationException>(async () => await _pipe.Reader.ReadAsync());
Assert.Equal("Writer exception", invalidOperationException.Message);
Assert.Contains("ThrowTestException", invalidOperationException.StackTrace);

Assert.Single(Regex.Matches(invalidOperationException.StackTrace, "Pipe.GetReadResult"));
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.

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

@pakrym pakrym Jul 26, 2018

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 •
edited
Loading

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

@pakrym pakrym Jul 26, 2018

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

@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.

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.

}

[Fact]
Expand Down