huahaofeng
/
aspnetcore
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

#2102 Always start the response before draining the request.

This commit is contained in:
Chris Ross (ASP.NET) 2017-10-10 16:26:22 -07:00
parent 9a8dd6ef12
commit 420500e2a9
4 changed files with 104 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
src/Kestrel.Core/Internal/Http/HttpProtocol.cs
View File

@ -517,19 +517,19 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
// If _requestAbort is set, the connection has already been closed. // If _requestAbort is set, the connection has already been closed.
if (_requestAborted == 0) if (_requestAborted == 0)
{ {
if (HasResponseStarted) // Call ProduceEnd() before consuming the rest of the request body to prevent
{ // delaying clients waiting for the chunk terminator:
// If the response has already started, call ProduceEnd() before //
// consuming the rest of the request body to prevent // https://github.com/dotnet/corefx/issues/17330#issuecomment-288248663
// delaying clients waiting for the chunk terminator: //
// // This also prevents the 100 Continue response from being sent if the app
// https://github.com/dotnet/corefx/issues/17330#issuecomment-288248663 // 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 // ProduceEnd() must be called before _application.DisposeContext(), to ensure
// IHttpContextFactory.Dispose(HttpContext) is called. // HttpContext.Response.StatusCode is correctly set when
await ProduceEnd(); // IHttpContextFactory.Dispose(HttpContext) is called.
} await ProduceEnd();
// ForZeroContentLength does not complete the reader nor the writer // ForZeroContentLength does not complete the reader nor the writer
if (!messageBody.IsEmpty && _keepAlive) if (!messageBody.IsEmpty && _keepAlive)
@ -537,11 +537,6 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
// Finish reading the request body in case the app did not. // Finish reading the request body in case the app did not.
await messageBody.ConsumeAsync(); await messageBody.ConsumeAsync();
} }
if (!HasResponseStarted)
{
await ProduceEnd();
}
} }
else if (!HasResponseStarted) else if (!HasResponseStarted)
{ {

3
test/Kestrel.FunctionalTests/MaxRequestBodySizeTests.cs
View File

@ -2,6 +2,7 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
using System; using System;
using System.IO;
using System.Text; using System.Text;
using System.Threading.Tasks; using System.Threading.Tasks;
using Microsoft.AspNetCore.Http.Features; using Microsoft.AspNetCore.Http.Features;
@ -140,7 +141,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
[Fact] [Fact]
public async Task DoesNotRejectBodylessGetRequestWithZeroMaxRequestBodySize() public async Task DoesNotRejectBodylessGetRequestWithZeroMaxRequestBodySize()
{ {
using (var server = new TestServer(context => Task.CompletedTask, using (var server = new TestServer(context => context.Request.Body.CopyToAsync(Stream.Null),
new TestServiceContext { ServerOptions = { Limits = { MaxRequestBodySize = 0 } } })) new TestServiceContext { ServerOptions = { Limits = { MaxRequestBodySize = 0 } } }))
{ {
using (var connection = server.CreateConnection()) using (var connection = server.CreateConnection())

16
test/Kestrel.FunctionalTests/RequestBodyTimeoutTests.cs
View File

@ -7,9 +7,12 @@ using System.Threading.Tasks;
using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Server.Kestrel.Core; using Microsoft.AspNetCore.Server.Kestrel.Core;
using Microsoft.AspNetCore.Server.Kestrel.Core.Features; using Microsoft.AspNetCore.Server.Kestrel.Core.Features;
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal;
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;
using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure; using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
using Microsoft.AspNetCore.Testing; using Microsoft.AspNetCore.Testing;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Logging.Testing;
using Xunit; using Xunit;
namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
@ -66,12 +69,16 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
[Fact] [Fact]
public async Task RequestTimesOutWhenNotDrainedWithinDrainTimeoutPeriod() public async Task RequestTimesOutWhenNotDrainedWithinDrainTimeoutPeriod()
{ {
var sink = new TestSink();
var logger = new TestLogger("TestLogger", sink, enabled: true);
// This test requires a real clock since we can't control when the drain timeout is set // This test requires a real clock since we can't control when the drain timeout is set
var systemClock = new SystemClock(); var systemClock = new SystemClock();
var serviceContext = new TestServiceContext var serviceContext = new TestServiceContext
{ {
SystemClock = systemClock, SystemClock = systemClock,
DateHeaderValueManager = new DateHeaderValueManager(systemClock) DateHeaderValueManager = new DateHeaderValueManager(systemClock),
Log = new KestrelTrace(logger)
}; };
var appRunningEvent = new ManualResetEventSlim(); var appRunningEvent = new ManualResetEventSlim();
@ -96,17 +103,20 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
Assert.True(appRunningEvent.Wait(TimeSpan.FromSeconds(10))); Assert.True(appRunningEvent.Wait(TimeSpan.FromSeconds(10)));
await connection.Receive( await connection.Receive(
"HTTP/1.1 408 Request Timeout", "HTTP/1.1 200 OK",
"Connection: close",
""); "");
await connection.ReceiveStartsWith( await connection.ReceiveStartsWith(
"Date: "); "Date: ");
// Disconnected due to the timeout
await connection.ReceiveForcedEnd( await connection.ReceiveForcedEnd(
"Content-Length: 0", "Content-Length: 0",
"", "",
""); "");
} }
} }
Assert.Contains(sink.Writes, w => w.EventId.Id == 17 && w.LogLevel == LogLevel.Information && w.Exception is BadHttpRequestException
&& ((BadHttpRequestException)w.Exception).StatusCode == StatusCodes.Status408RequestTimeout);
} }
[Fact] [Fact]

108
test/Kestrel.FunctionalTests/ResponseTests.cs
View File

@ -257,8 +257,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
{ {
return Task.CompletedTask; return Task.CompletedTask;
}, },
expectedClientStatusCode: null, expectedClientStatusCode: HttpStatusCode.OK,
expectedServerStatusCode: HttpStatusCode.BadRequest, expectedServerStatusCode: HttpStatusCode.OK,
sendMalformedRequest: true); sendMalformedRequest: true);
} }
@ -289,8 +289,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
{ {
} }
}, },
expectedClientStatusCode: null, expectedClientStatusCode: HttpStatusCode.OK,
expectedServerStatusCode: HttpStatusCode.BadRequest, expectedServerStatusCode: HttpStatusCode.OK,
sendMalformedRequest: true); sendMalformedRequest: true);
} }
@ -311,8 +311,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
disposedTcs.TrySetResult(c.Response.StatusCode); disposedTcs.TrySetResult(c.Response.StatusCode);
}); });
using (var server = new TestServer(handler, new TestServiceContext(), new ListenOptions(new IPEndPoint(IPAddress.Loopback, 0)), var sink = new TestSink();
services => services.AddSingleton(mockHttpContextFactory.Object))) var logger = new TestLogger("TestLogger", sink, enabled: true);
using (var server = new TestServer(handler, new TestServiceContext() { Log = new KestrelTrace(logger) },
new ListenOptions(new IPEndPoint(IPAddress.Loopback, 0)),
services => services.AddSingleton(mockHttpContextFactory.Object)))
{ {
if (!sendMalformedRequest) if (!sendMalformedRequest)
{ {
@ -342,32 +346,57 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
"Transfer-Encoding: chunked", "Transfer-Encoding: chunked",
"", "",
"gg"); "gg");
await connection.ReceiveForcedEnd( if (expectedClientStatusCode == HttpStatusCode.OK)
"HTTP/1.1 400 Bad Request", {
"Connection: close", await connection.ReceiveForcedEnd(
$"Date: {server.Context.DateHeaderValue}", "HTTP/1.1 200 OK",
"Content-Length: 0", $"Date: {server.Context.DateHeaderValue}",
"", "Content-Length: 0",
""); "",
"");
}
else
{
await connection.ReceiveForcedEnd(
"HTTP/1.1 400 Bad Request",
"Connection: close",
$"Date: {server.Context.DateHeaderValue}",
"Content-Length: 0",
"",
"");
}
} }
} }
var disposedStatusCode = await disposedTcs.Task.TimeoutAfter(TimeSpan.FromSeconds(10)); var disposedStatusCode = await disposedTcs.Task.TimeoutAfter(TimeSpan.FromSeconds(10));
Assert.Equal(expectedServerStatusCode, (HttpStatusCode)disposedStatusCode); Assert.Equal(expectedServerStatusCode, (HttpStatusCode)disposedStatusCode);
} }
if (sendMalformedRequest)
{
Assert.Contains(sink.Writes, w => w.EventId.Id == 17 && w.LogLevel == LogLevel.Information && w.Exception is BadHttpRequestException
&& ((BadHttpRequestException)w.Exception).StatusCode == StatusCodes.Status400BadRequest);
}
else
{
Assert.DoesNotContain(sink.Writes, w => w.EventId.Id == 17 && w.LogLevel == LogLevel.Information && w.Exception is BadHttpRequestException
&& ((BadHttpRequestException)w.Exception).StatusCode == StatusCodes.Status400BadRequest);
}
} }
// https://github.com/aspnet/KestrelHttpServer/pull/1111/files#r80584475 explains the reason for this test. // https://github.com/aspnet/KestrelHttpServer/pull/1111/files#r80584475 explains the reason for this test.
[Fact] [Fact]
public async Task SingleErrorResponseSentWhenAppSwallowsBadRequestException() public async Task NoErrorResponseSentWhenAppSwallowsBadRequestException()
{ {
BadHttpRequestException readException = null; BadHttpRequestException readException = null;
var sink = new TestSink();
var logger = new TestLogger("TestLogger", sink, enabled: true);
using (var server = new TestServer(async httpContext => using (var server = new TestServer(async httpContext =>
{ {
readException = await Assert.ThrowsAsync<BadHttpRequestException>( readException = await Assert.ThrowsAsync<BadHttpRequestException>(
async () => await httpContext.Request.Body.ReadAsync(new byte[1], 0, 1)); async () => await httpContext.Request.Body.ReadAsync(new byte[1], 0, 1));
})) }, new TestServiceContext() { Log = new KestrelTrace(logger) }))
{ {
using (var connection = server.CreateConnection()) using (var connection = server.CreateConnection())
{ {
@ -378,8 +407,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
"", "",
"gg"); "gg");
await connection.ReceiveForcedEnd( await connection.ReceiveForcedEnd(
"HTTP/1.1 400 Bad Request", "HTTP/1.1 200 OK",
"Connection: close",
$"Date: {server.Context.DateHeaderValue}", $"Date: {server.Context.DateHeaderValue}",
"Content-Length: 0", "Content-Length: 0",
"", "",
@ -388,6 +416,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
} }
Assert.NotNull(readException); Assert.NotNull(readException);
Assert.Contains(sink.Writes, w => w.EventId.Id == 17 && w.LogLevel == LogLevel.Information && w.Exception is BadHttpRequestException
&& ((BadHttpRequestException)w.Exception).StatusCode == StatusCodes.Status400BadRequest);
} }
[Fact] [Fact]
@ -1474,9 +1505,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
} }
[Fact] [Fact]
public async Task WhenResponseNotStartedResponseEndedAfterConsumingRequestBody() public async Task WhenResponseNotStartedResponseEndedBeforeConsumingRequestBody()
{ {
using (var server = new TestServer(httpContext => Task.CompletedTask)) var sink = new TestSink();
var logger = new TestLogger("TestLogger", sink, enabled: true);
using (var server = new TestServer(httpContext => Task.CompletedTask,
new TestServiceContext() { Log = new KestrelTrace(logger) }))
{ {
using (var connection = server.CreateConnection()) using (var connection = server.CreateConnection())
{ {
@ -1487,27 +1522,32 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
"", "",
"gg"); "gg");
// If the expected behavior is regressed, this will receive // This will receive a success response because the server flushed the response
// a success response because the server flushed the response // before reading the malformed chunk header in the request, but then it will close
// before reading the malformed chunk header in the request. // the connection.
await connection.ReceiveForcedEnd( await connection.ReceiveForcedEnd(
"HTTP/1.1 400 Bad Request", "HTTP/1.1 200 OK",
"Connection: close",
$"Date: {server.Context.DateHeaderValue}", $"Date: {server.Context.DateHeaderValue}",
"Content-Length: 0", "Content-Length: 0",
"", "",
""); "");
} }
} }
Assert.Contains(sink.Writes, w => w.EventId.Id == 17 && w.LogLevel == LogLevel.Information && w.Exception is BadHttpRequestException
&& ((BadHttpRequestException)w.Exception).StatusCode == StatusCodes.Status400BadRequest);
} }
[Fact] [Fact]
public async Task Sending100ContinueDoesNotStartResponse() public async Task Sending100ContinueDoesNotStartResponse()
{ {
var sink = new TestSink();
var logger = new TestLogger("TestLogger", sink, enabled: true);
using (var server = new TestServer(httpContext => using (var server = new TestServer(httpContext =>
{ {
return httpContext.Request.Body.ReadAsync(new byte[1], 0, 1); return httpContext.Request.Body.ReadAsync(new byte[1], 0, 1);
})) }, new TestServiceContext() { Log = new KestrelTrace(logger) }))
{ {
using (var connection = server.CreateConnection()) using (var connection = server.CreateConnection())
{ {
@ -1530,6 +1570,13 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
"a", "a",
""); "");
await connection.Receive(
"HTTP/1.1 200 OK",
$"Date: {server.Context.DateHeaderValue}",
"Content-Length: 0",
"",
"");
// This will be consumed by Http1Connection when it attempts to // This will be consumed by Http1Connection when it attempts to
// consume the request body and will cause an error. // consume the request body and will cause an error.
await connection.Send( await connection.Send(
@ -1538,15 +1585,12 @@ namespace Microsoft.AspNetCore.Server.Kestrel.FunctionalTests
// If 100 Continue sets HttpProtocol.HasResponseStarted to true, // If 100 Continue sets HttpProtocol.HasResponseStarted to true,
// a success response will be produced before the server sees the // a success response will be produced before the server sees the
// bad chunk header above, making this test fail. // bad chunk header above, making this test fail.
await connection.ReceiveForcedEnd( await connection.ReceiveEnd();
"HTTP/1.1 400 Bad Request",
"Connection: close",
$"Date: {server.Context.DateHeaderValue}",
"Content-Length: 0",
"",
"");
} }
} }
Assert.Contains(sink.Writes, w => w.EventId.Id == 17 && w.LogLevel == LogLevel.Information && w.Exception is BadHttpRequestException
&& ((BadHttpRequestException)w.Exception).StatusCode == StatusCodes.Status400BadRequest);
} }
[Fact] [Fact]