code review 6201044: net/http: non-keepalive connections close successfully

net/http: non-keepalive connections close successfully

Connections did not close if Request.Close or Response.Close was true. This meant that if the user wanted the connection to close, or if the server requested it via "Connection: close", the connection would not be closed.

Fixes issue 1967.

[james4k, 2012-05-04 08:52:41]

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

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

[rsc, 2012-05-04 13:30:59]

Can you test this without SetFinalizer, please?  There is no guarantee
that the GC will collect a particular block on a particular
collection.

[james4k, 2012-05-04 16:38:04]

On 2012/05/04 13:30:59, rsc wrote:
> Can you test this without SetFinalizer, please?  There is no guarantee
> that the GC will collect a particular block on a particular
> collection.

Ah, ok. Here's an alternative, but it involves a few global vars. It seems
fitting to reuse it in a couple of the tests that are specifically testing for
connections closing.

[bradfitz, 2012-05-07 10:18:46]

Sorry for the slow review.  I lost this.

http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
File src/pkg/net/http/transport_test.go (right):

http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:41: var connTestListMutex *sync.Mutex =
&sync.Mutex{}
you don't need to list the type (*sync.Mutex) and sync.Mutex is generally
declared just as a value.  do this:

var (
       connTestMutex sync.Mutex // protects connTestList
       connTestList     []*net.TCPConn
)

http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:48: if connTestList == nil {
these three lines are unnecessary.  append does this for you.

http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:51: connTestList = append(connTestList,
c.(*net.TCPConn))
might as well just make the list be on type []net.Conn, no?

http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:64: if c.Close() == nil {
I'm not sure you can rely on this property?

I'd recommend instead making testDial return a different implementation of
net.Conn which wraps a net.Conn and removes the net.Conn from a map of
outstanding conns (so change the protected datastructure from a slice to a
map[net.Conn]bool)

http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:72: t.Errorf("%v out of %v tcp connections
were not closed", unclosed, len(connTestList))
%d instead of %d when you know it's an integer.  (minor nit)

http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:116: tr.Dial = testDial
just put this in the {}.

tr := &Transport{
    Dial: testDial,
}

[james4k, 2012-05-07 21:18:40]

OK, I've implemented all of your suggestions. In addition, I've replaced the
global state with a closure in makeTestDial.

On 2012/05/07 10:18:46, bradfitz wrote:
> Sorry for the slow review.  I lost this.
> 
>
http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
> File src/pkg/net/http/transport_test.go (right):
> 
>
http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
> src/pkg/net/http/transport_test.go:41: var connTestListMutex *sync.Mutex =
> &sync.Mutex{}
> you don't need to list the type (*sync.Mutex) and sync.Mutex is generally
> declared just as a value.  do this:
> 
> var (
>        connTestMutex sync.Mutex // protects connTestList
>        connTestList     []*net.TCPConn
> )
> 
>
http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
> src/pkg/net/http/transport_test.go:48: if connTestList == nil {
> these three lines are unnecessary.  append does this for you.
> 
>
http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
> src/pkg/net/http/transport_test.go:51: connTestList = append(connTestList,
> c.(*net.TCPConn))
> might as well just make the list be on type []net.Conn, no?
> 
>
http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
> src/pkg/net/http/transport_test.go:64: if c.Close() == nil {
> I'm not sure you can rely on this property?
> 
> I'd recommend instead making testDial return a different implementation of
> net.Conn which wraps a net.Conn and removes the net.Conn from a map of
> outstanding conns (so change the protected datastructure from a slice to a
> map[net.Conn]bool)
> 
>
http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
> src/pkg/net/http/transport_test.go:72: t.Errorf("%v out of %v tcp connections
> were not closed", unclosed, len(connTestList))
> %d instead of %d when you know it's an integer.  (minor nit)
> 
>
http://codereview.appspot.com/6201044/diff/3003/src/pkg/net/http/transport_te...
> src/pkg/net/http/transport_test.go:116: tr.Dial = testDial
> just put this in the {}.
> 
> tr := &Transport{
>     Dial: testDial,
> }

[bradfitz, 2012-05-18 17:17:37]

LGTM

[bradfitz, 2012-05-18 17:34:41]

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

net/http: non-keepalive connections close successfully

Connections did not close if Request.Close or Response.Close was true. This
meant that if the user wanted the connection to close, or if the server
requested it via "Connection: close", the connection would not be closed.

Fixes issue 1967.

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

Committer: Brad Fitzpatrick <bradfitz@golang.org>

[cookieo9, 2012-05-19 08:06:33]

On 2012/05/18 17:34:41, bradfitz wrote:
> *** Submitted as http://code.google.com/p/go/source/detail?r=820ffde8c396 ***
> 
> net/http: non-keepalive connections close successfully
> 
> Connections did not close if Request.Close or Response.Close was true. This
> meant that if the user wanted the connection to close, or if the server
> requested it via "Connection: close", the connection would not be closed.
> 
> Fixes issue 1967.
> 
> R=golang-dev, rsc, bradfitz
> CC=golang-dev
> http://codereview.appspot.com/6201044
> 
> Committer: Brad Fitzpatrick <mailto:bradfitz@golang.org>

This code breaks Client.Get() on non-persistent ("Connection: close")
connections. Response.Body returns error("use of closed network connection").

eg:

response,err := http.Get("http://...") // err == nil, but response.Close == true
n,err := response.Body.Read(buf) // err == use of closed network connection

[cookieo9, 2012-05-19 08:21:52]

Full example (works in go1.0.1 but not after this CL):

package main

import (
	"io/ioutil"
	"log"
	"net/http"
)

func foo(rw http.ResponseWriter, rq *http.Request) {
	// Set header indicating keep-alive connection should be closed
	rw.Header().Set("Connection", "close")
	// Message must be of meaningful size to cause failure
	for i := 0; i < 1024; i++ {
		rw.Write([]byte("foo "))
	}
}

func init() {
	http.HandleFunc("/", foo)
	go func() {
		if err := http.ListenAndServe(":8081", nil); err != nil {
			panic(err)
		}
	}()
}

func main() {
	resp, err := http.Get("http://localhost:8081/")
	if err != nil {
		log.Fatal(err)
	}
	defer resp.Body.Close()
	log.Printf("%#v", resp)

	bs, err := ioutil.ReadAll(resp.Body)
	if err != nil {
		log.Fatal(err)
	}

	log.Print(string(bs))
}

[bradfitz, 2012-05-19 13:13:34]

Thanks for the report.

Will fix. http://code.google.com/p/go/issues/detail?id=3644


On Sat, May 19, 2012 at 1:21 AM, <cookieo9@gmail.com> wrote:

> Full example (works in go1.0.1 but not after this CL):
>
> package main
>
> import (
>        "io/ioutil"
>        "log"
>        "net/http"
> )
>
> func foo(rw http.ResponseWriter, rq *http.Request) {
>        // Set header indicating keep-alive connection should be closed
>        rw.Header().Set("Connection", "close")
>        // Message must be of meaningful size to cause failure
>        for i := 0; i < 1024; i++ {
>                rw.Write([]byte("foo "))
>        }
> }
>
> func init() {
>        http.HandleFunc("/", foo)
>        go func() {
>                if err := http.ListenAndServe(":8081", nil); err != nil {
>                        panic(err)
>                }
>        }()
> }
>
> func main() {
>        resp, err :=
http.Get("http://localhost:**8081/<http://localhost:8081/>
> ")
>        if err != nil {
>                log.Fatal(err)
>        }
>        defer resp.Body.Close()
>        log.Printf("%#v", resp)
>
>        bs, err := ioutil.ReadAll(resp.Body)
>        if err != nil {
>                log.Fatal(err)
>        }
>
>        log.Print(string(bs))
> }
>
>
>
>
http://codereview.appspot.com/**6201044/<http://codereview.appspot.com/6201044/>
>

[james4k, 2013-01-03 20:42:05]

*** Abandoned ***

[dave_cheney.net, 2013-01-03 22:59:06]

hg change -D 6201044

not -d, despite what the helpful text says.

On Fri, Jan 4, 2013 at 7:42 AM,  <james@james4k.com> wrote:
> *** Abandoned ***
>
> https://codereview.appspot.com/6201044/

[james4k, 2013-01-04 00:29:49]

Eek, sorry. Thanks.

On 2013/01/03 22:59:06, dfc wrote:
> hg change -D 6201044
> 
> not -d, despite what the helpful text says.
> 
> On Fri, Jan 4, 2013 at 7:42 AM,  <mailto:james@james4k.com> wrote:
> > *** Abandoned ***
> >
> > https://codereview.appspot.com/6201044/