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

benaadams · 2016-07-09T07:49:00Z

Now doesn't emit TE or body for non body responses

Resolves #952

Tratcher · 2016-07-09T12:25:37Z

So this allows 304 responses with bodies & connection: close? How well do browsers handle that?

benaadams · 2016-07-09T13:00:46Z

Is progress anyway... Will add an IO exception on write if status code doesn't allow body? Or just dump the writes?

benaadams · 2016-07-09T13:26:50Z

Probably should dump the body so HEAD requests will work when they return content

benaadams · 2016-07-09T14:08:11Z

Now doesn't emit TE or body for non body responses

Tratcher · 2016-07-11T21:46:35Z

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

@@ -486,6 +490,9 @@ public Task WriteAsync(ArraySegment<byte> data, CancellationToken cancellationTo
                return WriteAsyncAwaited(data, cancellationToken);
            }

+            // Don't write to body for HEAD requests or 101, 204, 205, 304 responses.
+            if (!_canHaveBody) return TaskUtilities.CompletedTask;


What about WriteAsyncAwaited?

Tratcher · 2016-07-11T21:50:20Z

This is an interesting way to enforce the protocol. The downside is that it's silent and the user doesn't know what they've done wrong. E.g. they would keep trying to copy a 5gb file not realizing they're responding to a HEAD request. Something like this should at least be logged as Debug.

cesarblum · 2016-07-11T22:18:31Z

@benaadams Can you add tests?

benaadams · 2016-07-11T22:32:44Z

@Tratcher Added logging as per ConnectionDisconnectedWrite
@CesarBS will do

halter73 · 2016-07-12T00:31:52Z

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

+            if (!_canHaveBody)
+            {
+                // Don't write to body for HEAD requests or 101, 204, 205, 304 responses.
+                LogNonBodyRequestWrite(data.Count);


I think this should throw if there is any actual data. Flush seems fine (though unnecessary), but not anything else.

Question on whether GET responses can serve HEAD requests?

Can differentiate the response codes from method, so 101, 204, 205, 304 throw and HEAD eats?

benaadams · 2016-07-17T23:07:37Z

@halter73 re: execptions, depends how you want HEAD requests to work; do they work automatically out of the box or does the user need to code a specific response for HEAD?

benaadams · 2016-07-19T23:55:57Z

@CesarBS added tests - however it still needs test to check the socket output (e.g. that its sending 500s and not doing something strange when it errors)

cesarblum · 2016-08-08T16:56:41Z

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

+            frame.HttpVersion = "HTTP/1.1";
+            ((IHttpResponseFeature)frame).StatusCode = 304;
+
+            //Act


nit: space after //

cesarblum · 2016-08-08T16:59:50Z

@benaadams I've added a few more comments. Address those and I think we're good to go 👍

benaadams · 2016-08-08T19:59:06Z

@CesarBS probably want to do another round on the exceptions when best approach in coreclr becomes more apparent.

benaadams · 2016-08-08T20:10:46Z

Microsoft.AspNetCore.Server.KestrelTests.SocketOutputTests.WritesDontCompleteImmediatelyWhenTooManyBytesAreAlreadyPreCompleted [FAIL] #1002

cesarblum · 2016-08-10T18:59:31Z

src/Microsoft.AspNetCore.Server.Kestrel/BadHttpResponseException.cs

+
+namespace Microsoft.AspNetCore.Server.Kestrel
+{
+    public sealed class BadHttpResponseException : InvalidOperationException


What is the rationale for having a new exception type instead of just throwing InvalidOperationException?

I think this exception type would only be appropriate for attempting to write a body when it's not allowed by the spec. In the other cases, like modifying the headers/statuscode/callbacks too late, I think a BadHttpResponseException is confusing. I would prefer an InvalidOperationException in all cases. It's not like this is an exception type you would have want to handle specifically. It should always be avoidable.

Differentiate server created exceptions from user code created exceptions?

I wouldn't consider these server created exceptions. These are application created exceptions. Most APIs will throw plain InvalidOperationExceptions if called incorrectly.

I actually don't mind BadHttpResponseException for the caller attempting to do things that would break the HTTP spec like setting Transfer-Encoding and a non-body status. I personally wouldn't do it, but I don't mind.

Throwing a BadHttpResponseException for calling OnStarted too late or attempting to add response headers too late seems confusing and wrong though.

benaadams · 2016-09-07T13:58:18Z

Rebased, some merge clash bugs still

cesarblum · 2016-09-07T21:13:34Z

I have a fix for the keep alive test failure here: #1090

cesarblum · 2016-09-28T19:34:50Z

@benaadams Can you rebase?

benaadams · 2016-09-28T22:44:12Z

I think rebase is going to be messy...

cesarblum · 2016-09-30T00:03:36Z

One thing I've noticed: StatusCanHaveBody should be changed to allow 101 to have a body. Otherwise WebSockets are broken because Write and WriteAsync will throw on writes.

I've pulled this PR to my machine and I'm working on rebasing it and fixing any test failures. It has useful stuff for #175.

benaadams · 2016-09-30T01:09:08Z

Ok works for me 👍

cesarblum · 2016-09-30T17:28:10Z

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

                    {
                        // Since the app has completed and we are only now generating
                        // the headers we can safely set the Content-Length to 0.
                        responseHeaders.SetRawContentLength("0", _bytesContentLengthZero);
                    }
-                }
-                else if(_keepAlive)


@benaadams I'm curious why you removed this condition in your change. I had to re-introduce the keep-alive check, otherwise some tests would fail.

Is it relevant? Unless you are saying only keep-alive can be chunked?

That's the current logic. Why bother chunking on Connection: close responses?

Haven't looked into it too closely; but couldn't you be returning a 200, chunked data and a Connection: close?

We have a test (ChunkedResponseTests.ResponsesAreNotChunkedAutomaticallyForHttp10RequestsAndHttp11NonKeepAliveRequests) that specifically checks that we don't chunk HTTP/1.1 non-keepalive responses. Is that in the spec?

You'd have to buffer all the data then to work out the size?

Not part of the spec for HTTP/1.1. It obviously is for HTTP/1.0 requests.

Chunked or not, the client won't work out the total size of the response until the connection closes. Why go through the extra cycles doing chunking for no apparent benefit?

Oh, you mean because the connection is closing it doesn't need a Content-Length or Transfer-Encoding header because the FIN indicates that? How does the client detect a partial response?

e.g. exception thrown mid file read

Agreed with @benaadams - if we don't chunk, the client has no way to tell whether the response ended or the connection closed abruptly. The client could in theory verify if it was a clean connection close or a reset, but we shouldn't count on that.

If it's not in the spec I say we should chunk HTTP/1.1 even on Connection: close.

Good point about application errors. That's a fact of life for HTTP/1.0, but reliably knowing that a response has gracefully closed is a benefit of chunked encoding even if it's a non keep-alive connection.

I'm now convinced about this changed. Just make sure we don't regress the behavior for HTTP/1.0 requests. It's easier to mess up now that all responses are HTTP/1.1.

cesarblum · 2016-09-30T19:28:05Z

Closing in favor of #1134.

dnfclas added the cla-already-signed label Jul 9, 2016

benaadams mentioned this pull request Jul 9, 2016

Random 502 cgi gateway error when response 304 then flush response #952

Closed

benaadams force-pushed the flush-304-fix branch from 67fc0f3 to f161963 Compare July 9, 2016 08:00

benaadams force-pushed the flush-304-fix branch 2 times, most recently from f6efde2 to f9656ef Compare July 9, 2016 13:59

benaadams changed the title ~~Don't set TE header for non-body responses~~ Don't set TE header or body for non-body responses Jul 9, 2016

benaadams changed the title ~~Don't set TE header or body for non-body responses~~ Don't emit TE header or body for non-body responses Jul 9, 2016

Tratcher reviewed Jul 11, 2016
View reviewed changes

benaadams force-pushed the flush-304-fix branch from f9656ef to c627c44 Compare July 11, 2016 21:53

halter73 reviewed Jul 12, 2016
View reviewed changes

benaadams force-pushed the flush-304-fix branch 8 times, most recently from 14caa67 to 4741968 Compare July 19, 2016 23:54

benaadams force-pushed the flush-304-fix branch from 4741968 to 20603bc Compare July 19, 2016 23:58

benaadams changed the title ~~Don't emit TE header or body for non-body responses~~ [Wip] Don't emit TE header or body for non-body responses Jul 22, 2016

cesarblum reviewed Aug 8, 2016
View reviewed changes

cesarblum mentioned this pull request Aug 8, 2016

Throw when setting OnStarting after response has already started (#806). #1038

Merged

benaadams force-pushed the flush-304-fix branch 2 times, most recently from 0b9baf4 to 5f4e0f2 Compare August 8, 2016 19:56

benaadams changed the title ~~[Wip] Don't emit TE header or body for non-body responses~~ Don't emit TE header or body for non-body responses Aug 8, 2016

cesarblum reviewed Aug 10, 2016
View reviewed changes

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

d8c57d2

benaadams force-pushed the flush-304-fix branch from 5f4e0f2 to d8c57d2 Compare September 7, 2016 13:57

cesarblum reviewed Sep 30, 2016

View reviewed changes

cesarblum mentioned this pull request Sep 30, 2016

Handle non-body responses #1134

Merged

1 task

cesarblum closed this Sep 30, 2016

halter73 mentioned this pull request Oct 5, 2016

Automatically chunk HTTP/1.1 Connection: close responses aspnet/HttpSysServer#259

Closed

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

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

Conversation

benaadams commented Jul 9, 2016 • edited Loading

Tratcher commented Jul 9, 2016

benaadams commented Jul 9, 2016

benaadams commented Jul 9, 2016 • edited Loading

benaadams commented Jul 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tratcher commented Jul 11, 2016

cesarblum commented Jul 11, 2016

benaadams commented Jul 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams commented Jul 17, 2016

benaadams commented Jul 19, 2016

Choose a reason for hiding this comment

cesarblum commented Aug 8, 2016

benaadams commented Aug 8, 2016

benaadams commented Aug 8, 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

benaadams commented Sep 7, 2016

cesarblum commented Sep 7, 2016

cesarblum commented Sep 28, 2016

benaadams commented Sep 28, 2016

cesarblum commented Sep 30, 2016

benaadams 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

cesarblum Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benaadams Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarblum commented Sep 30, 2016

benaadams commented Jul 9, 2016 •

edited

Loading

benaadams commented Jul 9, 2016 •

edited

Loading

cesarblum Sep 30, 2016 •

edited

Loading

benaadams Sep 30, 2016 •

edited

Loading