Skip to content

Capture Edi at Abort not throw #11875

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jul 19, 2019
Merged

Capture Edi at Abort not throw #11875

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 6 additions & 3 deletions src/Servers/Kestrel/Core/src/Internal/Http/HttpRequestPipeReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ internal sealed class HttpRequestPipeReader : PipeReader
{
private MessageBody _body;
private HttpStreamState _state;
private Exception _error;
private ExceptionDispatchInfo _error;

public HttpRequestPipeReader()
{
Expand Down Expand Up @@ -99,7 +99,10 @@ public void Abort(Exception error = null)
if (_state != HttpStreamState.Closed)
{
_state = HttpStreamState.Aborted;
_error = error;
if (error != null)
{
_error = ExceptionDispatchInfo.Capture(error);
Copy link
Member

@halter73 halter73 Jul 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I approved it since it feels more standard/correct, but is there an actual functional impact to this?

For example, if the same exception instance gets rethrown somewhere else after we "capture" it here, will it save us from those super-long stack traces being built up from the multiple throwers of the same exception?

Edit: I just saw "So it doesn't add to the exception on every throw" in the PR description. I think I get it now. Was it the multiple calls to ExceptionDispatchInfo.Capture() itself that created the long stack traces, not the throwing of the exception multiple times?

Copy link
Member Author

Choose a reason for hiding this comment

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

The exception is mutable; so each .Capture(error) adds another stack to the original exception; combined with the .Capture(error).Throw() it means each time ValidateState is called and throws it will add anew stack to the exception so each Throw will get a longer and longer stack trace (if it happens multiple times with the same exception).

Changing this to only capture once; means even if it throws multiple times the stack won't grow and grow (concurrent throws aside).

Was it the multiple calls to ExceptionDispatchInfo.Capture() itself that created the long stack traces, not the throwing of the exception multiple times?

This would do that; however... I don't see HttpRequestPipeReader in the stacks, so I'm not sure its the cause?

Copy link
Member Author

Choose a reason for hiding this comment

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

Putting it another way, this is a definite issue with what its doing here and a fix

Not sure about the other though; perhaps if the same exception is getting Edi captured in two or more places #11827 then the stack could get wacky?

Copy link
Member Author

Choose a reason for hiding this comment

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

e.g.

interface IThrow { void Throw(ExceptionDispatchInfo edi, Exception ex); }

struct EdiThrow : IThrow
{
    public void Throw(ExceptionDispatchInfo edi, Exception ex)
    {
        edi.Throw();
    }
}

struct EdiCaptureThrow : IThrow
{
    public void Throw(ExceptionDispatchInfo edi, Exception ex)
    {
        ExceptionDispatchInfo.Capture(ex).Throw();
    }
}

class Program
{
    static ExceptionDispatchInfo s_edi;
    static Exception s_e;

    static void Main(string[] args)
    {
        Exception exception = null;

        SetException();
        for (var i = 0; i < 4; i++)
        {
            exception = Throw(new EdiThrow());
        }

        Console.WriteLine("EdiThrow");
        Console.WriteLine(exception.StackTrace);
        Console.WriteLine();

        SetException();
        for (var i = 0; i < 4; i++)
        {
            exception = Throw(new EdiCaptureThrow());
        }

        Console.WriteLine("EdiCaptureThrow");
        Console.WriteLine(exception.StackTrace);
    }



    static void SetException()
    {
        try
        {
            throw new Exception("source");
        }
        catch (Exception ex)
        {
            s_e = ex;
            s_edi = ExceptionDispatchInfo.Capture(ex);
        }
    }

    static Exception Throw<TThrow>(TThrow thrower) where TThrow : IThrow
    {
        Exception exception = null;
        try { thrower.Throw(s_edi, s_e); }
        catch (Exception ex) { exception = ex; }
        return exception;
    }
}
EdiThrow
   at Program.SetException() in C:\Work\test\Exceptions\Program.cs:line 60
--- End of stack trace from previous location where exception was thrown ---
   at EdiThrow.Throw(ExceptionDispatchInfo edi, Exception ex) in C:\Work\test\Exceptions\Program.cs:line 13
   at Program.Throw[TThrow](TThrow thrower) in C:\Work\test\Exceptions\Program.cs:line 72

EdiCaptureThrow
   at Program.SetException() in C:\Work\test\Exceptions\Program.cs:line 60
--- End of stack trace from previous location where exception was thrown ---
   at EdiCaptureThrow.Throw(ExceptionDispatchInfo edi, Exception ex) in C:\Work\test\Exceptions\Program.cs:line 21
   at Program.Throw[TThrow](TThrow thrower) in C:\Work\test\Exceptions\Program.cs:line 72
--- End of stack trace from previous location where exception was thrown ---
   at EdiCaptureThrow.Throw(ExceptionDispatchInfo edi, Exception ex) in C:\Work\test\Exceptions\Program.cs:line 21
   at Program.Throw[TThrow](TThrow thrower) in C:\Work\test\Exceptions\Program.cs:line 72
--- End of stack trace from previous location where exception was thrown ---
   at EdiCaptureThrow.Throw(ExceptionDispatchInfo edi, Exception ex) in C:\Work\test\Exceptions\Program.cs:line 21
   at Program.Throw[TThrow](TThrow thrower) in C:\Work\test\Exceptions\Program.cs:line 72
--- End of stack trace from previous location where exception was thrown ---
   at EdiCaptureThrow.Throw(ExceptionDispatchInfo edi, Exception ex) in C:\Work\test\Exceptions\Program.cs:line 21
   at Program.Throw[TThrow](TThrow thrower) in C:\Work\test\Exceptions\Program.cs:line 72

}
}
}

Expand All @@ -119,7 +122,7 @@ private void ValidateState(CancellationToken cancellationToken = default)
{
if (_error != null)
{
ExceptionDispatchInfo.Capture(_error).Throw();
_error.Throw();
}
else
{
Expand Down