code review 5996044: net/http: allow clients to disable keep-alive

net/http: allow clients to disable keep-alive

Fixes issue 3540.

[niemeyer, 2012-04-13 02:42:41]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2012-04-13 02:49:38]

Read the spec to answer your TODO question, then fix the other spot where
you did ==.
On Apr 12, 2012 7:42 PM, <n13m3y3r@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> net/http: allow clients to disable keep-alive
>
> Please review this at
http://codereview.appspot.com/**5996044/<http://codereview.appspot.com/5996044/>
>
> Affected files:
>  M src/pkg/net/http/request.go
>  M src/pkg/net/http/serve_test.go
>  M src/pkg/net/http/server.go
>
>
> Index: src/pkg/net/http/request.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/request.go
> +++ b/src/pkg/net/http/request.go
> @@ -739,5 +739,10 @@
>        if r.ProtoMajor != 1 || r.ProtoMinor != 0 {
>                return false
>        }
> +       // TODO(gustavo): Why Contains here?
>        return
strings.Contains(strings.**ToLower(r.Header.Get("**Connection")),
> "keep-alive")
>  }
> +
> +func (r *Request) wantsClose() bool {
> +       return strings.ToLower(r.Header.Get("**Connection")) == "close"
> +}
> Index: src/pkg/net/http/serve_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/serve_test.**go
> +++ b/src/pkg/net/http/serve_test.**go
> @@ -415,6 +415,13 @@
>        }))
>  }
>
> +// TestClientCanClose verifies that clients can also force a connection
> to close.
> +func TestClientCanClose(t *testing.T) {
> +       testTcpConnectionCloses(t, "GET / HTTP/1.1\r\nConnection:
> close\r\n\r\n", HandlerFunc(func(w ResponseWriter, r *Request) {
> +               // Nothing.
> +       }))
> +}
> +
>  // TestHandlersCanSetConnectionCl**ose verifies that handlers can force
> a connection to close,
>  // even for HTTP/1.1 requests.
>  func TestHandlersCanSetConnectionCl**ose11(t *testing.T) {
> Index: src/pkg/net/http/server.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/server.go
> +++ b/src/pkg/net/http/server.go
> @@ -303,8 +303,7 @@
>                if !connectionHeaderSet {
>                        w.header.Set("Connection", "keep-alive")
>                }
> -       } else if !w.req.ProtoAtLeast(1, 1) {
> -               // Client did not ask to keep connection alive.
> +       } else if !w.req.ProtoAtLeast(1, 1) || w.req.wantsClose() {
>                w.closeAfterReply = true
>        }
>
>
>
>

[gustavo_niemeyer.net, 2012-04-13 03:19:11]

On Thu, Apr 12, 2012 at 23:49, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Read the spec to answer your TODO question, then fix the other spot where
> you did ==.

The spec says 1#(token), which means separated by comma and optional
whitespace. That's not what Contains does.

Either way, I dropped the TODO and replicated the bug. PTAL

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I'm not absolutely sure of anything.

[bradfitz, 2012-04-13 04:46:24]

On Thu, Apr 12, 2012 at 8:18 PM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote:

> On Thu, Apr 12, 2012 at 23:49, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > Read the spec to answer your TODO question, then fix the other spot where
> > you did ==.
>
> The spec says 1#(token), which means separated by comma and optional
> whitespace. That's not what Contains does.
>
> Either way, I dropped the TODO and replicated the bug. PTAL
>

You could add a function:

func containsToken(tokens, token string) bool {
   // TODO: do this right.  Issue nnnn.  Close approximation for now:
   return strings.Contains(tokens, token)
}

and we can fix them all later by fixing that one spot.

[niemeyer, 2012-04-15 14:56:12]

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

Please take another look.

[bradfitz, 2012-04-18 01:50:17]

LGTM

Sorry for the delay.

http://codereview.appspot.com/5996044/diff/12001/src/pkg/net/http/serve_test.go
File src/pkg/net/http/serve_test.go (right):

http://codereview.appspot.com/5996044/diff/12001/src/pkg/net/http/serve_test....
src/pkg/net/http/serve_test.go:420: testTcpConnectionCloses(t, "GET /
HTTP/1.1\r\nConnection: close\r\n\r\n", HandlerFunc(func(w ResponseWriter, r
*Request) {
initials should all be the same case.  testTCPConnectionCloses.

I know you're just copying this from below, so feel free to not fix it.  Or fix
it if you'd like.

[niemeyer, 2012-04-24 01:00:29]

*** Submitted as http://code.google.com/p/go/source/detail?r=97d027b3aa68 ***

net/http: allow clients to disable keep-alive

Fixes issue 3540.

R=golang-dev, bradfitz, gustavo
CC=golang-dev
http://codereview.appspot.com/5996044

http://codereview.appspot.com/5996044/diff/12001/src/pkg/net/http/serve_test.go
File src/pkg/net/http/serve_test.go (right):

http://codereview.appspot.com/5996044/diff/12001/src/pkg/net/http/serve_test....
src/pkg/net/http/serve_test.go:420: testTcpConnectionCloses(t, "GET /
HTTP/1.1\r\nConnection: close\r\n\r\n", HandlerFunc(func(w ResponseWriter, r
*Request) {
On 2012/04/18 01:50:17, bradfitz wrote:
> initials should all be the same case.  testTCPConnectionCloses.
> 
> I know you're just copying this from below, so feel free to not fix it.  Or
fix
> it if you'd like.

Done.