code review 7419050: net/http: close TCP connection on Response.Body.Close

net/http: close TCP connection on Response.Body.Close

Previously the HTTP client's (*Response).Body.Close would try
to keep reading until EOF, hoping to reuse the keep-alive HTTP
connection, but the EOF might never come, or it might take a
long time. Now we immediately close the TCP connection if we
haven't seen EOF.

This shifts the burden onto clients to read their whole response
bodies if they want the advantage of reusing TCP connections.

In the future maybe we could decide on heuristics to read some
number of bytes for some max amount of time before forcefully
closing, but I'd rather not for now.

Statistically, touching this code makes things regress, so I
wouldn't be surprised if this introduces new bugs, but all the
tests pass, and I think the code is simpler now too. Maybe.

Please test your HTTP client code before Go 1.1.

Fixes issue 3672

[bradfitz, 2013-03-04 23:04:52]

Hello golang-dev@googlegroups.com,

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

[adg, 2013-03-05 06:44:28]

It's definitely better, I don't know how much simpler it is though. The
disembodied closure callbacks are definitely tricky.

https://codereview.appspot.com/7419050/diff/4001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/7419050/diff/4001/src/pkg/net/http/transport.g...
src/pkg/net/http/transport.go:642: waitForBodyRead = make(chan bool, 2)
okay, so I knew what this did once, because I remember reviewing it when it
first went in, but now I'm having a harder time understanding it. A few comments
around this code would really help.

https://codereview.appspot.com/7419050/diff/4001/src/pkg/net/http/transport.g...
src/pkg/net/http/transport.go:644: waitForBodyRead <- false // don't reuse this
connection
see, I don't see why this does what the comment says :-/

https://codereview.appspot.com/7419050/diff/4001/src/pkg/net/http/transport.g...
src/pkg/net/http/transport.go:899: err := es.body.Close()
is it really closed if you don't call close?

https://codereview.appspot.com/7419050/diff/4001/src/pkg/net/http/transport.g...
src/pkg/net/http/transport.go:916: type readerAndCloser struct {
you mean io.ReadCloser ?

[bradfitz, 2013-03-05 16:35:50]

https://codereview.appspot.com/7419050/diff/4001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/7419050/diff/4001/src/pkg/net/http/transport.g...
src/pkg/net/http/transport.go:642: waitForBodyRead = make(chan bool, 2)
On 2013/03/05 06:44:28, adg wrote:
> okay, so I knew what this did once, because I remember reviewing it when it
> first went in, but now I'm having a harder time understanding it. A few
comments
> around this code would really help.

There are comments down below in the only place where this is read:

// Wait for the just-returned response body to be fully consumed
// before we race and peek on the underlying bufio reader.
if waitForBodyRead != nil {
     alive = <-waitForBodyRead
}

if !alive {
   pc.close()
}

https://codereview.appspot.com/7419050/diff/4001/src/pkg/net/http/transport.g...
src/pkg/net/http/transport.go:644: waitForBodyRead <- false // don't reuse this
connection
On 2013/03/05 06:44:28, adg wrote:
> see, I don't see why this does what the comment says :-/

I can make it say "set alive to false; closes conn"

https://codereview.appspot.com/7419050/diff/4001/src/pkg/net/http/transport.g...
src/pkg/net/http/transport.go:899: err := es.body.Close()
On 2013/03/05 06:44:28, adg wrote:
> is it really closed if you don't call close?

Yes, because this function is named Close.

This es.body.Close() would do the keep-reading-until-EOF thing, which is what we
don't want if we're in the middle of a body.  In that case we just want to kill
the connection.

https://codereview.appspot.com/7419050/diff/4001/src/pkg/net/http/transport.g...
src/pkg/net/http/transport.go:916: type readerAndCloser struct {
On 2013/03/05 06:44:28, adg wrote:
> you mean io.ReadCloser ?

No. This is a struct embedding a Reader and a Closer so it IMPLEMENTS
ReadCloser.

[bradfitz, 2013-03-05 18:51:35]

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

Please take another look.

[adg, 2013-03-06 01:30:37]

LGTM

https://codereview.appspot.com/7419050/diff/4001/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

https://codereview.appspot.com/7419050/diff/4001/src/pkg/net/http/transport.g...
src/pkg/net/http/transport.go:644: waitForBodyRead <- false // don't reuse this
connection
On 2013/03/05 16:35:50, bradfitz wrote:
> On 2013/03/05 06:44:28, adg wrote:
> > see, I don't see why this does what the comment says :-/
> 
> I can make it say "set alive to false; closes conn"

I think it's ok

https://codereview.appspot.com/7419050/diff/4001/src/pkg/net/http/transport.g...
src/pkg/net/http/transport.go:916: type readerAndCloser struct {
On 2013/03/05 16:35:50, bradfitz wrote:
> On 2013/03/05 06:44:28, adg wrote:
> > you mean io.ReadCloser ?
> 
> No. This is a struct embedding a Reader and a Closer so it IMPLEMENTS
> ReadCloser.

oh nuts. totes obvs

[bradfitz, 2013-03-06 02:47:23]

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

net/http: close TCP connection on Response.Body.Close

Previously the HTTP client's (*Response).Body.Close would try
to keep reading until EOF, hoping to reuse the keep-alive HTTP
connection, but the EOF might never come, or it might take a
long time. Now we immediately close the TCP connection if we
haven't seen EOF.

This shifts the burden onto clients to read their whole response
bodies if they want the advantage of reusing TCP connections.

In the future maybe we could decide on heuristics to read some
number of bytes for some max amount of time before forcefully
closing, but I'd rather not for now.

Statistically, touching this code makes things regress, so I
wouldn't be surprised if this introduces new bugs, but all the
tests pass, and I think the code is simpler now too. Maybe.

Please test your HTTP client code before Go 1.1.

Fixes issue 3672

R=golang-dev, adg
CC=golang-dev
https://codereview.appspot.com/7419050