Skip to content

Don't reuse the same exception through multiple paths #11827

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

Closed
wants to merge 5 commits into from
Closed

Don't reuse the same exception through multiple paths #11827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c00ce1b
Don't reuse the same exception through multiple paths
benaadams Jul 3, 2019
a4bd4c4
React tests
benaadams Jul 3, 2019
df804e0
Differentiate client and server aborts
benaadams Jul 3, 2019
ff2673a
Capture ExceptionDispatchInfo at Abort not throw
benaadams Jul 3, 2019
cc5db71
Code check
benaadams Jul 3, 2019
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
1 change: 1 addition & 0 deletions ...nections.Abstractions/ref/Microsoft.AspNetCore.Connections.Abstractions.netstandard2.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ public partial class ConnectionAbortedException : System.OperationCanceledExcept
public ConnectionAbortedException() { }
public ConnectionAbortedException(string message) { }
public ConnectionAbortedException(string message, System.Exception inner) { }
public ConnectionAbortedException(string message, System.Exception inner, System.Threading.CancellationToken cancellationToken) { }
}
public partial class ConnectionBuilder : Microsoft.AspNetCore.Connections.IConnectionBuilder
{
Expand Down
14 changes: 11 additions & 3 deletions src/Servers/Connections.Abstractions/src/Exceptions/ConnectionAbortedException.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System;
using System.Threading;

namespace Microsoft.AspNetCore.Connections
{
Expand All @@ -10,11 +11,18 @@ public ConnectionAbortedException() :

}

public ConnectionAbortedException(string message) : base(message)
public ConnectionAbortedException(string message)
: base(message)
{
}

public ConnectionAbortedException(string message, Exception inner) : base(message, inner)
public ConnectionAbortedException(string message, Exception inner)
: base(message, inner)
{
}

public ConnectionAbortedException(string message, Exception inner, CancellationToken cancellationToken)
: base(message, inner, cancellationToken)
{
}
}
Expand Down
7 changes: 6 additions & 1 deletion src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,12 @@ protected void AbortRequest()
}
}

protected void PoisonRequestBodyStream(Exception abortReason)
protected void PoisonRequestBodyStream(ConnectionAbortedException abortReason)
{
_bodyControl?.Abort(abortReason);
}

protected void PoisonRequestBodyStream(IOException abortReason)
{
_bodyControl?.Abort(abortReason);
}
Expand Down
10 changes: 7 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,11 @@ public void Abort(Exception error = null)
if (_state != HttpStreamState.Closed)
{
_state = HttpStreamState.Aborted;
_error = error;

if (error != null)
{
_error = ExceptionDispatchInfo.Capture(error);
}
}
}

Expand All @@ -119,7 +123,7 @@ private void ValidateState(CancellationToken cancellationToken = default)
{
if (_error != null)
{
ExceptionDispatchInfo.Capture(_error).Throw();
_error.Throw();
}
else
{
Expand Down
24 changes: 15 additions & 9 deletions src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,13 @@ public void Abort(IOException abortReason)
return;
}

AbortCore(abortReason);
AbortCore();

// Unblock the request body.
PoisonRequestBodyStream(new IOException(abortReason.Message, abortReason));
RequestBodyPipe.Writer.Complete(new IOException(abortReason.Message, abortReason));
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we really need to new up two exceptions here, since there should only be one reader accessing a single HTTP/2 request body at a time.

@Tratcher made a good point that it might be better to have the caller of Abort or ResetAndAbort clone the exceptions if needed. After all, it's the caller that knows if the exception is being used to complete multiple streams. As it is, this seems a little overly defensive, though certainly better than what's currently in master.

Copy link
Member Author

Choose a reason for hiding this comment

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

Except its used by Request PoisonRequestBodyStream and Response Writer.Complete which can happen in parallel?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but it's only a bug if it get's thrown/handled in parallel, right? There's only one reader for each stream who will either observe the exception via the poisoned request body stream OR the poisoned request body pipe.

The reader could only observe both exceptions it swallowed the exception from the pipe first and then tried reading again anyway thereby observing the the exception from the poisoned stream. This wouldn't cause parallel access to the exception though.

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 only obvious place I can see where it will keep adding to the exception stack trace is in HttpRequestPipeReader #11875 but I would have thought that would have shown up in the stack trace?

Http2Connection already creates a new exception for each Http2Stream https://github.com/aspnet/AspNetCore/blob/3f73a0fdcff217d061b994f3bd6528f1cd8caf2e/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs#L279-L282

However the stack trace would suggest either its happening in parallel or its chaining Write+Flush on Write+Flush on Write+Flush without ever waiting for any completion; so an earlier Flush would be active concurrently with a later Write?

Copy link
Member

Choose a reason for hiding this comment

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

I really want to find the root of the problem. The exception set here should only be observed once or maybe repeatedly thrown to one caller if the caller happens swallow the exception. I really doubt that we're adding to the exception stack trace because we're throwing from both the poisoned Stream and PipeReader.

Copy link
Member Author

Choose a reason for hiding this comment

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

Question is, do you think that could be an accurate the stacktrace?

Copy link
Member

Choose a reason for hiding this comment

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

Question is, do you think that could be an accurate the stacktrace?

I hope not, but I haven't ruled it out completely yet.

Copy link
Member

Choose a reason for hiding this comment

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

Once #12081 is merged, TimingPipeFlusher will no longer be maintaining a flush Task queue. Instead ConcurrentQueue will be doing this job.

I think ConcurrentQueue stack traces will be easier to reason about because there is only ever one TCS at a time configured with TaskCreationOptions.RunContinuationsAsynchronously instead of an arbitrarily long linked list of Tasks being awaited.


_inputFlowControl.Abort();
}

protected override void OnErrorAfterResponseStarted()
Expand Down Expand Up @@ -464,22 +470,22 @@ internal void ResetAndAbort(ConnectionAbortedException abortReason, Http2ErrorCo
// Don't block on IO. This never faults.
_ = _http2Output.WriteRstStreamAsync(error);

AbortCore(abortReason);
AbortCore();

// Unblock the request body.
PoisonRequestBodyStream(new ConnectionAbortedException(abortReason.Message, abortReason, abortReason.CancellationToken));
RequestBodyPipe.Writer.Complete(new ConnectionAbortedException(abortReason.Message, abortReason, abortReason.CancellationToken));

_inputFlowControl.Abort();
}

private void AbortCore(Exception abortReason)
private void AbortCore()
{
// Call _http2Output.Stop() prior to poisoning the request body stream or pipe to
// ensure that an app that completes early due to the abort doesn't result in header frames being sent.
_http2Output.Stop();

AbortRequest();

// Unblock the request body.
PoisonRequestBodyStream(abortReason);
RequestBodyPipe.Writer.Complete(abortReason);

_inputFlowControl.Abort();
}

private Pipe CreateRequestBodyPipe(uint windowSize)
Expand Down
14 changes: 11 additions & 3 deletions src/Servers/Kestrel/Core/src/Internal/Infrastructure/BodyControl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.IO;
using System.IO.Pipelines;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Connections;
using Microsoft.AspNetCore.Http.Features;
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;

Expand Down Expand Up @@ -70,10 +71,17 @@ public Task StopAsync()
return _responseWriter.StopAcceptingWritesAsync();
}

public void Abort(Exception error)
public void Abort(IOException error)
{
_requestReader.Abort(error);
_emptyRequestReader.Abort(error);
_requestReader.Abort(new IOException(error.Message, error));
_emptyRequestReader.Abort(new IOException(error.Message, error));
_responseWriter.Abort();
}

public void Abort(ConnectionAbortedException error)
{
_requestReader.Abort(new ConnectionAbortedException(error.Message, error, error.CancellationToken));
_emptyRequestReader.Abort(new ConnectionAbortedException(error.Message, error, error.CancellationToken));
_responseWriter.Abort();
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Servers/Kestrel/Core/src/Internal/Infrastructure/TimingPipeFlusher.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ private async ValueTask<FlushResult> TimeFlushAsyncAwaited(ValueTask<FlushResult
}
catch (OperationCanceledException ex) when (outputAborter != null)
{
outputAborter.Abort(new ConnectionAbortedException(CoreStrings.ConnectionOrStreamAbortedByCancellationToken, ex));
outputAborter.Abort(new ConnectionAbortedException(CoreStrings.ConnectionOrStreamAbortedByCancellationToken, ex, ex.CancellationToken));
}
catch (Exception ex)
{
Expand Down
148 changes: 108 additions & 40 deletions src/Servers/Kestrel/Core/test/BodyControlTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.IO;
using System.IO.Pipelines;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Connections;
using Microsoft.AspNetCore.Http.Features;
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
Expand All @@ -15,94 +17,160 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests
{
public class BodyControlTests
{
private delegate void AbortAction<TException>(BodyControl bodyControl, TException exception) where TException : Exception;

private readonly static AbortAction<ConnectionAbortedException> abortConnectionAbortedException = (bc, e) => bc.Abort(e);
private readonly static AbortAction<IOException> abortIOException = (bc, e) => bc.Abort(e);

[Fact]
public async Task BodyControlThrowAfterAbort_ConnectionAbortedException()
{
var ex = new ConnectionAbortedException("My error");

await BodyControlThrowAfterAbort(ex, abortConnectionAbortedException);
}

[Fact]
public async Task BodyControlThrowAfterAbort()
public async Task BodyControlThrowAfterAbort_IOException()
{
var ex = new IOException("My error");

await BodyControlThrowAfterAbort(ex, abortIOException);
}

private static async Task BodyControlThrowAfterAbort<TException>(TException exception, AbortAction<TException> abortAction)
where TException : Exception
{
var bodyControl = new BodyControl(Mock.Of<IHttpBodyControlFeature>(), Mock.Of<IHttpResponseControl>());
var (request, response, requestPipe, responsePipe) = bodyControl.Start(new MockMessageBody());

var ex = new Exception("My error");
bodyControl.Abort(ex);
abortAction(bodyControl, exception);

await response.WriteAsync(new byte[1], 0, 1);
Assert.Same(ex,
await Assert.ThrowsAsync<Exception>(() => request.ReadAsync(new byte[1], 0, 1)));
Assert.Same(ex,
await Assert.ThrowsAsync<Exception>(async () => await requestPipe.ReadAsync()));
Assert.Same(exception,
(await Assert.ThrowsAsync<TException>(() => request.ReadAsync(new byte[1], 0, 1))).InnerException);
Assert.Same(exception,
(await Assert.ThrowsAsync<TException>(async () => await requestPipe.ReadAsync())).InnerException);
}

[Fact]
public async Task BodyControlThrowOnAbortAfterUpgrade_ConnectionAbortedException()
{
var ex = new ConnectionAbortedException("My error");

await BodyControlThrowOnAbortAfterUpgrade(ex, abortConnectionAbortedException);
}

[Fact]
public async Task BodyControlThrowOnAbortAfterUpgrade()
public async Task BodyControlThrowOnAbortAfterUpgrade_IOException()
{
var ex = new IOException("My error");

await BodyControlThrowOnAbortAfterUpgrade(ex, abortIOException);
}

private async Task BodyControlThrowOnAbortAfterUpgrade<TException>(TException exception, AbortAction<TException> abortAction)
where TException : Exception
{
var bodyControl = new BodyControl(Mock.Of<IHttpBodyControlFeature>(), Mock.Of<IHttpResponseControl>());
var (request, response, requestPipe, responsePipe) = bodyControl.Start(new MockMessageBody(upgradeable: true));

var upgrade = bodyControl.Upgrade();
var ex = new Exception("My error");
bodyControl.Abort(ex);
abortAction(bodyControl, exception);

var writeEx = await Assert.ThrowsAsync<InvalidOperationException>(() => response.WriteAsync(new byte[1], 0, 1));
Assert.Equal(CoreStrings.ResponseStreamWasUpgraded, writeEx.Message);

Assert.Same(ex,
await Assert.ThrowsAsync<Exception>(() => request.ReadAsync(new byte[1], 0, 1)));
Assert.Same(exception,
(await Assert.ThrowsAsync<TException>(() => request.ReadAsync(new byte[1], 0, 1))).InnerException);

Assert.Same(ex,
await Assert.ThrowsAsync<Exception>(() => upgrade.ReadAsync(new byte[1], 0, 1)));
Assert.Same(exception,
(await Assert.ThrowsAsync<TException>(() => upgrade.ReadAsync(new byte[1], 0, 1))).InnerException);

Assert.Same(ex,
await Assert.ThrowsAsync<Exception>(async () => await requestPipe.ReadAsync()));
Assert.Same(exception,
(await Assert.ThrowsAsync<TException>(async () => await requestPipe.ReadAsync())).InnerException);

await upgrade.WriteAsync(new byte[1], 0, 1);
}

[Fact]
public async Task BodyControlThrowOnUpgradeAfterAbort()
public async Task BodyControlThrowOnUpgradeAfterAbort_ConnectionAbortedException()
{
var ex = new ConnectionAbortedException("My error");

await BodyControlThrowOnUpgradeAfterAbort(ex, abortConnectionAbortedException);
}

[Fact]
public async Task BodyControlThrowOnUpgradeAfterAbort_IOException()
{
var ex = new IOException("My error");

await BodyControlThrowOnUpgradeAfterAbort(ex, abortIOException);
}

private async Task BodyControlThrowOnUpgradeAfterAbort<TException>(TException exception, AbortAction<TException> abortAction)
where TException : Exception
{
var bodyControl = new BodyControl(Mock.Of<IHttpBodyControlFeature>(), Mock.Of<IHttpResponseControl>());

var (request, response, requestPipe, responsePipe) = bodyControl.Start(new MockMessageBody(upgradeable: true));
var ex = new Exception("My error");
bodyControl.Abort(ex);
abortAction(bodyControl, exception);

var upgrade = bodyControl.Upgrade();

var writeEx = await Assert.ThrowsAsync<InvalidOperationException>(() => response.WriteAsync(new byte[1], 0, 1));
Assert.Equal(CoreStrings.ResponseStreamWasUpgraded, writeEx.Message);

Assert.Same(ex,
await Assert.ThrowsAsync<Exception>(() => request.ReadAsync(new byte[1], 0, 1)));
Assert.Same(exception,
(await Assert.ThrowsAsync<TException>(() => request.ReadAsync(new byte[1], 0, 1))).InnerException);

Assert.Same(ex,
await Assert.ThrowsAsync<Exception>(() => upgrade.ReadAsync(new byte[1], 0, 1)));
Assert.Same(ex,
await Assert.ThrowsAsync<Exception>(async () => await requestPipe.ReadAsync()));
Assert.Same(exception,
(await Assert.ThrowsAsync<TException>(() => upgrade.ReadAsync(new byte[1], 0, 1))).InnerException);
Assert.Same(exception,
(await Assert.ThrowsAsync<TException>(async () => await requestPipe.ReadAsync())).InnerException);

await upgrade.WriteAsync(new byte[1], 0, 1);
}


[Fact]
public async Task RequestPipeMethodsThrowAfterAbort()
public async Task RequestPipeMethodsThrowAfterAbort_ConnectionAbortedException()
{
var ex = new ConnectionAbortedException("My error");

await RequestPipeMethodsThrowAfterAbort(ex, abortConnectionAbortedException);
}

[Fact]
public async Task RequestPipeMethodsThrowAfterAbort_IOException()
{
var ex = new IOException("My error");

await RequestPipeMethodsThrowAfterAbort(ex, abortIOException);
}

private async Task RequestPipeMethodsThrowAfterAbort<TException>(TException exception, AbortAction<TException> abortAction)
where TException : Exception
{
var bodyControl = new BodyControl(Mock.Of<IHttpBodyControlFeature>(), Mock.Of<IHttpResponseControl>());

var (_, response, requestPipe, responsePipe) = bodyControl.Start(new MockMessageBody(upgradeable: true));
var ex = new Exception("My error");
bodyControl.Abort(ex);
abortAction(bodyControl, exception);

await response.WriteAsync(new byte[1], 0, 1);
Assert.Same(ex,
Assert.Throws<Exception>(() => requestPipe.AdvanceTo(new SequencePosition())));
Assert.Same(ex,
Assert.Throws<Exception>(() => requestPipe.AdvanceTo(new SequencePosition(), new SequencePosition())));
Assert.Same(ex,
Assert.Throws<Exception>(() => requestPipe.CancelPendingRead()));
Assert.Same(ex,
Assert.Throws<Exception>(() => requestPipe.TryRead(out var res)));
Assert.Same(ex,
Assert.Throws<Exception>(() => requestPipe.Complete()));
Assert.Same(ex,
Assert.Throws<Exception>(() => requestPipe.OnWriterCompleted(null, null)));
Assert.Same(exception,
(Assert.Throws<TException>(() => requestPipe.AdvanceTo(new SequencePosition()))).InnerException);
Assert.Same(exception,
(Assert.Throws<TException>(() => requestPipe.AdvanceTo(new SequencePosition(), new SequencePosition()))).InnerException);
Assert.Same(exception,
(Assert.Throws<TException>(() => requestPipe.CancelPendingRead())).InnerException);
Assert.Same(exception,
(Assert.Throws<TException>(() => requestPipe.TryRead(out var res))).InnerException);
Assert.Same(exception,
(Assert.Throws<TException>(() => requestPipe.Complete())).InnerException);
Assert.Same(exception,
(Assert.Throws<TException>(() => requestPipe.OnWriterCompleted(null, null))).InnerException);
}

[Fact]
Expand Down
Loading