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

Call ProduceEnd() before consuming request body if response has already started (#1530) #1537

Merged
merged 2 commits into from
Mar 31, 2017
Merged

Call ProduceEnd() before consuming request body if response has already started (#1530) #1537

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
a6d9b4d
Call ProduceEnd() before consuming request body if response has alrea…
Mar 30, 2017
3f0f56b
Small test change.
Mar 30, 2017
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
22 changes: 18 additions & 4 deletions src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/FrameOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,16 +135,30 @@ public override async Task RequestProcessingAsync()
{
ResumeStreams();

if (HasResponseStarted)
{
// If the response has already started, 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
//
// ProduceEnd() must be called before _application.DisposeContext(), to ensure
// HttpContext.Response.StatusCode is correctly set when
// IHttpContextFactory.Dispose(HttpContext) is called.
await ProduceEnd();
}

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();
if (!HasResponseStarted)
{
await ProduceEnd();
}
}
else if (!HasResponseStarted)
{
Expand Down
190 changes: 173 additions & 17 deletions test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/ResponseTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ public async Task SingleErrorResponseSentWhenAppSwallowsBadRequestException()
{
readException = await Assert.ThrowsAsync<BadHttpRequestException>(
async () => await httpContext.Request.Body.ReadAsync(new byte[1], 0, 1));
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -370,7 +370,7 @@ public async Task TransferEncodingChunkedSetOnUnknownLengthHttp11Response()
{
await httpContext.Response.WriteAsync("hello, ");
await httpContext.Response.WriteAsync("world");
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -404,7 +404,7 @@ public async Task TransferEncodingChunkedNotSetOnNonBodyResponse(int statusCode)
{
httpContext.Response.StatusCode = statusCode;
return TaskCache.CompletedTask;
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand All @@ -427,7 +427,7 @@ public async Task TransferEncodingNotSetOnHeadResponse()
using (var server = new TestServer(httpContext =>
{
return TaskCache.CompletedTask;
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -880,7 +880,7 @@ public async Task HeadResponseCanContainContentLengthHeader()
{
httpContext.Response.ContentLength = 42;
return TaskCache.CompletedTask;
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -908,7 +908,7 @@ public async Task HeadResponseBodyNotWrittenWithAsyncWrite()
httpContext.Response.ContentLength = 12;
await httpContext.Response.WriteAsync("hello, world");
await flushed.WaitAsync();
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -939,7 +939,7 @@ public async Task HeadResponseBodyNotWrittenWithSyncWrite()
httpContext.Response.Body.Write(Encoding.ASCII.GetBytes("hello, world"), 0, 12);
flushed.Wait();
return TaskCache.CompletedTask;
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -970,7 +970,7 @@ public async Task ZeroLengthWritesFlushHeaders()
await httpContext.Response.WriteAsync("");
flushed.Wait();
await httpContext.Response.WriteAsync("hello, world");
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -1013,7 +1013,7 @@ public async Task WriteAfterConnectionCloseNoops()
{
tcs.TrySetException(ex);
}
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -1053,7 +1053,7 @@ public async Task AppCanWriteOwnBadRequestResponse()
await httpContext.Response.WriteAsync(ex.Message);
responseWritten.Release();
}
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -1084,7 +1084,7 @@ public async Task ConnectionClosedWhenChunkedIsNotFinalTransferCoding(string res
{
httpContext.Response.Headers["Transfer-Encoding"] = responseTransferEncoding;
await httpContext.Response.WriteAsync("hello, world");
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -1131,7 +1131,7 @@ public async Task ConnectionClosedWhenChunkedIsNotFinalTransferCodingEvenIfConne
httpContext.Response.Headers["Connection"] = "keep-alive";
httpContext.Response.Headers["Transfer-Encoding"] = responseTransferEncoding;
await httpContext.Response.WriteAsync("hello, world");
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -1177,7 +1177,7 @@ public async Task ConnectionKeptAliveWhenChunkedIsFinalTransferCoding(string res

// App would have to chunk manually, but here we don't care
await httpContext.Response.WriteAsync("hello, world");
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -1225,7 +1225,7 @@ public async Task FirstWriteVerifiedAfterOnStarting()
// If OnStarting is not run before verifying writes, an error response will be sent.
httpContext.Response.Body.Write(response, 0, response.Length);
return TaskCache.CompletedTask;
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -1266,7 +1266,7 @@ public async Task SubsequentWriteVerifiedAfterOnStarting()
httpContext.Response.Body.Write(response, 0, response.Length / 2);
httpContext.Response.Body.Write(response, response.Length / 2, response.Length - response.Length / 2);
return TaskCache.CompletedTask;
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -1307,7 +1307,7 @@ public async Task FirstWriteAsyncVerifiedAfterOnStarting()

// If OnStarting is not run before verifying writes, an error response will be sent.
return httpContext.Response.Body.WriteAsync(response, 0, response.Length);
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand Down Expand Up @@ -1347,7 +1347,7 @@ public async Task SubsequentWriteAsyncVerifiedAfterOnStarting()
// If OnStarting is not run before verifying writes, an error response will be sent.
await httpContext.Response.Body.WriteAsync(response, 0, response.Length / 2);
await httpContext.Response.Body.WriteAsync(response, response.Length / 2, response.Length - response.Length / 2);
}, new TestServiceContext()))
}))
{
using (var connection = server.CreateConnection())
{
Expand All @@ -1371,6 +1371,162 @@ await connection.Receive(
}
}

[Fact]
public async Task WhenResponseAlreadyStartedResponseEndedBeforeConsumingRequestBody()
{
using (var server = new TestServer(async httpContext =>
{
await httpContext.Response.WriteAsync("hello, world");
}))
{
using (var connection = server.CreateConnection())
{
await connection.Send(
"POST / HTTP/1.1",
"Content-Length: 1",
"",
"");

await connection.Receive(
"HTTP/1.1 200 OK",
$"Date: {server.Context.DateHeaderValue}",
$"Transfer-Encoding: chunked",
"",
"c",
"hello, world",
"");

// If the expected behavior is regressed, this will hang because the
// server will try to consume the request body before flushing the chunked
// terminator.
await connection.Receive(
Copy link
Member

Choose a reason for hiding this comment

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

Did you split this into two calls to connection.Receive() just to insert the above comment?

Copy link
Contributor Author

@cesarblum cesarblum Mar 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

No. It's to make it clear that the first part will be received fine, but if there's a regression the connection will time out waiting for the chunk terminator.

"0",
"",
"");
}
}
}

[Fact]
public async Task WhenResponseNotStartedResponseEndedAfterConsumingRequestBody()
{
using (var server = new TestServer(httpContext => TaskCache.CompletedTask))
{
using (var connection = server.CreateConnection())
{
await connection.Send(
"POST / HTTP/1.1",
"Transfer-Encoding: chunked",
"",
"gg");

// If the expected behavior is regressed, this will receive
// a success response because the server flushed the response
// before reading the malformed chunk header in the request.
await connection.ReceiveForcedEnd(
"HTTP/1.1 400 Bad Request",
"Connection: close",
$"Date: {server.Context.DateHeaderValue}",
"Content-Length: 0",
"",
"");
}
}
}

[Fact]
public async Task Sending100ContinueDoesNotStartResponse()
Copy link
Member

Choose a reason for hiding this comment

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

Can we test sending the chunk terminator pre-consume when we send a 100-continue and the app sends a response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

{
using (var server = new TestServer(httpContext =>
{
return httpContext.Request.Body.ReadAsync(new byte[1], 0, 1);
}))
{
using (var connection = server.CreateConnection())
{
await connection.Send(
"POST / HTTP/1.1",
"Transfer-Encoding: chunked",
"Expect: 100-continue",
"",
"");

await connection.Receive(
"HTTP/1.1 100 Continue",
"",
"");

// Let the app finish
await connection.Send(
"1",
"a",
"");

// This will be consumed by Frame when it attempts to
// consume the request body and will cause an error.
await connection.Send(
"gg");

// If 100 Continue sets Frame.HasResponseStarted to true,
// a success response will be produced before the server sees the
// bad chunk header above, making this test fail.
await connection.ReceiveForcedEnd(
"HTTP/1.1 400 Bad Request",
"Connection: close",
$"Date: {server.Context.DateHeaderValue}",
"Content-Length: 0",
"",
"");
}
}
}

[Fact]
public async Task Sending100ContinueAndResponseSendsChunkTerminatorBeforeConsumingRequestBody()
{
using (var server = new TestServer(async httpContext =>
{
await httpContext.Request.Body.ReadAsync(new byte[1], 0, 1);
await httpContext.Response.WriteAsync("hello, world");
}))
{
using (var connection = server.CreateConnection())
{
await connection.Send(
"POST / HTTP/1.1",
"Content-Length: 2",
"Expect: 100-continue",
"",
"");

await connection.Receive(
"HTTP/1.1 100 Continue",
"",
"");

await connection.Send(
"a");

await connection.Receive(
"HTTP/1.1 200 OK",
$"Date: {server.Context.DateHeaderValue}",
$"Transfer-Encoding: chunked",
"",
"c",
"hello, world",
"");

// If the expected behavior is regressed, this will hang because the
// server will try to consume the request body before flushing the chunked
// terminator.
await connection.Receive(
"0",
"",
"");
}
}
}

public static TheoryData<string, StringValues, string> NullHeaderData
{
get
Expand Down