Improve implementation of IHttpUpgradeFeature #1706

natemcmaster · 2017-04-18T00:13:28Z

After upgrade has been accepted by the server:

Reads to HttpRequest.Body always return 0
Writes to HttpResponse.Body always throw
The only valid way to communicate is to use the stream returned by IHttpUpgradeFeature.UpgradeAsync()

Also, Kestrel returns HTTP 400 if requests attempt to send a request body along with Connection: Upgrade. Resolves #1570

davidfowl · 2017-04-18T05:01:10Z

.gitignore

@@ -4,6 +4,7 @@ TestResults/
 .nuget/
 *.sln.ide/
 _ReSharper.*/
+.idea/


Fix everywhere?

davidfowl · 2017-04-18T05:10:37Z

test/shared/TestConnection.cs

+        public Task ReceiveUntil(string line, StringComparison comparer = StringComparison.OrdinalIgnoreCase)
+            => ReceiveUntil(next => next.Equals(line, comparer));
+
+        public async Task ReceiveUntil(Predicate<string> predicate)


Predicate<string>, feels like 1999 😄

davidfowl · 2017-04-18T05:11:22Z

test/Microsoft.AspNetCore.Server.Kestrel.Core.Tests/FrameTests.cs

@@ -256,18 +256,15 @@ public void InitializeStreamsResetsStreams()

            var originalRequestBody = _frame.RequestBody;
            var originalResponseBody = _frame.ResponseBody;
-            var originalDuplexStream = _frame.DuplexStream;


Add some unit tests for the Streams

davidfowl · 2017-04-18T05:12:38Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/UpgradeTests.cs

+                var feature = context.Features.Get<IHttpUpgradeFeature>();
+                var stream = await feature.UpgradeAsync();
+
+                Assert.Throws<InvalidOperationException>(() => context.Response.Body.WriteByte((byte)' '));


Does this throw the exception you think it's throwing?

cesarblum · 2017-04-18T16:01:36Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/BadHttpRequestException.cs

@@ -89,6 +89,9 @@ internal static BadHttpRequestException GetException(RequestRejectionReason reas
                case RequestRejectionReason.InvalidHostHeader:
                    ex = new BadHttpRequestException("Invalid Host header.", StatusCodes.Status400BadRequest);
                    break;
+                case RequestRejectionReason.UpgradeRequestCannotHavePayload:
+                    ex = new BadHttpRequestException("Requests with 'Connection: Upgrade' cannot have content in the request body", StatusCodes.Status400BadRequest);


nit: end error message with a period.

cesarblum · 2017-04-18T16:06:25Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/UpgradeTests.cs

+                        "",
+                        "");
+                    await connection.Receive("HTTP/1.1 101 ");
+                    await connection.ReceiveUntil("New protocol data");


Assert the exact message you're expecting to receive.

cesarblum · 2017-04-18T16:06:44Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/UpgradeTests.cs

+                        "");
+                    await connection.Receive("HTTP/1.1 101 ");
+                    await connection.ReceiveUntil("New protocol data");
+                    await upgrade.Task.TimeoutAfter(TimeSpan.FromSeconds(30));


Capture the exception and assert that it is exactly what you expect.

cesarblum · 2017-04-18T16:07:24Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/UpgradeTests.cs

+                    await connection.Receive("HTTP/1.1 101 ");
+
+                    var sendTask = connection.Send(send + "\r\n");
+                    var recvTask = connection.ReceiveUntil(recv);


Assert the exact message you're expecting

cesarblum · 2017-04-18T16:08:12Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/UpgradeTests.cs

+        }
+
+        [Fact]
+        public async Task RequestWithContentLengthAndUpgradeThrows()


nit: AreRejected or Receive400Response are better than Throws

cesarblum · 2017-04-18T16:08:21Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/UpgradeTests.cs

+                        "",
+                        "1");
+
+                    await connection.Receive("HTTP/1.1 400 ");


Assert exact message

cesarblum · 2017-04-18T16:19:33Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/UpgradeTests.cs

+        }
+
+        [Fact]
+        public async Task RequestWithContentLengthAndUpgradeThrows()


Add test for Transfer-Encoding: chunked.

natemcmaster · 2017-04-18T18:22:38Z

🆙 📅 responded to your feedback and added more testing.

halter73 · 2017-04-18T23:04:01Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs

                : base(context)
            {
-                RequestUpgrade = upgrade;
+                RequestUpgrade = true;
            }

            protected override ValueTask<ArraySegment<byte>> PeekAsync(CancellationToken cancellationToken)


Shouldn't this always return new ValueTask<ArraySegment<byte>>(); since the upgrade requests can't have a payload? I think doing otherwise is inviting buffering middleware to interfere.

Currently, this stream is what is eventually used to back the FrameDuplexStream returned by UpgradeAsync()

halter73 · 2017-04-18T23:07:49Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/FrameRequestStream.cs

-            _state = FrameStreamState.Closed;
+            if (_state != FrameStreamState.Upgraded)
+            {
+                _state = FrameStreamState.Closed;


Why not doe this if the _state is Upgraded?

halter73 · 2017-04-18T23:09:32Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/FrameRequestStream.cs

+            {
+                // Can't use dispose (or close) as can be disposed too early by user code
+                // As exampled in EngineTests.ZeroContentLengthNotSetAutomaticallyForCertainStatusCodes
+                _state = FrameStreamState.Closed;


halter73 · 2017-04-18T23:10:30Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/FrameRequestStream.cs

+            var upgraded = new FrameRequestStream(this);
+            if (_state != FrameStreamState.Aborted)
+            {
+                _state = FrameStreamState.Upgraded;


Why not change state only if Aborted, but not closed? Why still return an upgraded stream?

halter73 · 2017-04-18T23:10:33Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/FrameRequestStream.cs

@@ -209,6 +224,8 @@ private Task<int> ValidateState(CancellationToken cancellationToken)
                    return _error != null ?
                        Task.FromException<int>(_error) :
                        Task.FromCanceled<int>(new CancellationToken(true));
+                case FrameStreamState.Upgraded:
+                    return _nullRead;


The stream should return nullRead even if it hasn't been upgraded.

halter73 · 2017-04-18T23:11:03Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/FrameStreamState.cs

@@ -7,6 +7,7 @@ enum FrameStreamState
    {
        Open,
        Closed,
-        Aborted
+        Aborted,
+        Upgraded


I would remove the Upgraded state.

halter73 · 2017-04-18T23:11:55Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Http/MessageBody.cs

@@ -28,7 +28,7 @@ protected MessageBody(Frame context)

        public bool RequestUpgrade { get; protected set; }

-        public Task<int> ReadAsync(ArraySegment<byte> buffer, CancellationToken cancellationToken = default(CancellationToken))
+        public virtual Task<int> ReadAsync(ArraySegment<byte> buffer, CancellationToken cancellationToken = default(CancellationToken))


PeekAsync is already abstract. Use that instead.

halter73 · 2017-04-18T23:22:28Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Infrastructure/Streams.cs

+
+        public Stream Upgrade()
+        {
+            _upgradedRequest = _upgradedRequest ?? RequestBody.Upgrade();


I think asking the primary RequestBody stream to return an upgraded version of itself causes some grossness. I don't like either FrameRequestStream or FrameResponseStream having an Upgraded state or an Upgrade method.

I think that an upgradable request should have completely fake RequestBody and ResponseBodys that no-op. The upgraded stream should be built from the real streams that aren't accessible via the HttpContext directly.

cesarblum · 2017-04-18T22:53:40Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/CoreStrings.resx

@@ -0,0 +1,123 @@
+<?xml version="1.0" encoding="utf-8"?>


So we can assert the exception being thrown has the message we expect. Per this comment: #1706 (comment)

Now that we have this, I think we should move all our user-facing strings to this file (not in this PR of course). Can you file a task for that?

natemcmaster · 2017-04-19T20:09:21Z

🆙 📅 rewritten per @halter73's feedback

halter73 · 2017-04-19T23:33:48Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Infrastructure/Streams.cs

+            return _upgradeStream;
+        }
+
+        public (Stream request, Stream response) Start(MessageBody body)


+1 Pranav Points

halter73 · 2017-04-19T23:36:13Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Infrastructure/WrappedStream.cs

+
+namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Infrastructure
+{
+    internal class WrappedStream : Stream


Nit: maybe call this WrappingStream

natemcmaster · 2017-04-20T16:16:54Z

Plaintext benchmark: lots of noise on dev, but appears average perf is stable.

dev

RPS: 1317002.91
RPS: 1334182.6
RPS: 1265391.26
RPS: 1283916.25

namc/upgrade

RPS: 1283744.76
RPS: 1292864.19
RPS: 1274556.47
RPS: 1290861.72

cesarblum · 2017-04-20T16:42:58Z

src/Microsoft.AspNetCore.Server.Kestrel.Core/Internal/Infrastructure/WrappingStream.cs

+using System;
+using System.IO;
+#if NET46
+using System.Runtime.Remoting;


What is this?

System.Runtime.Remoting.ObjRef

cesarblum · 2017-04-20T16:45:18Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/UpgradeTests.cs

+        }
+
+        [Fact]
+        public async Task RejectsRequestWithChunckedEncodingAndUpgrade()


typo: Chuncked

cesarblum · 2017-04-20T19:10:45Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/UpgradeTests.cs

+            {
+                using (var connection = server.CreateConnection())
+                {
+                    await connection.Send("POST / HTTP/1.1",


This case should be in a separate test.

After upgrade has been accepted by the server: - Reads to HttpRequest.Body always return 0 - Writes to HttpResponse.Body always throw - The only valid way to communicate is to use the stream returned by IHttpUpgradeFeature.UpgradeAsync() Also, Kestrel returns HTTP 400 if requests attempt to send a request body along with Connection: Upgrade

natemcmaster requested review from halter73, davidfowl and cesarblum April 18, 2017 00:13

dnfclas added the cla-already-signed label Apr 18, 2017

davidfowl reviewed Apr 18, 2017

View reviewed changes

cesarblum suggested changes Apr 18, 2017

View reviewed changes

cesarblum reviewed Apr 18, 2017

View reviewed changes

natemcmaster force-pushed the namc/upgrade branch from 7b5f1ca to d8fc979 Compare April 18, 2017 18:26

halter73 suggested changes Apr 18, 2017

View reviewed changes

cesarblum reviewed Apr 18, 2017

View reviewed changes

halter73 approved these changes Apr 19, 2017

View reviewed changes

natemcmaster force-pushed the namc/upgrade branch 2 times, most recently from 256ca59 to df5984e Compare April 20, 2017 16:07

cesarblum reviewed Apr 20, 2017

View reviewed changes

natemcmaster force-pushed the namc/upgrade branch from df5984e to 4382a2e Compare April 20, 2017 18:20

cesarblum suggested changes Apr 20, 2017

View reviewed changes

natemcmaster force-pushed the namc/upgrade branch from 4382a2e to ee9feed Compare April 20, 2017 19:43

cesarblum approved these changes Apr 20, 2017

View reviewed changes

natemcmaster merged commit ee9feed into dev Apr 20, 2017

natemcmaster deleted the namc/upgrade branch April 20, 2017 22:42

Improve implementation of IHttpUpgradeFeature #1706

Improve implementation of IHttpUpgradeFeature #1706

Conversation

natemcmaster commented Apr 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarblum Apr 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Apr 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

natemcmaster commented Apr 18, 2017 •

edited

Loading

cesarblum Apr 19, 2017 •

edited

Loading