Handle non-body responses #1134

cesarblum · 2016-09-30T19:22:14Z

Rebased @benaadams's #962 and made some changes.

TODO: chunk HTTP/1.1 non-keepalive responses (see discussion at https://github.com/aspnet/KestrelHttpServer/pull/962/files#r81381973)

Like the original PR, this addresses #952.

@halter73 @benaadams @Tratcher

halter73 · 2016-09-30T19:26:33Z

Let's close #962 then.

cesarblum · 2016-09-30T21:25:47Z

Updated.

halter73 · 2016-09-30T23:00:47Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs

            if (_keepAlive && hasConnection)
            {
                var connectionValue = responseHeaders.HeaderConnection.ToString();
                _keepAlive = connectionValue.Equals("keep-alive", StringComparison.OrdinalIgnoreCase);
            }

-            if (!responseHeaders.HasTransferEncoding && !responseHeaders.HasContentLength)
+            // Set whether response can have body
+            _canHaveBody = StatusCanHaveBody(StatusCode) && Method != "HEAD";


Since 101 status can have a body of sorts, I would prefer _canHaveBody is true in that case. When deciding whether or not to set Content-Length: 0 we can check for 101 specifically.

halter73 · 2016-09-30T23:01:28Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs

                {
-                    // Don't set the Content-Length or Transfer-Encoding headers
-                    // automatically for HEAD requests or 101, 204, 205, 304 responses.


I would like to keep this comment somewhere.

Will move above if (_canHaveBody).

halter73 · 2016-09-30T23:10:45Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs

+            else
+            {
+                // Don't set the Content-Length or Transfer-Encoding headers
+                // automatically for HEAD requests or 204, 205, 304 responses.


This comment doesn't makes sense here since no header is being automatically set.

Also, why not reject 101 responses with a Transfer-Encoding?
And why not reject 101/204/205/304 responses with a content-length. It looks like that's also illegal.

Can 101/204/205/304 have a Content-Length of 0 (if set)?

https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.3.5

If the conditional GET used a strong cache validator (see section 13.3.3), the response SHOULD NOT include other entity-headers. Otherwise (i.e., the conditional GET used a weak validator), the response MUST NOT include other entity-headers; this prevents inconsistencies between cached entity-bodies and updated headers.

Entity-headers include Content-Length, but I don't know anything about strong cache validators, so I guess there is some wiggle room. The other status codes don't seem to mandate anything.

Where is the spec requiring that Transfer-Encoding header can't be sent? Like Content-Length it doesn't really make sense, and we wouldn't set it automatically, but where is it explicitely prohibited?

https://tools.ietf.org/html/rfc7230#section-3.3.1

Transfer-Encoding MAY be sent in a response to a HEAD request or in a 304 (Not Modified) response (Section 4.1 of [RFC7232]) to a GET request, neither of which includes a message body, to indicate that the origin server would have applied a transfer coding to the message body if the request had been an unconditional GET. This indication is not required, however, because any recipient on the response chain (including the origin server) can remove transfer codings when they are not needed.

A server MUST NOT send a Transfer-Encoding header field in any response with a status code of 1xx (Informational) or 204 (No Content). A server MUST NOT send a Transfer-Encoding header field in any 2xx (Successful) response to a CONNECT request (Section 4.3.6 of [RFC7231]).

Has lots more here

A sender MUST NOT send a Content-Length header field in any message that contains a Transfer-Encoding header field.

A user agent SHOULD send a Content-Length in a request message when no Transfer-Encoding is sent and the request method defines a meaning for an enclosed payload body. For example, a Content-Length header field is normally sent in a POST request even when the value is 0 (indicating an empty payload body). A user agent SHOULD NOT send a Content-Length header field when the request message does not contain a payload body and the method semantics do not anticipate such a body.

A server MAY send a Content-Length header field in a response to a HEAD request (Section 4.3.2 of [RFC7231]); a server MUST NOT send Content-Length in such a response unless its field-value equals the decimal number of octets that would have been sent in the payload body of a response if the same request had used the GET method.

A server MAY send a Content-Length header field in a 304 (Not Modified) response to a conditional GET request (Section 4.1 of [RFC7232]); a server MUST NOT send Content-Length in such a response unless its field-value equals the decimal number of octets that would have been sent in the payload body of a 200 (OK) response to the same request.

etc

Note: Not saying my logic was completely right according to spec - it seems to have got bigger since I read it last (which it also obviously hasn't... 😟)

halter73 · 2016-09-30T23:17:01Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs

+            if (Method == "HEAD")
+            {
+                // Don't write to body for HEAD requests.
+                Log.ConnectionHeadResponseBodyWrite(ConnectionId, count);


Can we have a flag so this is only logged once per request/response? Maybe log it at the end of the response so we can count the size of all the writes.

cesarblum · 2016-10-03T19:16:02Z

@halter73 I've addressed your feedback.

halter73 · 2016-10-03T22:19:57Z

test/Microsoft.AspNetCore.Server.KestrelTests/EngineTests.cs

@@ -548,9 +548,6 @@ public void ConnectionCanReadAndWrite(TestServiceContext testContext)
                        "POST / HTTP/1.1",
                        "Content-Length: 3",
                        "",
-                        "101POST / HTTP/1.1",
-                        "Content-Length: 3",
-                        "",


It's weird to set a 101 response and complete without writing anything, but this is still a valid test case right?

101 is now closed after response so that made this test fail. That's why I added the new test.

halter73 · 2016-10-03T22:20:58Z

test/Microsoft.AspNetCore.Server.KestrelTests/EngineTests.cs

+                {
+                    await connection.SendEnd(
+                        "GET / HTTP/1.0",
+                        "Connection: kee-alive",


kee-alive

halter73 · 2016-10-03T22:23:25Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs

@@ -75,6 +75,8 @@ public abstract partial class Frame : IFrameControl
        protected readonly long _keepAliveMilliseconds;
        private readonly long _requestHeadersTimeoutMilliseconds;

+        private int _responseBytesWritten;


This should be long. Maybe call this _responseBytesIgnored since we're only using it to log the number of bytes not written.

I'll use it when fixing #175, so less diff when I make that change 😄

halter73 · 2016-10-03T22:24:00Z

src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs

@@ -542,6 +547,8 @@ public Task WriteAsync(ArraySegment<byte> data, CancellationToken cancellationTo
                return WriteAsyncAwaited(data, cancellationToken);
            }

+            _responseBytesWritten += data.Count;


Could this be done just in the !_canHaveBody case?

Same as above.

cesarblum · 2016-10-03T22:58:54Z

Updated.

halter73 · 2016-10-03T23:37:54Z

I would also like to see the /plaintext benchmark results for this. If there is no degradation.

cesarblum · 2016-10-03T23:42:35Z

dev

[04:39:53.046] Starting scenario Plaintext on benchmark client...
[04:40:04.074] Scenario Plaintext completed on benchmark client
[04:40:04.074] RPS: 1185406.81

PR

[04:40:46.510] Starting scenario Plaintext on benchmark client...
[04:40:57.537] Scenario Plaintext completed on benchmark client
[04:40:57.537] RPS: 1194005.61

Usual fluctuation seen across multiple runs.

- Add functional tests - Remove BadHttpResponse, ResponseRejectionReasons and TestFrameProtectedMembers - Chunk non-keepalive HTTP/1.1 responses - Set _canHaveBody to true when StatusCode == 101 - Add test to check that upgraded connections are always closed when the app is done - Log writes on responses to HEAD requests only once.

dnfclas added the cla-already-signed label Sep 30, 2016

cesarblum mentioned this pull request Sep 30, 2016

Don't emit TE header or body for non-body responses #962

Closed

cesarblum force-pushed the cesarbs/handle-non-body-response branch from d50bcf1 to 96857f4 Compare September 30, 2016 19:36

halter73 suggested changes Sep 30, 2016

View reviewed changes

halter73 reviewed Oct 3, 2016

View reviewed changes

Don't emit TE header or body for non-body responses

e7e6b89

cesarblum force-pushed the cesarbs/handle-non-body-response branch from db1b445 to 9ec8bf0 Compare October 3, 2016 22:58

halter73 approved these changes Oct 3, 2016

View reviewed changes

cesarblum force-pushed the cesarbs/handle-non-body-response branch from 9ec8bf0 to 4117ad8 Compare October 3, 2016 23:48

cesarblum merged commit 4117ad8 into dev Oct 3, 2016

cesarblum deleted the cesarbs/handle-non-body-response branch October 3, 2016 23:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle non-body responses #1134

Handle non-body responses #1134

cesarblum commented Sep 30, 2016 •

edited

Loading

halter73 commented Sep 30, 2016

cesarblum commented Sep 30, 2016

halter73 Sep 30, 2016

halter73 Sep 30, 2016

cesarblum Oct 3, 2016

halter73 Sep 30, 2016

benaadams Sep 30, 2016

halter73 Sep 30, 2016

benaadams Sep 30, 2016

benaadams Oct 1, 2016

halter73 Sep 30, 2016

cesarblum commented Oct 3, 2016

halter73 Oct 3, 2016

cesarblum Oct 3, 2016

halter73 Oct 3, 2016

halter73 Oct 3, 2016

cesarblum Oct 3, 2016

halter73 Oct 3, 2016

cesarblum Oct 3, 2016

cesarblum commented Oct 3, 2016

halter73 commented Oct 3, 2016

cesarblum commented Oct 3, 2016

Handle non-body responses #1134

Handle non-body responses #1134

Conversation

cesarblum commented Sep 30, 2016 • edited Loading

halter73 commented Sep 30, 2016

cesarblum commented Sep 30, 2016

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 commented Oct 3, 2016

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 commented Oct 3, 2016

halter73 commented Oct 3, 2016

cesarblum commented Oct 3, 2016

dev

PR

cesarblum commented Sep 30, 2016 •

edited

Loading