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

Set StatusCode before disposing HttpContext (#876) #993

Merged
merged 2 commits into from
Oct 17, 2016
Merged

Set StatusCode before disposing HttpContext (#876) #993

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
f1071de
Set StatusCode before disposing HttpContext (#876)
mikeharder Jul 19, 2016
7858479
Separate request rejection from bad request state setting.
Oct 12, 2016
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
40 changes: 18 additions & 22 deletions src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;
using System.IO;
using System.Linq;
using System.Net;
Expand Down Expand Up @@ -48,7 +47,6 @@ public abstract partial class Frame : IFrameControl
private readonly object _onStartingSync = new Object();
private readonly object _onCompletedSync = new Object();

protected bool _requestRejected;
private Streams _frameStreams;

protected Stack<KeyValuePair<Func<object, Task>, object>> _onStarting;
Expand All @@ -65,6 +63,7 @@ public abstract partial class Frame : IFrameControl
private bool _canHaveBody;
private bool _autoChunk;
protected Exception _applicationException;
private BadHttpRequestException _requestRejectedException;

protected HttpVersion _httpVersion;

Expand Down Expand Up @@ -718,7 +717,7 @@ private void ProduceStart(bool appCompleted)

protected Task TryProduceInvalidRequestResponse()
{
if (_requestRejected)
if (_requestRejectedException != null)
{
if (FrameRequestHeaders == null || FrameResponseHeaders == null)
{
Expand All @@ -733,7 +732,7 @@ protected Task TryProduceInvalidRequestResponse()

protected Task ProduceEnd()
{
if (_requestRejected || _applicationException != null)
if (_requestRejectedException != null || _applicationException != null)
{
if (HasResponseStarted)
{
Expand All @@ -742,8 +741,13 @@ protected Task ProduceEnd()
return TaskCache.CompletedTask;
}

// If the request was rejected, the error state has already been set by SetBadRequestState
if (!_requestRejected)
// If the request was rejected, the error state has already been set by SetBadRequestState and
// that should take precedence.
if (_requestRejectedException != null)
{
SetErrorResponseHeaders(statusCode: _requestRejectedException.StatusCode);
}
else
{
// 500 Internal Server Error
SetErrorResponseHeaders(statusCode: 500);
Expand Down Expand Up @@ -1395,24 +1399,19 @@ private void ThrowResponseAbortedException()
_applicationException);
}

public void RejectRequest(string message)
public void RejectRequest(RequestRejectionReason reason)
{
throw new ObjectDisposedException(
"The response has been aborted due to an unhandled application exception.",
_applicationException);
RejectRequest(BadHttpRequestException.GetException(reason));
}

public void RejectRequest(RequestRejectionReason reason)
public void RejectRequest(RequestRejectionReason reason, string value)
{
var ex = BadHttpRequestException.GetException(reason);
SetBadRequestState(ex);
throw ex;
RejectRequest(BadHttpRequestException.GetException(reason, value));
}

public void RejectRequest(RequestRejectionReason reason, string value)
private void RejectRequest(BadHttpRequestException ex)
{
var ex = BadHttpRequestException.GetException(reason, value);
SetBadRequestState(ex);
Log.ConnectionBadRequest(ConnectionId, ex);
throw ex;
}

Expand All @@ -1423,17 +1422,14 @@ public void SetBadRequestState(RequestRejectionReason reason)

public void SetBadRequestState(BadHttpRequestException ex)
{
// Setting status code will throw if response has already started
if (!HasResponseStarted)
{
SetErrorResponseHeaders(statusCode: ex.StatusCode);
SetErrorResponseHeaders(ex.StatusCode);
}

_keepAlive = false;
_requestProcessingStopping = true;
_requestRejected = true;

Log.ConnectionBadRequest(ConnectionId, ex);
_requestRejectedException = ex;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't _requestRejectedException be set in RejectRequest?

Copy link
Contributor

@cesarblum cesarblum Oct 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is the appropriate place to set that. Connection calls SetBadRequestState() when a time out occurs, and that will be picked up by later by TryProduceInvalidRequestResponse(). Besides, _requestRejectedException is state too.

}

protected void ReportApplicationError(Exception ex)
Expand Down
113 changes: 68 additions & 45 deletions src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/FrameOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,57 +92,83 @@ public override async Task RequestProcessingAsync()
var context = _application.CreateContext(this);
try
{
await _application.ProcessRequestAsync(context).ConfigureAwait(false);
VerifyResponseContentLength();
}
catch (Exception ex)
{
ReportApplicationError(ex);
}
finally
{
// 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)
try
{
await FireOnStarting();
await _application.ProcessRequestAsync(context).ConfigureAwait(false);
VerifyResponseContentLength();
}
catch (Exception ex)
{
ReportApplicationError(ex);

PauseStreams();

if (_onCompleted != null)
if (ex is BadHttpRequestException)
{
throw;
}
}
finally
{
await FireOnCompleted();
// 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();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

We should be sure to call _application.DisposeContext if messageBody.Consume() throws. The best way to test this is to have an app func that doesn't read the request body and then send a request with malformed chunking.

I mentioned this in a previous iteration, but want to make sure it didn't get lost.


_application.DisposeContext(context, _applicationException);
}

// If _requestAbort is set, the connection has already been closed.
if (Volatile.Read(ref _requestAborted) == 0)
{
ResumeStreams();

if (_keepAlive)
// If _requestAbort is set, the connection has already been closed.
if (Volatile.Read(ref _requestAborted) == 0)
{
// Finish reading the request body in case the app did not.
await messageBody.Consume();
ResumeStreams();

if (_keepAlive)
{
// Finish reading the request body in case the app did not.
await messageBody.Consume();
}

// 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;
Copy link
Member

Choose a reason for hiding this comment

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

How does this interact with TryProduceInvalidRequestResponse()?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking I should move await TryProduceInvalidRequestResponse(); inside the check for _requestAborted == 0.

}

await ProduceEnd();
}

StopStreams();

if (!_keepAlive)
catch (BadHttpRequestException ex)
{
// End the connection for non keep alive as data incoming may have been thrown off
return;
// 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);
Copy link
Member

Choose a reason for hiding this comment

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

Add comment that this is for bad request data in the request body (not start line/headers) and SetBadRequestState can't be called later because _application.DisposeContext logs the status code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Added comments.

}
finally
{
_application.DisposeContext(context, _applicationException);
Copy link
Member

Choose a reason for hiding this comment

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

I think this should be called even if _requestAborted is true. This can be the case if the app calls HttpContext.Abort(). Might want to add a test for that.

It will also probably require another finally block because messageBody.Consume(); can throw too.

Copy link
Contributor Author

@mikeharder mikeharder Jul 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, I will investigate further. This code was changed recently in 3186e1b.

CC: @CesarBS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, if the app calls HttpContext.Abort(), then HttpContext.Response.StatusCode will be 200 when it's disposed (which appears in the logs). I agree this seems wrong. However, if the app aborts the request then no status code is sent to the client, so the status of the request is technically undefined.

What should HttpContext.Response.StatusCode be in this case? We can set it to 500 which is arguably better than 200, but something like null or -1 (property is an int rather than HttpStatusCode) might be more appropriate.

Copy link
Contributor

Choose a reason for hiding this comment

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

503? Service Unavailable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps HttpContext.Response.StatusCode should just be ignored in this case, and the logging code should check HttpContext.RequestAborted, and display something other than the status code (e.g. "aborted").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benaadams: I think HttpContext.Response.StatusCode should remain whatever it was before the request was aborted, which is already working with my current changes. This seems like a separate issue of whether logging should differentiate between completed and aborted requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

One edge case currently not handled, is an app which calls HttpContext.Abort() and then throws an exception, which should probably result in HttpContext.Response.StatusCode = 500. We can try to call ProduceEnd() even if the request has been aborted, which will correct set the status code, but I'm not sure if ProduceEnd() assumes the request was not aborted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not changing it to something related to it being aborted might confuse automated log parsing?

}
}

StopStreams();
Copy link
Member

Choose a reason for hiding this comment

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

Do you think StopStreams() should go in the finally?


if (!_keepAlive)
{
// End the connection for non keep alive as data incoming may have been thrown off
return;
}

// Don't reset frame state if we're exiting the loop. This avoids losing request rejection
// information (for 4xx response), and prevents ObjectDisposedException on HTTPS (ODEs
// will be thrown if PrepareRequest is not null and references objects disposed on connection
Expand All @@ -155,11 +181,9 @@ public override async Task RequestProcessingAsync()
}
catch (BadHttpRequestException ex)
{
if (!_requestRejected)
{
// SetBadRequestState logs the error.
SetBadRequestState(ex);
}
// Handle BadHttpRequestException thrown during request line or header parsing.
// SetBadRequestState logs the error.
SetBadRequestState(ex);
}
catch (Exception ex)
{
Expand All @@ -169,11 +193,10 @@ public override async Task RequestProcessingAsync()
{
try
{
await TryProduceInvalidRequestResponse();

// If _requestAborted is set, the connection has already been closed.
if (Volatile.Read(ref _requestAborted) == 0)
{
await TryProduceInvalidRequestResponse();
ConnectionControl.End(ProduceEndType.SocketShutdown);
}
}
Expand Down
Loading