Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Flatten exception handling #2313

Merged
halter73 merged 1 commit into from
Feb 23, 2018
Merged

Flatten exception handling #2313

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
286 changes: 152 additions & 134 deletions src/Kestrel.Core/Internal/Http/HttpProtocol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public abstract partial class HttpProtocol : IHttpResponseControl
private CancellationToken? _manuallySetRequestAbortToken;

protected RequestProcessingStatus _requestProcessingStatus;
protected volatile bool _keepAlive = true; // volatile, see: https://msdn.microsoft.com/en-us/library/x13ttww7.aspx
protected volatile bool _keepAlive; // volatile, see: https://msdn.microsoft.com/en-us/library/x13ttww7.aspx
protected bool _upgradeAvailable;
private bool _canHaveBody;
private bool _autoChunk;
Expand Down Expand Up @@ -440,139 +440,7 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> appl
{
try
{
while (_keepAlive)
{
BeginRequestProcessing();

var result = default(ReadResult);
var endConnection = false;
do
{
if (BeginRead(out var awaitable))
{
result = await awaitable;
}
} while (!TryParseRequest(result, out endConnection));

if (endConnection)
{
return;
}

var messageBody = CreateMessageBody();

if (!messageBody.RequestKeepAlive)
{
_keepAlive = false;
}

_upgradeAvailable = messageBody.RequestUpgrade;

InitializeStreams(messageBody);

var httpContext = application.CreateContext(this);

try
{
try
{
KestrelEventSource.Log.RequestStart(this);

await application.ProcessRequestAsync(httpContext);

if (_requestAborted == 0)
{
VerifyResponseContentLength();
}
}
catch (Exception ex)
{
ReportApplicationError(ex);

if (ex is BadHttpRequestException)
{
throw;
}
}
finally
{
KestrelEventSource.Log.RequestStop(this);

// Trigger OnStarting if it hasn't been called yet and the app hasn't
// already failed. If an OnStarting callback throws we can go through
// our normal error handling in ProduceEnd.
// https://github.com/aspnet/KestrelHttpServer/issues/43
if (!HasResponseStarted && _applicationException == null && _onStarting != null)
{
await FireOnStarting();
}

PauseStreams();

if (_onCompleted != null)
{
await FireOnCompleted();
}
}

// If _requestAbort is set, the connection has already been closed.
if (_requestAborted == 0)
{
// Call ProduceEnd() before consuming the rest of the request body to prevent
// delaying clients waiting for the chunk terminator:
//
// https://github.com/dotnet/corefx/issues/17330#issuecomment-288248663
//
// This also prevents the 100 Continue response from being sent if the app
// never tried to read the body.
// https://github.com/aspnet/KestrelHttpServer/issues/2102
//
// ProduceEnd() must be called before _application.DisposeContext(), to ensure
// HttpContext.Response.StatusCode is correctly set when
// IHttpContextFactory.Dispose(HttpContext) is called.
await ProduceEnd();

// ForZeroContentLength does not complete the reader nor the writer
if (!messageBody.IsEmpty)
{
// Finish reading the request body in case the app did not.
await messageBody.ConsumeAsync();
}
}
else if (!HasResponseStarted)
{
// If the request was aborted and no response was sent, there's no
// meaningful status code to log.
StatusCode = 0;
}
}
catch (BadHttpRequestException ex)
{
// Handle BadHttpRequestException thrown during app execution or remaining message body consumption.
// This has to be caught here so StatusCode is set properly before disposing the HttpContext
// (DisposeContext logs StatusCode).
SetBadRequestState(ex);
}
finally
{
application.DisposeContext(httpContext, _applicationException);

// StopStreams should be called before the end of the "if (!_requestProcessingStopping)" block
// to ensure InitializeStreams has been called.
StopStreams();

if (HasStartedConsumingRequestBody)
{
RequestBodyPipe.Reader.Complete();

// Wait for MessageBody.PumpAsync() to call RequestBodyPipe.Writer.Complete().
await messageBody.StopAsync();

// At this point both the request body pipe reader and writer should be completed.
RequestBodyPipe.Reset();
}
}
}
await ProcessRequests(application);
}
catch (BadHttpRequestException ex)
Copy link
Member

@davidfowl davidfowl Feb 21, 2018

Choose a reason for hiding this comment

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

Can this still happen? At this level all error are connection oriented right?

Copy link
Contributor Author

@benaadams benaadams Feb 21, 2018

Choose a reason for hiding this comment

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

Parsing headers and request line can throw a BadHttpRequestException which gets caught here

Copy link
Member

@davidfowl davidfowl Feb 21, 2018

Choose a reason for hiding this comment

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

I see

Copy link
Contributor Author

@benaadams benaadams Feb 21, 2018

Choose a reason for hiding this comment

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

Is before any request has started so it catches here and tears down the connection

{
Expand Down Expand Up @@ -615,6 +483,156 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> appl
}
}

private async Task ProcessRequests<TContext>(IHttpApplication<TContext> application)
{
// Keep-alive is default for HTTP/1.1 and HTTP/2; parsing and errors will change its value
_keepAlive = true;
do
{
BeginRequestProcessing();

var result = default(ReadResult);
var endConnection = false;
do
{
if (BeginRead(out var awaitable))
{
result = await awaitable;
}
} while (!TryParseRequest(result, out endConnection));

if (endConnection)
{
// Connection finished, stop processing requests
return;
}

var messageBody = CreateMessageBody();
if (!messageBody.RequestKeepAlive)
{
_keepAlive = false;
}

_upgradeAvailable = messageBody.RequestUpgrade;

InitializeStreams(messageBody);

var httpContext = application.CreateContext(this);

BadHttpRequestException badRequestException = null;
try
{
KestrelEventSource.Log.RequestStart(this);

// Run the application code for this request
await application.ProcessRequestAsync(httpContext);

if (_requestAborted == 0)
{
VerifyResponseContentLength();
}
}
catch (Exception ex)
{
ReportApplicationError(ex);
// Capture BadHttpRequestException for further processing
badRequestException = ex as BadHttpRequestException;
}

KestrelEventSource.Log.RequestStop(this);

// Trigger OnStarting if it hasn't been called yet and the app hasn't
// already failed. If an OnStarting callback throws we can go through
// our normal error handling in ProduceEnd.
// https://github.com/aspnet/KestrelHttpServer/issues/43
if (!HasResponseStarted && _applicationException == null && _onStarting != null)
{
await FireOnStarting();
}

PauseStreams();

if (_onCompleted != null)
{
await FireOnCompleted();
}

if (badRequestException == null)
{
// If _requestAbort is set, the connection has already been closed.
if (_requestAborted == 0)
{
// Call ProduceEnd() before consuming the rest of the request body to prevent
// delaying clients waiting for the chunk terminator:
//
// https://github.com/dotnet/corefx/issues/17330#issuecomment-288248663
//
// This also prevents the 100 Continue response from being sent if the app
// never tried to read the body.
// https://github.com/aspnet/KestrelHttpServer/issues/2102
//
// ProduceEnd() must be called before _application.DisposeContext(), to ensure
// HttpContext.Response.StatusCode is correctly set when
// IHttpContextFactory.Dispose(HttpContext) is called.
await ProduceEnd();

// ForZeroContentLength does not complete the reader nor the writer
if (!messageBody.IsEmpty)
{
try
{
// Finish reading the request body in case the app did not.
await messageBody.ConsumeAsync();
Copy link
Member

@halter73 halter73 Feb 17, 2018

Choose a reason for hiding this comment

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

How confident are we that this is the only thing that can throw after invoking the application and verifying the response body length?

Personally, I'm fairly confident this is the case, but I want to make extra sure that we dispose the HttpContext in all the common scenarios.

Copy link
Contributor Author

@benaadams benaadams Feb 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Was thinking FireOnStarting and FireOnCompleted if the user code they executed threw; but they don't let exceptions escape; so should be ok?

Copy link
Member

@halter73 halter73 Feb 21, 2018

Choose a reason for hiding this comment

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

Yep. I think this change is safe. I just wanted an extra pair of eyes.

}
catch (BadHttpRequestException ex)
{
// Capture BadHttpRequestException for further processing
badRequestException = ex;
}
}
}
else if (!HasResponseStarted)
{
// If the request was aborted and no response was sent, there's no
// meaningful status code to log.
StatusCode = 0;
}
}

if (badRequestException != null)
{
// Handle BadHttpRequestException thrown during app execution or remaining message body consumption.
// This has to be caught here so StatusCode is set properly before disposing the HttpContext
// (DisposeContext logs StatusCode).
SetBadRequestState(badRequestException);
}

application.DisposeContext(httpContext, _applicationException);
Copy link
Member

@davidfowl davidfowl Feb 21, 2018

Choose a reason for hiding this comment

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

This not being in a finally makes me nervous, @halter73 I'm hoping our tests are complete here.

Copy link
Contributor Author

@benaadams benaadams Feb 21, 2018

Choose a reason for hiding this comment

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

Only thing would be if await ProduceEnd could throw a BadRequestException

Copy link
Member

@davidfowl davidfowl Feb 21, 2018

Choose a reason for hiding this comment

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

Seems like it could? What happens if you set a utf8 response header before the response starts then you throw from the application?

Copy link
Contributor Author

@benaadams benaadams Feb 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

They are checked when they are set and would throw then in the application. When they are written out they use PipelineExtensions.WriteAsciiNoValidation

Copy link
Contributor Author

@benaadams benaadams Feb 21, 2018

Choose a reason for hiding this comment

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

Taking of which EncodeAsciiCharsToBytes looks like it could go faster :)

Copy link
Member

@halter73 halter73 Feb 21, 2018

Choose a reason for hiding this comment

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

I don't think ProduceEnd should ever throw.


// StopStreams should be called before the end of the "if (!_requestProcessingStopping)" block
// to ensure InitializeStreams has been called.
StopStreams();

if (HasStartedConsumingRequestBody)
{
RequestBodyPipe.Reader.Complete();

// Wait for MessageBody.PumpAsync() to call RequestBodyPipe.Writer.Complete().
await messageBody.StopAsync();

// At this point both the request body pipe reader and writer should be completed.
RequestBodyPipe.Reset();
}

if (badRequestException != null)
{
// Bad request reported, stop processing requests
return;
}

} while (_keepAlive);
}

public void OnStarting(Func<object, Task> callback, object state)
{
lock (_onStartingSync)
Expand Down