code review 5656046: net/http: fix client goroutine leak with persistent con...

net/http: fix client goroutine leak with persistent connections

Thanks to Sascha Matzke & Florian Weimer for diagnosing.

[bradfitz, 2012-02-14 00:06:21]

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

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

[adg, 2012-02-14 00:17:30]

LGTM for the fix

Some comments about the test.

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

http://codereview.appspot.com/5656046/diff/4001/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:651: n0 := runtime.Goroutines()
It would be nice to not have to rely on this. Is it possible to punch through
and look at the connection pools themselves?

http://codereview.appspot.com/5656046/diff/4001/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:681: runtime.Gosched()
This also deserves a comment. Why do we need to Gosched at all?

http://codereview.appspot.com/5656046/diff/4001/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:683: runtime.GC()
Add a comment
// Russ said to GC twice for good measure.

http://codereview.appspot.com/5656046/diff/4001/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:688: if growth > 10 {
where does the magic 10 come from?

[bradfitz, 2012-02-14 00:29:20]

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

Please take another look.

[bradfitzgoog, 2012-02-14 00:29:26]

PTAL

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

http://codereview.appspot.com/5656046/diff/4001/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:651: n0 := runtime.Goroutines()
On 2012/02/14 00:17:30, adg wrote:
> It would be nice to not have to rely on this. Is it possible to punch through
> and look at the connection pools themselves?

No.

The connections pools would look fine.  The problem is goroutines not exiting.

http://codereview.appspot.com/5656046/diff/4001/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:681: runtime.Gosched()
On 2012/02/14 00:17:30, adg wrote:
> This also deserves a comment. Why do we need to Gosched at all?

This can actually go now.  It was an experiment to see if it'd be stable without
the sleep.  It wasn't.

http://codereview.appspot.com/5656046/diff/4001/src/pkg/net/http/transport_te...
src/pkg/net/http/transport_test.go:688: if growth > 10 {
On 2012/02/14 00:17:30, adg wrote:
> where does the magic 10 come from?

Double the actual number of expected goroutine growth.  We actually see 5.  I'll
add a comment.

[kevlar, 2012-02-14 01:16:28]

LGTM though I have been bitten later by these sorts of tests (counting active
goroutines) in my own code.

[adg, 2012-02-14 01:47:58]

LGTM

[bradfitz, 2012-02-14 01:49:03]

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

net/http: fix client goroutine leak with persistent connections

Thanks to Sascha Matzke & Florian Weimer for diagnosing.

R=golang-dev, adg, bradfitz, kevlar
CC=golang-dev
http://codereview.appspot.com/5656046

[rsc, 2012-02-14 03:27:20]

Appears to leak fds on darwin/386.  Disabled test.

[bradfitz, 2012-02-14 03:50:44]

I don't think it actually leaks. I think it just hits the OS X small fd
ulimit of 256.

I've lowered the number of parallel connections from 100 to 50, which
should keep it under 256.

$ go test -test.run=Leak -test.v
=== RUN TestTransportPersistConnLeak
Got:
total 0
lr-x------ 1 bradfitz eng 64 2012-02-14 14:49 0 -> /dev/null
lrwx------ 1 bradfitz eng 64 2012-02-14 14:49 1 -> /dev/pts/19
lrwx------ 1 bradfitz eng 64 2012-02-14 14:49 2 -> /dev/pts/19
lrwx------ 1 bradfitz eng 64 2012-02-14 14:49 3 -> socket:[1386530]
lr-x------ 1 bradfitz eng 64 2012-02-14 14:49 4 -> pipe:[1386531]
l-wx------ 1 bradfitz eng 64 2012-02-14 14:49 5 -> pipe:[1386531]
lrwx------ 1 bradfitz eng 64 2012-02-14 14:49 6 -> anon_inode:[eventpoll]
lr-x------ 1 bradfitz eng 64 2012-02-14 14:49 8 -> pipe:[1386678]
--- PASS: TestTransportPersistConnLeak (0.12 seconds)
   transport_test.go:697: goroutine growth: 4 -> 154 -> 5 (delta: 1)


On Tue, Feb 14, 2012 at 2:27 PM, <rsc@golang.org> wrote:

> Appears to leak fds on darwin/386.  Disabled test.
>
>
>
http://codereview.appspot.com/**5656046/<http://codereview.appspot.com/5656046/>
>

[rsc, 2012-02-14 03:52:18]

What's printing the GB of output?
Can we make it stop after printing say 10 errors?