code review 6228046: undo CL 6112054 / 2eec2501961c

undo CL 6112054 / 2eec2501961c

Now that we've fixed the Expect: test, this CL should be okay.

««« original CL description
net/http: revert 97d027b3aa68

Revert the following change set:

        changeset:   13018:97d027b3aa68
        user:        Gustavo Niemeyer <gustavo@niemeyer.net>
        date:        Mon Apr 23 22:00:16 2012 -0300
        summary:     net/http: allow clients to disable keep-alive

This broke a test on Windows 64 and somebody else
will have to check.

R=golang-dev, r
CC=golang-dev
http://codereview.appspot.com/6112054
»»»

Fixes issue 3540.

[rsc, 2012-05-22 17:49:37]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-05-22 17:54:27]

LGTM

There's a TODO that should (or at least could) be done with this CL too.

bradfitz@gopher:~/go/src$ grep -A 5 -r -E TODO.*gustavo pkg/net/http/
pkg/net/http/transport_test.go: // TODO(bradfitz,gustavo): make the
following
pkg/net/http/transport_test.go- // line an Errorf, not Logf, once issue 3540
pkg/net/http/transport_test.go- // is fixed again.
pkg/net/http/transport_test.go- t.Logf("TCP connection #%d (of %d total)
was not closed", i+1, len(tcs.list))
pkg/net/http/transport_test.go- }
pkg/net/http/transport_test.go- }

And this can re-"Fixes issue 3540".


On Tue, May 22, 2012 at 10:49 AM, <rsc@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> undo CL 6112054 / 2eec2501961c
>
> Now that we've fixed the Expect: test, this CL should be okay.
>
> ««« original CL description
> net/http: revert 97d027b3aa68
>
> Revert the following change set:
>
>        changeset:   13018:97d027b3aa68
>        user:        Gustavo Niemeyer <gustavo@niemeyer.net>
>        date:        Mon Apr 23 22:00:16 2012 -0300
>        summary:     net/http: allow clients to disable keep-alive
>
> This broke a test on Windows 64 and somebody else
> will have to check.
>
> R=golang-dev, r
> CC=golang-dev
>
http://codereview.appspot.com/**6112054<http://codereview.appspot.com/6112054>
> »»»
>
> Please review this at
http://codereview.appspot.com/**6228046/<http://codereview.appspot.com/6228046/>
>
> 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
> @@ -732,12 +732,24 @@
>  }
>
>  func (r *Request) expectsContinue() bool {
> -       return strings.ToLower(r.Header.Get("**Expect")) == "100-continue"
> +       return hasToken(r.Header.Get("Expect"**), "100-continue")
>  }
>
>  func (r *Request) wantsHttp10KeepAlive() bool {
>        if r.ProtoMajor != 1 || r.ProtoMinor != 0 {
>                return false
>        }
> -       return
strings.Contains(strings.**ToLower(r.Header.Get("**Connection")),
> "keep-alive")
> +       return hasToken(r.Header.Get("**Connection"), "keep-alive")
>  }
> +
> +func (r *Request) wantsClose() bool {
> +       return hasToken(r.Header.Get("**Connection"), "close")
> +}
> +
> +func hasToken(s, token string) bool {
> +       if s == "" {
> +               return false
> +       }
> +       // TODO This is a poor implementation of the RFC. See
> http://golang.org/issue/3535
> +       return strings.Contains(strings.**ToLower(s), token)
> +}
> Index: src/pkg/net/http/serve_test.go
> ==============================**==============================**=======
> --- a/src/pkg/net/http/serve_test.**go
> +++ b/src/pkg/net/http/serve_test.**go
> @@ -370,7 +370,7 @@
>        })
>  }
>
> -func testTcpConnectionCloses(t *testing.T, req string, h Handler) {
> +func testTCPConnectionCloses(t *testing.T, req string, h Handler) {
>        s := httptest.NewServer(h)
>        defer s.Close()
>
> @@ -410,21 +410,28 @@
>
>  // TestServeHTTP10Close verifies that HTTP/1.0 requests won't be kept
> alive.
>  func TestServeHTTP10Close(t *testing.T) {
> -       testTcpConnectionCloses(t, "GET / HTTP/1.0\r\n\r\n",
> HandlerFunc(func(w ResponseWriter, r *Request) {
> +       testTCPConnectionCloses(t, "GET / HTTP/1.0\r\n\r\n",
> HandlerFunc(func(w ResponseWriter, r *Request) {
>                ServeFile(w, r, "testdata/file")
>        }))
>  }
>
> +// 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) {
> -       testTcpConnectionCloses(t, "GET / HTTP/1.1\r\n\r\n",
> HandlerFunc(func(w ResponseWriter, r *Request) {
> +       testTCPConnectionCloses(t, "GET / HTTP/1.1\r\n\r\n",
> HandlerFunc(func(w ResponseWriter, r *Request) {
>                w.Header().Set("Connection", "close")
>        }))
>  }
>
>  func TestHandlersCanSetConnectionCl**ose10(t *testing.T) {
> -       testTcpConnectionCloses(t, "GET / HTTP/1.0\r\nConnection:
> keep-alive\r\n\r\n", HandlerFunc(func(w ResponseWriter, r *Request) {
> +       testTCPConnectionCloses(t, "GET / HTTP/1.0\r\nConnection:
> keep-alive\r\n\r\n", HandlerFunc(func(w ResponseWriter, r *Request) {
>                w.Header().Set("Connection", "close")
>        }))
>  }
> 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
>        }
>
>
>
>

[rsc, 2012-05-22 17:56:23]

I'd rather not touch that file in this CL, since it's not part of the
original, and this is already an undo of an undo, but I will add the
Fixes note.

[rsc, 2012-05-22 17:56:44]

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

undo CL 6112054 / 2eec2501961c

Now that we've fixed the Expect: test, this CL should be okay.

««« original CL description
net/http: revert 97d027b3aa68

Revert the following change set:

        changeset:   13018:97d027b3aa68
        user:        Gustavo Niemeyer <gustavo@niemeyer.net>
        date:        Mon Apr 23 22:00:16 2012 -0300
        summary:     net/http: allow clients to disable keep-alive

This broke a test on Windows 64 and somebody else
will have to check.

R=golang-dev, r
CC=golang-dev
http://codereview.appspot.com/6112054
»»»

Fixes issue 3540.

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

[bradfitz, 2012-05-22 17:59:15]

On Tue, May 22, 2012 at 10:56 AM, Russ Cox <rsc@golang.org> wrote:

> I'd rather not touch that file in this CL, since it's not part of the
> original,
>

okay, CL coming after testing it.