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

Don't emit TE header or body for non-body responses #962

Closed
wants to merge 1 commit into from
Closed

Don't emit TE header or body for non-body responses #962

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
75 changes: 75 additions & 0 deletions src/Microsoft.AspNetCore.Server.Kestrel/BadHttpResponse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// Copyright (c) .NET Foundation. All rights reserved.
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Runtime.CompilerServices;
using Microsoft.AspNetCore.Server.Kestrel.Internal.Http;

namespace Microsoft.AspNetCore.Server.Kestrel
{
public static class BadHttpResponse
{
internal static void ThrowException(ResponseRejectionReasons reason)
{
throw GetException(reason);
}

internal static void ThrowException(ResponseRejectionReasons reason, int value)
{
throw GetException(reason, value.ToString());
}

internal static void ThrowException(ResponseRejectionReasons reason, ResponseRejectionParameter parameter)
{
throw GetException(reason, parameter.ToString());
}

internal static InvalidOperationException GetException(ResponseRejectionReasons reason, int value)
{
return GetException(reason, value.ToString());
}

[MethodImpl(MethodImplOptions.NoInlining)]
internal static InvalidOperationException GetException(ResponseRejectionReasons reason)
{
InvalidOperationException ex;
switch (reason)
{
case ResponseRejectionReasons.HeadersReadonlyResponseStarted:
ex = new InvalidOperationException("Headers are read-only, response has already started.");
break;
case ResponseRejectionReasons.OnStartingCannotBeSetResponseStarted:
ex = new InvalidOperationException("OnStarting cannot be set, response has already started.");
break;
default:
ex = new InvalidOperationException("Bad response.");
break;
}

return ex;
}

[MethodImpl(MethodImplOptions.NoInlining)]
private static InvalidOperationException GetException(ResponseRejectionReasons reason, string value)
{
InvalidOperationException ex;
switch (reason)
{
case ResponseRejectionReasons.ValueCannotBeSetResponseStarted:
ex = new InvalidOperationException(value + " cannot be set, response had already started.");
break;
case ResponseRejectionReasons.TransferEncodingSetOnNonBodyResponse:
ex = new InvalidOperationException($"Transfer-Encoding set on a {value} non-body request.");
break;
case ResponseRejectionReasons.WriteToNonBodyResponse:
ex = new InvalidOperationException($"Write to non-body {value} response.");
break;
default:
ex = new InvalidOperationException("Bad response.");
break;
}

return ex;
}
}
}
193 changes: 133 additions & 60 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 @@ -58,6 +58,7 @@ public abstract partial class Frame : ConnectionContext, IFrameControl

private RequestProcessingStatus _requestProcessingStatus;
protected bool _keepAlive;
private bool _canHaveBody;
private bool _autoChunk;
protected Exception _applicationException;

Expand Down Expand Up @@ -135,7 +136,7 @@ public int StatusCode
{
if (HasResponseStarted)
{
ThrowResponseAlreadyStartedException(nameof(StatusCode));
BadHttpResponse.ThrowException(ResponseRejectionReasons.ValueCannotBeSetResponseStarted, ResponseRejectionParameter.StatusCode);
}

_statusCode = value;
Expand All @@ -153,7 +154,7 @@ public string ReasonPhrase
{
if (HasResponseStarted)
{
ThrowResponseAlreadyStartedException(nameof(ReasonPhrase));
BadHttpResponse.ThrowException(ResponseRejectionReasons.ValueCannotBeSetResponseStarted, ResponseRejectionParameter.ReasonPhrase);
}

_reasonPhrase = value;
Expand Down Expand Up @@ -388,7 +389,7 @@ public void OnStarting(Func<object, Task> callback, object state)
{
if (HasResponseStarted)
{
ThrowResponseAlreadyStartedException(nameof(OnStarting));
BadHttpResponse.ThrowException(ResponseRejectionReasons.OnStartingCannotBeSetResponseStarted, ResponseRejectionParameter.OnStarting);
}

if (_onStarting == null)
Expand Down Expand Up @@ -475,17 +476,24 @@ public void Write(ArraySegment<byte> data)
{
ProduceStartAndFireOnStarting().GetAwaiter().GetResult();

if (_autoChunk)
if (_canHaveBody)
{
if (data.Count == 0)
if (_autoChunk)
{
if (data.Count == 0)
{
return;
}
WriteChunked(data);
}
else
{
return;
SocketOutput.Write(data);
}
WriteChunked(data);
}
else
{
SocketOutput.Write(data);
HandleNonBodyResponseWrite(data.Count);
}
}

Expand All @@ -496,36 +504,53 @@ public Task WriteAsync(ArraySegment<byte> data, CancellationToken cancellationTo
return WriteAsyncAwaited(data, cancellationToken);
}

if (_autoChunk)
if (_canHaveBody)
{
if (data.Count == 0)
if (_autoChunk)
{
return TaskUtilities.CompletedTask;
if (data.Count == 0)
{
return TaskUtilities.CompletedTask;
}
return WriteChunkedAsync(data, cancellationToken);
}
else
{
return SocketOutput.WriteAsync(data, cancellationToken: cancellationToken);
}
return WriteChunkedAsync(data, cancellationToken);
}
else
{
return SocketOutput.WriteAsync(data, cancellationToken: cancellationToken);
HandleNonBodyResponseWrite(data.Count);
return TaskUtilities.CompletedTask;
}
}

public async Task WriteAsyncAwaited(ArraySegment<byte> data, CancellationToken cancellationToken)
{
await ProduceStartAndFireOnStarting();

if (_autoChunk)
if (_canHaveBody)
{
if (data.Count == 0)
if (_autoChunk)
{
if (data.Count == 0)
{
return;
}
await WriteChunkedAsync(data, cancellationToken);
}
else
{
return;
await SocketOutput.WriteAsync(data, cancellationToken: cancellationToken);
}
await WriteChunkedAsync(data, cancellationToken);
}
else
{
await SocketOutput.WriteAsync(data, cancellationToken: cancellationToken);
HandleNonBodyResponseWrite(data.Count);
return;
}

}

private void WriteChunked(ArraySegment<byte> data)
Expand Down Expand Up @@ -640,28 +665,14 @@ protected Task ProduceEnd()

if (_requestRejected)
{
// 400 Bad Request
StatusCode = 400;
_keepAlive = false;
// 400 Bad Request
ErrorResetHeadersToDefaults(statusCode: 400);
}
else
{
// 500 Internal Server Error
StatusCode = 500;
}

ReasonPhrase = null;

var responseHeaders = FrameResponseHeaders;
responseHeaders.Reset();
var dateHeaderValues = DateHeaderValueManager.GetDateHeaderValues();

responseHeaders.SetRawDate(dateHeaderValues.String, dateHeaderValues.Bytes);
responseHeaders.SetRawContentLength("0", _bytesContentLengthZero);

if (ServerOptions.AddServerHeader)
{
responseHeaders.SetRawServer(Constants.ServerName, _bytesServer);
ErrorResetHeadersToDefaults(statusCode: 500);
}
}

Expand Down Expand Up @@ -715,50 +726,61 @@ private void CreateResponseHeader(
bool appCompleted)
{
var responseHeaders = FrameResponseHeaders;
responseHeaders.SetReadOnly();

var hasConnection = responseHeaders.HasConnection;

// Set whether response can have body
_canHaveBody = StatusCanHaveBody(StatusCode) && Method != "HEAD";

var end = SocketOutput.ProducingStart();
if (_keepAlive && hasConnection)
{
var connectionValue = responseHeaders.HeaderConnection.ToString();
_keepAlive = connectionValue.Equals("keep-alive", StringComparison.OrdinalIgnoreCase);
}

if (!responseHeaders.HasTransferEncoding && !responseHeaders.HasContentLength)
if (_canHaveBody)
Copy link
Contributor

Choose a reason for hiding this comment

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

@benaadams I'm curious why you removed this condition in your change. I had to re-introduce the keep-alive check, otherwise some tests would fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it relevant? Unless you are saying only keep-alive can be chunked?

Copy link
Member

Choose a reason for hiding this comment

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

That's the current logic. Why bother chunking on Connection: close responses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't looked into it too closely; but couldn't you be returning a 200, chunked data and a Connection: close?

Copy link
Contributor

@cesarblum cesarblum Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

We have a test (ChunkedResponseTests.ResponsesAreNotChunkedAutomaticallyForHttp10RequestsAndHttp11NonKeepAliveRequests) that specifically checks that we don't chunk HTTP/1.1 non-keepalive responses. Is that in the spec?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You'd have to buffer all the data then to work out the size?

Copy link
Member

Choose a reason for hiding this comment

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

Not part of the spec for HTTP/1.1. It obviously is for HTTP/1.0 requests.

Chunked or not, the client won't work out the total size of the response until the connection closes. Why go through the extra cycles doing chunking for no apparent benefit?

Copy link
Contributor Author

@benaadams benaadams Sep 30, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, you mean because the connection is closing it doesn't need a Content-Length or Transfer-Encoding header because the FIN indicates that? How does the client detect a partial response?

e.g. exception thrown mid file read

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed with @benaadams - if we don't chunk, the client has no way to tell whether the response ended or the connection closed abruptly. The client could in theory verify if it was a clean connection close or a reset, but we shouldn't count on that.

If it's not in the spec I say we should chunk HTTP/1.1 even on Connection: close.

Copy link
Member

Choose a reason for hiding this comment

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

Good point about application errors. That's a fact of life for HTTP/1.0, but reliably knowing that a response has gracefully closed is a benefit of chunked encoding even if it's a non keep-alive connection.

I'm now convinced about this changed. Just make sure we don't regress the behavior for HTTP/1.0 requests. It's easier to mess up now that all responses are HTTP/1.1.

{
if (appCompleted)
if (!responseHeaders.HasTransferEncoding && !responseHeaders.HasContentLength)
{
// Don't set the Content-Length or Transfer-Encoding headers
// automatically for HEAD requests or 101, 204, 205, 304 responses.
if (Method != "HEAD" && StatusCanHaveBody(StatusCode))
if (appCompleted)
{
// Since the app has completed and we are only now generating
// the headers we can safely set the Content-Length to 0.
responseHeaders.SetRawContentLength("0", _bytesContentLengthZero);
}
}
else if(_keepAlive)
{
// Note for future reference: never change this to set _autoChunk to true on HTTP/1.0
// connections, even if we were to infer the client supports it because an HTTP/1.0 request
// was received that used chunked encoding. Sending a chunked response to an HTTP/1.0
// client would break compliance with RFC 7230 (section 3.3.1):
//
// A server MUST NOT send a response containing Transfer-Encoding unless the corresponding
// request indicates HTTP/1.1 (or later).
if (_httpVersion == Http.HttpVersion.Http11)
{
_autoChunk = true;
responseHeaders.SetRawTransferEncoding("chunked", _bytesTransferEncodingChunked);
}
else
{
_keepAlive = false;
// Note for future reference: never change this to set _autoChunk to true on HTTP/1.0
// connections, even if we were to infer the client supports it because an HTTP/1.0 request
// was received that used chunked encoding. Sending a chunked response to an HTTP/1.0
// client would break compliance with RFC 7230 (section 3.3.1):
//
// A server MUST NOT send a response containing Transfer-Encoding unless the corresponding
// request indicates HTTP/1.1 (or later).
if (_httpVersion == Http.HttpVersion.Http11)
{
_autoChunk = true;
responseHeaders.SetRawTransferEncoding("chunked", _bytesTransferEncodingChunked);
}
else
{
_keepAlive = false;
}
}
}
}
else
{
// Don't set the Content-Length or Transfer-Encoding headers
// automatically for HEAD requests or 101, 204, 205, 304 responses.
if (responseHeaders.HasTransferEncoding)
Copy link
Member

Choose a reason for hiding this comment

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

Why not reject if the Content-Length header is set while we're at it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Content-Length can be part of the HEAD request; as long as it is what GET would say.

https://tools.ietf.org/html/rfc7230#section-3.3.2

A server MAY send a Content-Length header field in a response to a
HEAD request (Section 4.3.2 of [RFC7231]); a server MUST NOT send
Content-Length in such a response unless its field-value equals the
decimal number of octets that would have been sent in the payload
body of a response if the same request had used the GET method.

A server MAY send a Content-Length header field in a 304 (Not
Modified) response to a conditional GET request (Section 4.1 of
[RFC7232]); a server MUST NOT send Content-Length in such a response
unless its field-value equals the decimal number of octets that would
have been sent in the payload body of a 200 (OK) response to the same
request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

e.g. don't set it; but don't remove it or error it

{
RejectNonBodyTransferEncodingResponse(appCompleted);
}
}

responseHeaders.SetReadOnly();

if (!_keepAlive && !hasConnection)
{
Expand Down Expand Up @@ -1215,12 +1237,63 @@ public bool StatusCanHaveBody(int statusCode)
statusCode != 304;
}

private void ThrowResponseAlreadyStartedException(string value)
private void RejectNonBodyTransferEncodingResponse(bool appCompleted)
{
throw new InvalidOperationException(value + " cannot be set, response had already started.");
var ex = BadHttpResponse.GetException(ResponseRejectionReasons.TransferEncodingSetOnNonBodyResponse, StatusCode);
if (!appCompleted)
{
// Back out of header creation surface exeception in user code
_requestProcessingStatus = RequestProcessingStatus.RequestStarted;
Copy link
Member

Choose a reason for hiding this comment

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

If we're logging an not throwing for HEAD requests in HandleNonBodyResponseWrite, shouldn't we just remove the header and do the same here? I think we should be consistent.

throw ex;
}
else
{
ReportApplicationError(ex);
// 500 Internal Server Error
ErrorResetHeadersToDefaults(statusCode: 500);
}
}

private void ErrorResetHeadersToDefaults(int statusCode)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'd prefer a different name. I'm thinking of PrepareErrorResponse or SetErrorResponse.

{
StatusCode = statusCode;
ReasonPhrase = null;

var responseHeaders = FrameResponseHeaders;
responseHeaders.Reset();
var dateHeaderValues = DateHeaderValueManager.GetDateHeaderValues();

responseHeaders.SetRawDate(dateHeaderValues.String, dateHeaderValues.Bytes);
responseHeaders.SetRawContentLength("0", _bytesContentLengthZero);

if (ServerOptions.AddServerHeader)
{
responseHeaders.SetRawServer(Constants.ServerName, _bytesServer);
}
}

public void HandleNonBodyResponseWrite(int count)
{
if (Method == "HEAD")
{
// Don't write to body for HEAD requests.
Log.ConnectionHeadResponseBodyWrite(ConnectionId, count);
}
else
{
// Throw Exception for 101, 204, 205, 304 responses.
BadHttpResponse.ThrowException(ResponseRejectionReasons.WriteToNonBodyResponse, StatusCode);
}
}

private void ThrowResponseAbortedException()
{
throw new ObjectDisposedException(
"The response has been aborted due to an unhandled application exception.",
_applicationException);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: single indent level

}

public void RejectRequest(string message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't that be RejectResponse?

{
throw new ObjectDisposedException(
"The response has been aborted due to an unhandled application exception.",
Expand Down
Loading