From 69759231ff8aff3fda5056ee58be0d5c729a6bd7 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Tue, 25 Aug 2015 23:07:03 -0700 Subject: [PATCH 1/2] Set Content-Length: 0 when an AppFunc completes without a write - Previously an incomplete chunked response would be written instead. - Add test to verify Content-Length: 0 is set automatically. - Add test to verify HTTP/1.0 keep-alive isn't used if no Content-Length is set for the response. - Add tests to verify errors are handled properly after chunked writes. #173 --- .../Http/Frame.cs | 30 ++++--- .../ChunkedResponseTests.cs | 81 +++++++++++++++++++ .../EngineTests.cs | 54 +++++++++++++ 3 files changed, 156 insertions(+), 9 deletions(-) diff --git a/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs b/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs index 43bed0797..a78d7f7ea 100644 --- a/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs +++ b/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs @@ -254,11 +254,6 @@ private async Task ExecuteAsync() { FireOnStarting(); } - - if (_autoChunk) - { - WriteChunkedResponseSuffix(); - } } catch (Exception ex) { @@ -380,7 +375,7 @@ public void ProduceContinue() } } - public void ProduceStart(bool immediate = true) + public void ProduceStart(bool immediate = true, bool appCompleted = false) { // ProduceStart shouldn't no-op in the future just b/c FireOnStarting throws. if (_responseStarted) return; @@ -389,7 +384,7 @@ public void ProduceStart(bool immediate = true) var status = ReasonPhrases.ToStatus(StatusCode, ReasonPhrase); - var responseHeader = CreateResponseHeader(status, ResponseHeaders); + var responseHeader = CreateResponseHeader(status, appCompleted, ResponseHeaders); SocketOutput.Write( responseHeader.Item1, (error, x) => @@ -428,7 +423,14 @@ public void ProduceEnd(Exception ex) } } - ProduceStart(); + ProduceStart(immediate: true, appCompleted: true); + + // _autoChunk should be checked after we are sure ProduceStart() has been called + // since ProduceStart() may set _autoChunk to true. + if (_autoChunk) + { + WriteChunkedResponseSuffix(); + } if (!_keepAlive) { @@ -440,7 +442,9 @@ public void ProduceEnd(Exception ex) } private Tuple, IDisposable> CreateResponseHeader( - string status, IEnumerable> headers) + string status, + bool appCompleted, + IEnumerable> headers) { var writer = new MemoryPoolTextWriter(Memory); writer.Write(HttpVersion); @@ -490,6 +494,14 @@ private Tuple, IDisposable> CreateResponseHeader( } } + if (appCompleted && !hasTransferEncoding && !hasContentLength) + { + // Since the app has completed and we are only now generating + // the headers we can safely set the Content-Length to 0. + writer.Write("Content-Length: 0\r\n"); + hasContentLength = true; + } + if (_keepAlive && !hasTransferEncoding && !hasContentLength) { if (HttpVersion == "HTTP/1.1") diff --git a/test/Microsoft.AspNet.Server.KestrelTests/ChunkedResponseTests.cs b/test/Microsoft.AspNet.Server.KestrelTests/ChunkedResponseTests.cs index a1b7ac186..162a29263 100644 --- a/test/Microsoft.AspNet.Server.KestrelTests/ChunkedResponseTests.cs +++ b/test/Microsoft.AspNet.Server.KestrelTests/ChunkedResponseTests.cs @@ -1,6 +1,7 @@ // 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.Text; using System.Threading.Tasks; using Xunit; @@ -71,5 +72,85 @@ await connection.ReceiveEnd( } } } + + [Fact] + public async Task EmptyResponseBodyHandledCorrectlyWithZeroLengthWrite() + { + using (var server = new TestServer(async frame => + { + frame.ResponseHeaders.Clear(); + await frame.ResponseBody.WriteAsync(new byte[0], 0, 0); + })) + { + using (var connection = new TestConnection()) + { + await connection.SendEnd( + "GET / HTTP/1.1", + "", + ""); + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Transfer-Encoding: chunked", + "", + "0", + "", + ""); + } + } + } + + [Fact] + public async Task ConnectionClosedIfExeptionThrownAfterWrite() + { + using (var server = new TestServer(async frame => + { + frame.ResponseHeaders.Clear(); + await frame.ResponseBody.WriteAsync(Encoding.ASCII.GetBytes("Hello World!"), 0, 12); + throw new Exception(); + })) + { + using (var connection = new TestConnection()) + { + // SendEnd is not called, so it isn't the client closing the connection. + // client closing the connection. + await connection.Send( + "GET / HTTP/1.1", + "", + ""); + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Transfer-Encoding: chunked", + "", + "c", + "Hello World!", + ""); + } + } + } + + [Fact] + public async Task ConnectionClosedIfExeptionThrownAfterZeroLengthWrite() + { + using (var server = new TestServer(async frame => + { + frame.ResponseHeaders.Clear(); + await frame.ResponseBody.WriteAsync(new byte[0], 0, 0); + throw new Exception(); + })) + { + using (var connection = new TestConnection()) + { + // SendEnd is not called, so it isn't the client closing the connection. + await connection.Send( + "GET / HTTP/1.1", + "", + ""); + + // Nothing (not even headers) are written, but the connection is closed. + await connection.ReceiveEnd(); + } + } + } } } + diff --git a/test/Microsoft.AspNet.Server.KestrelTests/EngineTests.cs b/test/Microsoft.AspNet.Server.KestrelTests/EngineTests.cs index 27526edd8..d36d02aee 100644 --- a/test/Microsoft.AspNet.Server.KestrelTests/EngineTests.cs +++ b/test/Microsoft.AspNet.Server.KestrelTests/EngineTests.cs @@ -235,6 +235,35 @@ await connection.ReceiveEnd( } } + [Fact] + public async Task Http10KeepAliveNotUsedIfResponseContentLengthNotSet() + { + using (var server = new TestServer(App)) + { + using (var connection = new TestConnection()) + { + await connection.SendEnd( + "GET / HTTP/1.0", + "Connection: keep-alive", + "", + "POST / HTTP/1.0", + "Connection: keep-alive", + "Content-Length: 7", + "", + "Goodbye"); + await connection.Receive( + "HTTP/1.0 200 OK", + "Content-Length: 0", + "Connection: keep-alive", + "\r\n"); + await connection.ReceiveEnd( + "HTTP/1.0 200 OK", + "", + "Goodbye"); + } + } + } + [Fact] public async Task Http10KeepAliveContentLength() { @@ -341,11 +370,36 @@ await connection.SendEnd( "\r\n"); await connection.ReceiveEnd( "HTTP/1.0 200 OK", + "Content-Length: 0", "\r\n"); } } } + [Fact] + public async Task EmptyResponseBodyHandledCorrectlyWithoutAnyWrites() + { + using (var server = new TestServer(frame => + { + frame.ResponseHeaders.Clear(); + return Task.FromResult(null); + })) + { + using (var connection = new TestConnection()) + { + await connection.SendEnd( + "GET / HTTP/1.1", + "", + ""); + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Content-Length: 0", + "", + ""); + } + } + } + [Fact] public async Task ThrowingResultsIn500Response() { From 2642c84bf99190eed5f454e6c37b88b913dbf596 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Wed, 26 Aug 2015 12:41:14 -0700 Subject: [PATCH 2/2] Don't automatically set Content-Length: 0 in some circumstances - When in response to a HEAD Request - For 101, 204, 205 and 304 responses - For non keep-alive connections --- .../Http/Frame.cs | 40 ++++-- .../EngineTests.cs | 122 +++++++++++++++++- 2 files changed, 144 insertions(+), 18 deletions(-) diff --git a/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs b/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs index a78d7f7ea..d5dc95808 100644 --- a/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs +++ b/src/Microsoft.AspNet.Server.Kestrel/Http/Frame.cs @@ -494,26 +494,33 @@ private Tuple, IDisposable> CreateResponseHeader( } } - if (appCompleted && !hasTransferEncoding && !hasContentLength) - { - // Since the app has completed and we are only now generating - // the headers we can safely set the Content-Length to 0. - writer.Write("Content-Length: 0\r\n"); - hasContentLength = true; - } - if (_keepAlive && !hasTransferEncoding && !hasContentLength) { - if (HttpVersion == "HTTP/1.1") + if (appCompleted) { - _autoChunk = true; - writer.Write("Transfer-Encoding: chunked\r\n"); + // 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)) + { + // Since the app has completed and we are only now generating + // the headers we can safely set the Content-Length to 0. + writer.Write("Content-Length: 0\r\n"); + } } else { - _keepAlive = false; + if (HttpVersion == "HTTP/1.1") + { + _autoChunk = true; + writer.Write("Transfer-Encoding: chunked\r\n"); + } + else + { + _keepAlive = false; + } } } + if (_keepAlive == false && hasConnection == false && HttpVersion == "HTTP/1.1") { writer.Write("Connection: close\r\n\r\n"); @@ -673,5 +680,14 @@ private void AddRequestHeader(byte[] keyBytes, int keyOffset, int keyLength, str { _requestHeaders.Append(keyBytes, keyOffset, keyLength, value); } + + public bool StatusCanHaveBody(int statusCode) + { + // List of status codes taken from Microsoft.Net.Http.Server.Response + return statusCode != 101 && + statusCode != 204 && + statusCode != 205 && + statusCode != 304; + } } } diff --git a/test/Microsoft.AspNet.Server.KestrelTests/EngineTests.cs b/test/Microsoft.AspNet.Server.KestrelTests/EngineTests.cs index d36d02aee..a293e8bae 100644 --- a/test/Microsoft.AspNet.Server.KestrelTests/EngineTests.cs +++ b/test/Microsoft.AspNet.Server.KestrelTests/EngineTests.cs @@ -77,6 +77,12 @@ private async Task AppChunked(Frame frame) await frame.ResponseBody.WriteAsync(bytes, 0, bytes.Length); } + private Task EmptyApp(Frame frame) + { + frame.ResponseHeaders.Clear(); + return Task.FromResult(null); + } + [Fact] public void EngineCanStartAndStop() { @@ -370,28 +376,132 @@ await connection.SendEnd( "\r\n"); await connection.ReceiveEnd( "HTTP/1.0 200 OK", - "Content-Length: 0", "\r\n"); } } } [Fact] - public async Task EmptyResponseBodyHandledCorrectlyWithoutAnyWrites() + public async Task ZeroContentLengthSetAutomaticallyAfterNoWrites() { - using (var server = new TestServer(frame => + using (var server = new TestServer(EmptyApp)) { - frame.ResponseHeaders.Clear(); - return Task.FromResult(null); - })) + using (var connection = new TestConnection()) + { + await connection.SendEnd( + "GET / HTTP/1.1", + "", + "GET / HTTP/1.0", + "Connection: keep-alive", + "", + ""); + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Content-Length: 0", + "", + "HTTP/1.0 200 OK", + "Content-Length: 0", + "Connection: keep-alive", + "", + ""); + } + } + } + + [Fact] + public async Task ZeroContentLengthNotSetAutomaticallyForNonKeepAliveRequests() + { + using (var server = new TestServer(EmptyApp)) { using (var connection = new TestConnection()) { await connection.SendEnd( "GET / HTTP/1.1", + "Connection: close", + "", + ""); + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "Connection: close", + "", + ""); + } + + using (var connection = new TestConnection()) + { + await connection.SendEnd( + "GET / HTTP/1.0", "", ""); await connection.ReceiveEnd( + "HTTP/1.0 200 OK", + "", + ""); + } + } + } + + [Fact] + public async Task ZeroContentLengthNotSetAutomaticallyForHeadRequests() + { + using (var server = new TestServer(EmptyApp)) + { + using (var connection = new TestConnection()) + { + await connection.SendEnd( + "HEAD / HTTP/1.1", + "", + ""); + await connection.ReceiveEnd( + "HTTP/1.1 200 OK", + "", + ""); + } + } + } + + [Fact] + public async Task ZeroContentLengthNotSetAutomaticallyForCertainStatusCodes() + { + using (var server = new TestServer(async frame => + { + frame.ResponseHeaders.Clear(); + + using (var reader = new StreamReader(frame.RequestBody, Encoding.ASCII)) + { + var statusString = await reader.ReadLineAsync(); + frame.StatusCode = int.Parse(statusString); + } + })) + { + using (var connection = new TestConnection()) + { + await connection.SendEnd( + "POST / HTTP/1.1", + "Content-Length: 3", + "", + "101POST / HTTP/1.1", + "Content-Length: 3", + "", + "204POST / HTTP/1.1", + "Content-Length: 3", + "", + "205POST / HTTP/1.1", + "Content-Length: 3", + "", + "304POST / HTTP/1.1", + "Content-Length: 3", + "", + "200"); + await connection.ReceiveEnd( + "HTTP/1.1 101 Switching Protocols", + "", + "HTTP/1.1 204 No Content", + "", + "HTTP/1.1 205 Reset Content", + "", + "HTTP/1.1 304 Not Modified", + "", "HTTP/1.1 200 OK", "Content-Length: 0", "",