Skip to content

Start each request on fresh ExecutionContext #14146

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
Apr 3, 2020
Merged

Start each request on fresh ExecutionContext #14146

Show file tree
Hide file tree
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
267 changes: 112 additions & 155 deletions 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 @@ -547,6 +547,8 @@ public async Task ProcessRequestsAsync<TContext>(IHttpApplication<TContext> appl
{
try
{
// We run the request processing loop in a seperate async method so per connection
// exception handling doesn't complicate the generated asm for the loop.
await ProcessRequests(application);
}
catch (BadHttpRequestException ex)
Expand Down Expand Up @@ -624,103 +626,111 @@ private async Task ProcessRequests<TContext>(IHttpApplication<TContext> applicat

InitializeBodyControl(messageBody);

var context = application.CreateContext(this);
// We run user controlled request processing in a seperate async method
// so any changes made to ExecutionContext are undone when it returns and
// each request starts with a fresh ExecutionContext state.
await ProcessRequest(application);

try
{
KestrelEventSource.Log.RequestStart(this);

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

// 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?.Count > 0)
{
await FireOnStarting();
}

if (!_connectionAborted && !VerifyResponseContentLength(out var lengthException))
{
ReportApplicationError(lengthException);
}
}
catch (BadHttpRequestException ex)
// Even for non-keep-alive requests, try to consume the entire body to avoid RSTs.
if (!_connectionAborted && _requestRejectedException == null && !messageBody.IsEmpty)
{
// Capture BadHttpRequestException for further processing
// This has to be caught here so StatusCode is set properly before disposing the HttpContext
// (DisposeContext logs StatusCode).
SetBadRequestState(ex);
ReportApplicationError(ex);
await messageBody.ConsumeAsync();
}
catch (Exception ex)

if (HasStartedConsumingRequestBody)
{
ReportApplicationError(ex);
await messageBody.StopAsync();
}
}
}

KestrelEventSource.Log.RequestStop(this);
private async ValueTask ProcessRequest<TContext>(IHttpApplication<TContext> application)
Copy link
Member Author

Choose a reason for hiding this comment

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

Task is a more performant return here than ValueTask and will allocate the same; i.e none in the sync completion and the async box in the async wait scenario (subject to dotnet/coreclr#26310 when ValueTask becomes better as it doesn't allocate in async)

{
var context = application.CreateContext(this);

// At this point all user code that needs use to the request or response streams has completed.
// Using these streams in the OnCompleted callback is not allowed.
try
{
await _bodyControl.StopAsync();
}
catch (Exception ex)
{
// BodyControl.StopAsync() can throw if the PipeWriter was completed prior to the application writing
// enough bytes to satisfy the specified Content-Length. This risks double-logging the exception,
// but this scenario generally indicates an app bug, so I don't want to risk not logging it.
ReportApplicationError(ex);
}
try
{
KestrelEventSource.Log.RequestStart(this);

// 4XX responses are written by TryProduceInvalidRequestResponse during connection tear down.
if (_requestRejectedException == null)
// Run the application code for this request
await application.ProcessRequestAsync(context);

// 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?.Count > 0)
{
if (!_connectionAborted)
{
// 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();
}
else if (!HasResponseStarted)
{
// If the request was aborted and no response was sent, there's no
// meaningful status code to log.
StatusCode = 0;
}
await FireOnStarting();
}

if (_onCompleted?.Count > 0)
if (!_connectionAborted && !VerifyResponseContentLength(out var lengthException))
{
await FireOnCompleted();
ReportApplicationError(lengthException);
}
}
catch (BadHttpRequestException ex)
{
// Capture BadHttpRequestException for further processing
// This has to be caught here so StatusCode is set properly before disposing the HttpContext
// (DisposeContext logs StatusCode).
SetBadRequestState(ex);
ReportApplicationError(ex);
}
catch (Exception ex)
{
ReportApplicationError(ex);
}

application.DisposeContext(context, _applicationException);
KestrelEventSource.Log.RequestStop(this);

// Even for non-keep-alive requests, try to consume the entire body to avoid RSTs.
if (!_connectionAborted && _requestRejectedException == null && !messageBody.IsEmpty)
// At this point all user code that needs use to the request or response streams has completed.
// Using these streams in the OnCompleted callback is not allowed.
try
{
await _bodyControl.StopAsync();
}
catch (Exception ex)
{
// BodyControl.StopAsync() can throw if the PipeWriter was completed prior to the application writing
// enough bytes to satisfy the specified Content-Length. This risks double-logging the exception,
// but this scenario generally indicates an app bug, so I don't want to risk not logging it.
ReportApplicationError(ex);
}

// 4XX responses are written by TryProduceInvalidRequestResponse during connection tear down.
if (_requestRejectedException == null)
{
if (!_connectionAborted)
{
await messageBody.ConsumeAsync();
// 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();
}

if (HasStartedConsumingRequestBody)
else if (!HasResponseStarted)
{
await messageBody.StopAsync();
// If the request was aborted and no response was sent, there's no
// meaningful status code to log.
StatusCode = 0;
}
}

if (_onCompleted?.Count > 0)
{
await FireOnCompleted();
}

application.DisposeContext(context, _applicationException);
}

public void OnStarting(Func<object, Task> callback, object state)
Expand Down Expand Up @@ -749,108 +759,55 @@ public void OnCompleted(Func<object, Task> callback, object state)
protected Task FireOnStarting()
{
var onStarting = _onStarting;

if (onStarting == null || onStarting.Count == 0)
{
return Task.CompletedTask;
}
else
if (onStarting?.Count > 0)
{
return FireOnStartingMayAwait(onStarting);
return ProcessEvents(this, onStarting);
}
}

private Task FireOnStartingMayAwait(Stack<KeyValuePair<Func<object, Task>, object>> onStarting)
{
try
return Task.CompletedTask;

static async Task ProcessEvents(HttpProtocol protocol, Stack<KeyValuePair<Func<object, Task>, object>> events)
{
while (onStarting.TryPop(out var entry))
// Try/Catch is outside the loop as any error that occurs is before the request starts.
// So we want to report it as an ApplicationError to fail the request and not process more events.
try
{
var task = entry.Key.Invoke(entry.Value);
if (!task.IsCompletedSuccessfully)
while (events.TryPop(out var entry))
{
return FireOnStartingAwaited(task, onStarting);
await entry.Key.Invoke(entry.Value);
}
}
}
catch (Exception ex)
{
ReportApplicationError(ex);
}

return Task.CompletedTask;
}

private async Task FireOnStartingAwaited(Task currentTask, Stack<KeyValuePair<Func<object, Task>, object>> onStarting)
{
try
{
await currentTask;

while (onStarting.TryPop(out var entry))
catch (Exception ex)
{
await entry.Key.Invoke(entry.Value);
protocol.ReportApplicationError(ex);
}
}
catch (Exception ex)
{
ReportApplicationError(ex);
}
}

protected Task FireOnCompleted()
{
var onCompleted = _onCompleted;

if (onCompleted == null || onCompleted.Count == 0)
if (onCompleted?.Count > 0)
{
return Task.CompletedTask;
return ProcessEvents(this, onCompleted);
}

return FireOnCompletedMayAwait(onCompleted);
}
return Task.CompletedTask;

private Task FireOnCompletedMayAwait(Stack<KeyValuePair<Func<object, Task>, object>> onCompleted)
{
while (onCompleted.TryPop(out var entry))
static async Task ProcessEvents(HttpProtocol protocol, Stack<KeyValuePair<Func<object, Task>, object>> events)
{
try
// Try/Catch is inside the loop as any error that occurs is after the request has finished.
// So we will just log it and keep processing the events, as the completion has already happened.
while (events.TryPop(out var entry))
{
var task = entry.Key.Invoke(entry.Value);
if (!task.IsCompletedSuccessfully)
try
{
return FireOnCompletedAwaited(task, onCompleted);
await entry.Key.Invoke(entry.Value);
}
catch (Exception ex)
{
protocol.Log.ApplicationError(protocol.ConnectionId, protocol.TraceIdentifier, ex);
}
}
catch (Exception ex)
{
ReportApplicationError(ex);
}
}

return Task.CompletedTask;
}

private async Task FireOnCompletedAwaited(Task currentTask, Stack<KeyValuePair<Func<object, Task>, object>> onCompleted)
{
try
{
await currentTask;
}
catch (Exception ex)
{
Log.ApplicationError(ConnectionId, TraceIdentifier, ex);
}

while (onCompleted.TryPop(out var entry))
{
try
{
await entry.Key.Invoke(entry.Value);
}
catch (Exception ex)
{
Log.ApplicationError(ConnectionId, TraceIdentifier, ex);
}
}
}
Expand Down
Loading