code review 6347061: net/http: fix Transport race(s) with high GOMAXPROCS

net/http: fix Transport race(s) with high GOMAXPROCS

Also adds a new test for GOMAXPROCS=16 explicitly, which now passes
reliably in a stress loop like:

$ go test -c
$ (while ./http.test -test.v -test.run=Concurrency; do echo pass; done ) 2>&1 | tee foo; less foo

(It used to fail very quickly and reliably on at least Linux/amd64)

Fixes issue 3793

[bradfitz, 2012-07-03 22:42:27]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-07-03 22:49:38]

FYI

I fought this all day.  There are some gross parts but I tried to document
them and I couldn't think of anything better.

And every time I tried to fix it in different sometimes-simpler ways, a
dozen HTTP tests would (legitimately, helpfully, annoyingly) fail.  The
tests finally run reliably.

Maybe I'll have better ideas next week if this isn't reviewed in the next
hour or two.  I'll be offline Wed-Sun.


On Tue, Jul 3, 2012 at 3:42 PM, <bradfitz@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:
> net/http: fix Transport race(s) with high GOMAXPROCS
>
> Also adds a new test for GOMAXPROCS=16 explicitly, which now passes
> reliably in a stress loop like:
>
> $ go test -c
> $ (while ./http.test -test.v -test.run=Concurrency; do echo pass; done )
> 2>&1 | tee foo; less foo
>
> (It used to fail very quickly and reliably on at least Linux/amd64)
>
> Fixes issue 3793
>
> Please review this at
http://codereview.appspot.com/**6347061/<http://codereview.appspot.com/6347061/>
>
> Affected files:
>   M src/pkg/net/http/transport.go
>   M src/pkg/net/http/transport_**test.go
>
>
>

[adg, 2012-07-03 23:25:37]

This looks reasonable to me, but I think you should wait until next week and
read it with fresh eyes before submitting.

http://codereview.appspot.com/6347061/diff/3003/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

http://codereview.appspot.com/6347061/diff/3003/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:267: log.Fatalf("dup idle pconn %p in freelist",
pconn)
panic might be more helpful.

http://codereview.appspot.com/6347061/diff/3003/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:535: // closing the channel because there may be
multiple writers sharing a
there may be multiple writeLoops ?

http://codereview.appspot.com/6347061/diff/3003/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:814: once     int32
once int32 ?
why not once sync.Once ?

then it's es.once.Do(es.fn)

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

http://codereview.appspot.com/6347061/diff/3003/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:863: defer
runtime.GOMAXPROCS(runtime.GOMAXPROCS(maxProcs))
hah!

[bradfitz, 2012-07-09 16:54:14]

http://codereview.appspot.com/6347061/diff/3003/src/pkg/net/http/transport.go
File src/pkg/net/http/transport.go (right):

http://codereview.appspot.com/6347061/diff/3003/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:267: log.Fatalf("dup idle pconn %p in freelist",
pconn)
On 2012/07/03 23:25:37, adg wrote:
> panic might be more helpful.

I had panic and changed it to log.Fatalf because it crashed  with nicer output
not interleaved in my other logging.  I also don't want it caught.  I also don't
feel strongly.

http://codereview.appspot.com/6347061/diff/3003/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:535: // closing the channel because there may be
multiple writers sharing a
On 2012/07/03 23:25:37, adg wrote:
> there may be multiple writeLoops ?

Actually this whole function is no longer needed.  This assumption was wrong and
an old debugging path I took.

Reverted this bit and tests still pass.

http://codereview.appspot.com/6347061/diff/3003/src/pkg/net/http/transport.go...
src/pkg/net/http/transport.go:814: once     int32
On 2012/07/03 23:25:37, adg wrote:
> once int32 ?
> why not once sync.Once ?
> 
> then it's es.once.Do(es.fn)

Done.

[bradfitz, 2012-07-09 16:54:40]

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

Please take another look.

[bradfitz, 2012-07-09 22:34:17]

I probably shouldn't review my own CL.

On Mon, Jul 9, 2012 at 3:08 PM, <r@golang.org> wrote:

> R=bradfitz
>
> (sent by gocodereview)
>

[r, 2012-07-09 22:36:09]

it was a mouse-o. i think i fixed it by setting it to noone before you sent
this mail. if it's still wrong, i'll just pick someone else at random.

-rob



On Mon, Jul 9, 2012 at 3:34 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> I probably shouldn't review my own CL.
>
> On Mon, Jul 9, 2012 at 3:08 PM, <r@golang.org> wrote:
>
>> R=bradfitz
>>
>> (sent by gocodereview)
>>
>
>

[adg, 2012-07-11 16:38:21]

LGTM

[bradfitz, 2012-07-11 23:40:50]

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

net/http: fix Transport race(s) with high GOMAXPROCS

Also adds a new test for GOMAXPROCS=16 explicitly, which now passes
reliably in a stress loop like:

$ go test -c
$ (while ./http.test -test.v -test.run=Concurrency; do echo pass; done ) 2>&1 |
tee foo; less foo

(It used to fail very quickly and reliably on at least Linux/amd64)

Fixes issue 3793

R=golang-dev, adg, r
CC=golang-dev
http://codereview.appspot.com/6347061