Don't close connections after upgrade requests without a 101 response

halter73 · halter73 · commit 08c46ccb6183 · 2020-09-30T15:18:55.000-07:00
diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ChunkedEncodingMessageBody.cs
@@ -30,10 +30,9 @@ internal sealed class Http1ChunkedEncodingMessageBody : Http1MessageBody
         private readonly Pipe _requestBodyPipe;
         private ReadResult _readResult;
 
-        public Http1ChunkedEncodingMessageBody(bool keepAlive, Http1Connection context)
-            : base(context)
+        public Http1ChunkedEncodingMessageBody(Http1Connection context, bool keepAlive)
+            : base(context, keepAlive)
         {
-            RequestKeepAlive = keepAlive;
             _requestBodyPipe = CreateRequestBodyPipe(context);
         }
 
diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1ContentLengthMessageBody.cs
@@ -10,8 +10,6 @@
 
 namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
 {
-    using BadHttpRequestException = Microsoft.AspNetCore.Http.BadHttpRequestException;
-
     internal sealed class Http1ContentLengthMessageBody : Http1MessageBody
     {
         private ReadResult _readResult;
@@ -23,12 +21,11 @@ internal sealed class Http1ContentLengthMessageBody : Http1MessageBody
         private bool _finalAdvanceCalled;
         private bool _cannotResetInputPipe;
 
-        public Http1ContentLengthMessageBody(bool keepAlive, long contentLength, Http1Connection context)
-            : base(context)
+        public Http1ContentLengthMessageBody(Http1Connection context, long contentLength, bool keepAlive)
+            : base(context, keepAlive)
         {
-            RequestKeepAlive = keepAlive;
             _contentLength = contentLength;
-            _unexaminedInputLength = _contentLength;
+            _unexaminedInputLength = contentLength;
         }
 
         public override async ValueTask<ReadResult> ReadAsyncInternal(CancellationToken cancellationToken = default)
diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs
@@ -18,9 +18,10 @@ internal abstract class Http1MessageBody : MessageBody
         protected readonly Http1Connection _context;
         private bool _readerCompleted;
 
-        protected Http1MessageBody(Http1Connection context) : base(context)
+        protected Http1MessageBody(Http1Connection context, bool keepAlive) : base(context)
         {
             _context = context;
+            RequestKeepAlive = keepAlive;
         }
 
         public override ValueTask<ReadResult> ReadAsync(CancellationToken cancellationToken = default)
@@ -123,14 +124,15 @@ public static MessageBody For(
         {
             // see also http://tools.ietf.org/html/rfc2616#section-4.4
             var keepAlive = httpVersion != HttpVersion.Http10;
-
             var upgrade = false;
+
             if (headers.HasConnection)
             {
                 var connectionOptions = HttpHeaders.ParseConnection(headers.HeaderConnection);
 
                 upgrade = (connectionOptions & ConnectionOptions.Upgrade) != 0;
-                keepAlive = (connectionOptions & ConnectionOptions.KeepAlive) != 0;
+                keepAlive = keepAlive || (connectionOptions & ConnectionOptions.KeepAlive) != 0;
+                keepAlive = keepAlive && (connectionOptions & ConnectionOptions.Close) == 0;
             }
 
             if (upgrade)
@@ -141,7 +143,7 @@ public static MessageBody For(
                 }
 
                 context.OnTrailersComplete(); // No trailers for these.
-                return new Http1UpgradeMessageBody(context);
+                return new Http1UpgradeMessageBody(context, keepAlive);
             }
 
             if (headers.HasTransferEncoding)
@@ -162,7 +164,7 @@ public static MessageBody For(
 
                 // TODO may push more into the wrapper rather than just calling into the message body
                 // NBD for now.
-                return new Http1ChunkedEncodingMessageBody(keepAlive, context);
+                return new Http1ChunkedEncodingMessageBody(context, keepAlive);
             }
 
             if (headers.ContentLength.HasValue)
@@ -174,7 +176,7 @@ public static MessageBody For(
                     return keepAlive ? MessageBody.ZeroContentLengthKeepAlive : MessageBody.ZeroContentLengthClose;
                 }
 
-                return new Http1ContentLengthMessageBody(keepAlive, contentLength, context);
+                return new Http1ContentLengthMessageBody(context, contentLength, keepAlive);
             }
 
             // If we got here, request contains no Content-Length or Transfer-Encoding header.
diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1UpgradeMessageBody.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1UpgradeMessageBody.cs
@@ -14,8 +14,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http
     /// </summary>
     internal sealed class Http1UpgradeMessageBody : Http1MessageBody
     {
-        public Http1UpgradeMessageBody(Http1Connection context)
-            : base(context)
+        public Http1UpgradeMessageBody(Http1Connection context, bool keepAlive)
+            : base(context, keepAlive)
         {
             RequestUpgrade = true;
         }
diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs b/src/Servers/Kestrel/Core/src/Internal/Http/HttpProtocol.cs
@@ -1105,13 +1105,13 @@ private HttpResponseHeaders CreateResponseHeaders(bool appCompleted)
             {
                 RejectNonBodyTransferEncodingResponse(appCompleted);
             }
+            else if (StatusCode == StatusCodes.Status101SwitchingProtocols)
+            {
+                _keepAlive = false;
+            }
             else if (!hasTransferEncoding && !responseHeaders.ContentLength.HasValue)
             {
-                if (StatusCode == StatusCodes.Status101SwitchingProtocols)
-                {
-                    _keepAlive = false;
-                }
-                else if ((appCompleted || !_canWriteResponseBody) && !_hasAdvanced) // Avoid setting contentLength of 0 if we wrote data before calling CreateResponseHeaders
+                if ((appCompleted || !_canWriteResponseBody) && !_hasAdvanced) // Avoid setting contentLength of 0 if we wrote data before calling CreateResponseHeaders
                 {
                     // Don't set the Content-Length header automatically for HEAD requests, 204 responses, or 304 responses.
                     if (CanAutoSetContentLengthZeroResponseHeader())
diff --git a/src/Servers/Kestrel/perf/Kestrel.Performance/Http1ReadingBenchmark.cs b/src/Servers/Kestrel/perf/Kestrel.Performance/Http1ReadingBenchmark.cs
@@ -12,6 +12,7 @@
 using Microsoft.AspNetCore.Server.Kestrel.Core;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http;
+using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2;
 using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure;
 using Microsoft.AspNetCore.Testing;
 
@@ -112,7 +113,7 @@ private TestHttp1Connection MakeHttp1Connection()
             });
 
             http1Connection.Reset();
-            http1Connection.InitializeBodyControl(new Http1ContentLengthMessageBody(keepAlive: true, 100, http1Connection));
+            http1Connection.InitializeBodyControl(new Http1ContentLengthMessageBody(http1Connection, contentLength: 100, keepAlive: true));
             serviceContext.DateHeaderValueManager.OnHeartbeat(DateTimeOffset.UtcNow);
 
             return http1Connection;

Original file line number	Diff line number	Diff line change
`@@ -30,10 +30,9 @@ internal sealed class Http1ChunkedEncodingMessageBody : Http1MessageBody`
`30`	`30`	`private readonly Pipe _requestBodyPipe;`
`31`	`31`	`private ReadResult _readResult;`
`32`	`32`
`33`		`- public Http1ChunkedEncodingMessageBody(bool keepAlive, Http1Connection context)`
`34`		`- : base(context)`
	`33`	`+ public Http1ChunkedEncodingMessageBody(Http1Connection context, bool keepAlive)`
	`34`	`+ : base(context, keepAlive)`
`35`	`35`	`{`
`36`		`- RequestKeepAlive = keepAlive;`
`37`	`36`	`_requestBodyPipe = CreateRequestBodyPipe(context);`
`38`	`37`	`}`
`39`	`38`
Original file line number	Diff line number	Diff line change
`@@ -10,8 +10,6 @@`
`10`	`10`
`11`	`11`	`namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http`
`12`	`12`	`{`
`13`		`- using BadHttpRequestException = Microsoft.AspNetCore.Http.BadHttpRequestException;`
`14`		`-`
`15`	`13`	`internal sealed class Http1ContentLengthMessageBody : Http1MessageBody`
`16`	`14`	`{`
`17`	`15`	`private ReadResult _readResult;`
`@@ -23,12 +21,11 @@ internal sealed class Http1ContentLengthMessageBody : Http1MessageBody`
`23`	`21`	`private bool _finalAdvanceCalled;`
`24`	`22`	`private bool _cannotResetInputPipe;`
`25`	`23`
`26`		`- public Http1ContentLengthMessageBody(bool keepAlive, long contentLength, Http1Connection context)`
`27`		`- : base(context)`
	`24`	`+ public Http1ContentLengthMessageBody(Http1Connection context, long contentLength, bool keepAlive)`
	`25`	`+ : base(context, keepAlive)`
`28`	`26`	`{`
`29`		`- RequestKeepAlive = keepAlive;`
`30`	`27`	`_contentLength = contentLength;`
`31`		`- _unexaminedInputLength = _contentLength;`
	`28`	`+ _unexaminedInputLength = contentLength;`
`32`	`29`	`}`
`33`	`30`
`34`	`31`	`public override async ValueTask<ReadResult> ReadAsyncInternal(CancellationToken cancellationToken = default)`
Original file line number	Diff line number	Diff line change
`@@ -18,9 +18,10 @@ internal abstract class Http1MessageBody : MessageBody`
`18`	`18`	`protected readonly Http1Connection _context;`
`19`	`19`	`private bool _readerCompleted;`
`20`	`20`
`21`		`- protected Http1MessageBody(Http1Connection context) : base(context)`
	`21`	`+ protected Http1MessageBody(Http1Connection context, bool keepAlive) : base(context)`
`22`	`22`	`{`
`23`	`23`	`_context = context;`
	`24`	`+ RequestKeepAlive = keepAlive;`
`24`	`25`	`}`
`25`	`26`
`26`	`27`	`public override ValueTask<ReadResult> ReadAsync(CancellationToken cancellationToken = default)`
`@@ -123,14 +124,15 @@ public static MessageBody For(`
`123`	`124`	`{`
`124`	`125`	`// see also http://tools.ietf.org/html/rfc2616#section-4.4`
`125`	`126`	`var keepAlive = httpVersion != HttpVersion.Http10;`
`126`		`-`
`127`	`127`	`var upgrade = false;`
	`128`	`+`
`128`	`129`	`if (headers.HasConnection)`
`129`	`130`	`{`
`130`	`131`	`var connectionOptions = HttpHeaders.ParseConnection(headers.HeaderConnection);`
`131`	`132`
`132`	`133`	`upgrade = (connectionOptions & ConnectionOptions.Upgrade) != 0;`
`133`		`- keepAlive = (connectionOptions & ConnectionOptions.KeepAlive) != 0;`
	`134`	`+ keepAlive = keepAlive \|\| (connectionOptions & ConnectionOptions.KeepAlive) != 0;`
	`135`	`+ keepAlive = keepAlive && (connectionOptions & ConnectionOptions.Close) == 0;`
`134`	`136`	`}`
`135`	`137`
`136`	`138`	`if (upgrade)`
`@@ -141,7 +143,7 @@ public static MessageBody For(`
`141`	`143`	`}`
`142`	`144`
`143`	`145`	`context.OnTrailersComplete(); // No trailers for these.`
`144`		`- return new Http1UpgradeMessageBody(context);`
	`146`	`+ return new Http1UpgradeMessageBody(context, keepAlive);`
`145`	`147`	`}`
`146`	`148`
`147`	`149`	`if (headers.HasTransferEncoding)`
`@@ -162,7 +164,7 @@ public static MessageBody For(`
`162`	`164`
`163`	`165`	`// TODO may push more into the wrapper rather than just calling into the message body`
`164`	`166`	`// NBD for now.`
`165`		`- return new Http1ChunkedEncodingMessageBody(keepAlive, context);`
	`167`	`+ return new Http1ChunkedEncodingMessageBody(context, keepAlive);`
`166`	`168`	`}`
`167`	`169`
`168`	`170`	`if (headers.ContentLength.HasValue)`
`@@ -174,7 +176,7 @@ public static MessageBody For(`
`174`	`176`	`return keepAlive ? MessageBody.ZeroContentLengthKeepAlive : MessageBody.ZeroContentLengthClose;`
`175`	`177`	`}`
`176`	`178`
`177`		`- return new Http1ContentLengthMessageBody(keepAlive, contentLength, context);`
	`179`	`+ return new Http1ContentLengthMessageBody(context, contentLength, keepAlive);`
`178`	`180`	`}`
`179`	`181`
`180`	`182`	`// If we got here, request contains no Content-Length or Transfer-Encoding header.`
Original file line number	Diff line number	Diff line change
`@@ -14,8 +14,8 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http`
`14`	`14`	`/// </summary>`
`15`	`15`	`internal sealed class Http1UpgradeMessageBody : Http1MessageBody`
`16`	`16`	`{`
`17`		`- public Http1UpgradeMessageBody(Http1Connection context)`
`18`		`- : base(context)`
	`17`	`+ public Http1UpgradeMessageBody(Http1Connection context, bool keepAlive)`
	`18`	`+ : base(context, keepAlive)`
`19`	`19`	`{`
`20`	`20`	`RequestUpgrade = true;`
`21`	`21`	`}`
Original file line number	Diff line number	Diff line change
`@@ -1105,13 +1105,13 @@ private HttpResponseHeaders CreateResponseHeaders(bool appCompleted)`
`1105`	`1105`	`{`
`1106`	`1106`	`RejectNonBodyTransferEncodingResponse(appCompleted);`
`1107`	`1107`	`}`
	`1108`	`+ else if (StatusCode == StatusCodes.Status101SwitchingProtocols)`
	`1109`	`+ {`
	`1110`	`+ _keepAlive = false;`
	`1111`	`+ }`
`1108`	`1112`	`else if (!hasTransferEncoding && !responseHeaders.ContentLength.HasValue)`
`1109`	`1113`	`{`
`1110`		`- if (StatusCode == StatusCodes.Status101SwitchingProtocols)`
`1111`		`- {`
`1112`		`- _keepAlive = false;`
`1113`		`- }`
`1114`		`- else if ((appCompleted \|\| !_canWriteResponseBody) && !_hasAdvanced) // Avoid setting contentLength of 0 if we wrote data before calling CreateResponseHeaders`
	`1114`	`+ if ((appCompleted \|\| !_canWriteResponseBody) && !_hasAdvanced) // Avoid setting contentLength of 0 if we wrote data before calling CreateResponseHeaders`
`1115`	`1115`	`{`
`1116`	`1116`	`// Don't set the Content-Length header automatically for HEAD requests, 204 responses, or 304 responses.`
`1117`	`1117`	`if (CanAutoSetContentLengthZeroResponseHeader())`