code review 86290043: net/http: don't reuse Transport connection unless Reque...

net/http: don't reuse Transport connection unless Request.Write finished

In a typical HTTP request, the client writes the request, and
then the server replies. Go's HTTP client code (Transport) has
two goroutines per connection: one writing, and one reading. A
third goroutine (the one initiating the HTTP request)
coordinates with those two.

Because most HTTP requests are done when the server replies,
the Go code has always handled connection reuse purely in the
readLoop goroutine.

But if a client is writing a large request and the server
replies before it's consumed the entire request (e.g. it
replied with a 403 Forbidden and had no use for the body), it
was possible for Go to re-select that connection for a
subsequent request before we were done writing the first. That
wasn't actually a data race; the second HTTP request would
just get enqueued to write its request on the writeLoop. But
because the previous writeLoop didn't finish writing (and
might not ever), that connection is in a weird state. We
really just don't want to get into a state where we're
re-using a connection when the server spoke out of turn.

This CL changes the readLoop goroutine to verify that the
writeLoop finished before returning the connection.

In the process, it also fixes a potential goroutine leak where
a connection could close but the recycling logic could be
blocked forever waiting for the client to read to EOF or
error. Now it also selects on the persistConn's close channel,
and the closer of that is no longer the readLoop (which was
dead locking in some cases before). It's now closed at the
same place the underlying net.Conn is closed. This likely fixes
or helps Issue 7620.

Also addressed some small cosmetic things in the process.

Update Issue 7620
Fixes Issue 7569

[bradfitz, 2014-04-10 00:36:21]

Hello golang-codereviews@googlegroups.com (cc: adg@golang.org,
dsymonds@golang.org, rsc@golang.org),

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

[adg, 2014-04-10 03:28:19]

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

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport.go:733: // whether or not a connection can be reused.
Issue
Issue?

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:197: //	time.Sleep(100 * time.Millisecond)
?

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:197: //	time.Sleep(100 * time.Millisecond)
?

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:1916: type byteFromChanReader chan int
maybe just document how this works very briefly

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:1922: log.Printf("pre-chan read")
delete, or put a *testing.T in the receiver value and use t.Logf

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:1924: log.Printf("post chan read %q, %v", b,
ok)
delete

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:1928: if b == -1 {
this doesn't seem to be in use
any reason not to make byteFromChanReader a chan byte ?

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:1943: var sconnmu sync.Mutex // guards sconn
var sconn struct {
  sync.Mutex
  c net.Conn
}
?

[bradfitz, 2014-04-10 04:41:32]

PTAL

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

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport.go:733: // whether or not a connection can be reused.
Issue
On 2014/04/10 03:28:19, adg wrote:
> Issue?

Done.

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:1916: type byteFromChanReader chan int
On 2014/04/10 03:28:19, adg wrote:
> maybe just document how this works very briefly

Done.

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:1922: log.Printf("pre-chan read")
On 2014/04/10 03:28:19, adg wrote:
> delete, or put a *testing.T in the receiver value and use t.Logf

deleted. whoops.

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:1924: log.Printf("post chan read %q, %v", b,
ok)
On 2014/04/10 03:28:19, adg wrote:
> delete

deleted

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:1928: if b == -1 {
On 2014/04/10 03:28:19, adg wrote:
> this doesn't seem to be in use
> any reason not to make byteFromChanReader a chan byte ?

yeah, this is old. will simplify.

https://codereview.appspot.com/86290043/diff/30001/src/pkg/net/http/transport...
src/pkg/net/http/transport_test.go:1943: var sconnmu sync.Mutex // guards sconn
On 2014/04/10 03:28:19, adg wrote:
> var sconn struct {
>   sync.Mutex
>   c net.Conn
> }
> ?

Done.

[bradfitz, 2014-04-10 04:41:41]

Hello golang-codereviews@googlegroups.com, adg@golang.org (cc:
dsymonds@golang.org, golang-codereviews@googlegroups.com, rsc@golang.org),

Please take another look.

[adg, 2014-04-10 04:46:24]

LGTM

[bradfitz, 2014-04-10 04:50:27]

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

net/http: don't reuse Transport connection unless Request.Write finished

In a typical HTTP request, the client writes the request, and
then the server replies. Go's HTTP client code (Transport) has
two goroutines per connection: one writing, and one reading. A
third goroutine (the one initiating the HTTP request)
coordinates with those two.

Because most HTTP requests are done when the server replies,
the Go code has always handled connection reuse purely in the
readLoop goroutine.

But if a client is writing a large request and the server
replies before it's consumed the entire request (e.g. it
replied with a 403 Forbidden and had no use for the body), it
was possible for Go to re-select that connection for a
subsequent request before we were done writing the first. That
wasn't actually a data race; the second HTTP request would
just get enqueued to write its request on the writeLoop. But
because the previous writeLoop didn't finish writing (and
might not ever), that connection is in a weird state. We
really just don't want to get into a state where we're
re-using a connection when the server spoke out of turn.

This CL changes the readLoop goroutine to verify that the
writeLoop finished before returning the connection.

In the process, it also fixes a potential goroutine leak where
a connection could close but the recycling logic could be
blocked forever waiting for the client to read to EOF or
error. Now it also selects on the persistConn's close channel,
and the closer of that is no longer the readLoop (which was
dead locking in some cases before). It's now closed at the
same place the underlying net.Conn is closed. This likely fixes
or helps Issue 7620.

Also addressed some small cosmetic things in the process.

Update Issue 7620
Fixes Issue 7569

LGTM=adg
R=golang-codereviews, adg
CC=dsymonds, golang-codereviews, rsc
https://codereview.appspot.com/86290043