code review 12435043: net/http: do not send redundant Connection: close heade...

net/http: do not send redundant Connection: close header in HTTP/1.0 responses

HTTP/1.0 connections are closed implicitly, unless otherwise specified.

Note that this change does not test or fix "request too large" responses.
Reasoning: (a) it complicates tests and fixes, (b) they should be rare,
and (c) this is just a minor wire optimization, and thus not really worth worrying
about in this context.

Fixes issue 5955.

[josharian, 2013-08-04 16:04:58]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[bradfitz, 2013-08-04 19:46:48]

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/serve_test.go
File src/pkg/net/http/serve_test.go (right):

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/serve_test...
src/pkg/net/http/serve_test.go:1777: keepalive bool
keepAlive bool // want a "Connection: keep-alive" response

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/serve_test...
src/pkg/net/http/serve_test.go:1780: {req: "GET / HTTP/1.0\r\nExpect:
100-Continue\r\n\r\n"},
100-continue isn't valid unless you're using at least HTTP/1.1 anyway, so this
test case is nonsensical.

If you're testing that we ignore 100-continue when HTTP/1.0 is announced, that
would be fine, but that feels like it should go in a different test, or should
have a big fat comment above this case.

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/serve_test...
src/pkg/net/http/serve_test.go:1781: {req: "GET * HTTP/1.0\r\n\r\n"},
"OPTIONS *" would be a better example than "GET *" if that still tests what you
want to test.

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/serve_test...
src/pkg/net/http/serve_test.go:1782: {req: "GET / HTTP/1.0\r\nExpect:
417-Expectation-Failed\r\n\r\n"},
likewise

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/serve_test...
src/pkg/net/http/serve_test.go:1789: for _, test := range tests {
use "tt" instead of test here.  convention.

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/serve_test...
src/pkg/net/http/serve_test.go:1814: if test.keepalive && header[0] !=
"keep-alive" {
more paranoid would be:

if tt.keepAlive && !reflect.DeepEqual(header, []string{"keep-alive"}) {

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/server.go#...
src/pkg/net/http/server.go:846: if w.closeAfterReply &&
!hasToken(cw.header.get("Connection"), "close") && w.req.ProtoAtLeast(1, 1) {
I think you might want to move the ProtoAtLeast check down into the if body,
around just:

   setHeader.connection = "close"

I think you always want to delete the "Connection" header, no?

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/server.go#...
src/pkg/net/http/server.go:1124: if w.req.ProtoAtLeast(1, 1) {
delete.  instead, expectsContinue should return false if !req.ProtoAtLeast(1, 1)

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/server.go#...
src/pkg/net/http/server.go:1169: if w.req.ProtoAtLeast(1, 1) {
same

[bradfitz, 2013-08-04 19:49:35]

The CL description could be more clear too.

This is only about omitting the redundant "Connection: close" response in
HTTP/1.0 responses, where it is implicit.  Any other Connection headers are
retained.

[josharian, 2013-08-06 00:05:02]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[josharian, 2013-08-06 00:05:48]

On 2013/08/06 00:05:02, josharian wrote:
> Hello mailto:golang-dev@googlegroups.com, mailto:bradfitz@golang.org (cc:
> mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

Argh. It didn't pick up my CL description. One moment...

[josharian, 2013-08-06 00:06:16]

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/serve_test.go
File src/pkg/net/http/serve_test.go (right):

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/serve_test...
src/pkg/net/http/serve_test.go:1780: {req: "GET / HTTP/1.0\r\nExpect:
100-Continue\r\n\r\n"},
On 2013/08/04 19:46:48, bradfitz wrote:
> 100-continue isn't valid unless you're using at least HTTP/1.1 anyway, so this
> test case is nonsensical.
> 
> If you're testing that we ignore 100-continue when HTTP/1.0 is announced, that
> would be fine, but that feels like it should go in a different test, or should
> have a big fat comment above this case.

[sheepishly] good point. That is indeed a separate issue; I'll deal with it in
its own CL or issue.

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/12435043/diff/5001/src/pkg/net/http/server.go#...
src/pkg/net/http/server.go:846: if w.closeAfterReply &&
!hasToken(cw.header.get("Connection"), "close") && w.req.ProtoAtLeast(1, 1) {
On 2013/08/04 19:46:48, bradfitz wrote:
> I think you might want to move the ProtoAtLeast check down into the if body,
> around just:
> 
>    setHeader.connection = "close"
> 
> I think you always want to delete the "Connection" header, no?

I had convinced myself that it was functionally equivalent, but that's
more obviously correct. Done.

[josharian, 2013-08-06 00:10:02]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-08-07 01:36:42]

LGTM

[bradfitz, 2013-08-07 01:37:34]

*** Submitted as https://code.google.com/p/go/source/detail?r=83227ed66d41 ***

net/http: do not send redundant Connection: close header in HTTP/1.0 responses

HTTP/1.0 connections are closed implicitly, unless otherwise specified.

Note that this change does not test or fix "request too large" responses.
Reasoning: (a) it complicates tests and fixes, (b) they should be rare,
and (c) this is just a minor wire optimization, and thus not really worth
worrying
about in this context.

Fixes issue 5955.

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/12435043

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[josharian, 2013-09-12 20:53:26]

*** Abandoned ***