net/http: Transport leaks net.Conns if connections never become idle

[bcmills]

The following test case fails at go1.13rc1 due to a leak introduced in CL 184262. The leak occurs due to persistConn pointers entering the idleConnWait queue and remaining reachable via that queue until a connection becomes idle: if a connection never becomes idle, the queue is never cleared.

// A countedConn is a net.Conn that decrements an atomic counter when finalized.
type countedConn struct {
	net.Conn
	atomicConnCount *int64
}

// A countingDialer dials connections and counts the number that remain reachable.
type countingDialer struct {
	dialer      net.Dialer
	atomicCount int64
}

func (d *countingDialer) DialContext(ctx context.Context, network, address string) (net.Conn, error) {
	conn, err := d.dialer.DialContext(ctx, network, address)
	if err != nil {
		return nil, err
	}

	counted := new(countedConn)
	counted.Conn = conn
	atomic.AddInt64(&d.atomicCount, 1)
	runtime.SetFinalizer(counted, d.decrement)
	return counted, nil
}

func (d *countingDialer) decrement(*countedConn) {
	atomic.AddInt64(&d.atomicCount, -1)
}

func (d *countingDialer) Count() int64 {
	return atomic.LoadInt64(&d.atomicCount)
}

func TestTransportPersistConnLeakNeverIdle(t *testing.T) {
	defer afterTest(t)

	ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) {
		// Close every connection so that it cannot be kept alive.
		conn, _, err := w.(Hijacker).Hijack()
		if err != nil {
			t.Errorf("Hijack failed unexpectedly: %v", err)
			return
		}
		conn.Close()
	}))
	defer ts.Close()

	var d countingDialer
	c := ts.Client()
	c.Transport.(*Transport).DialContext = d.DialContext

	body := []byte("Hello")
	prevCount := d.Count()
	for i := 0; ; i++ {
		n := d.Count()
		if n < prevCount {
			break
		}
		if i >= 1<<12 {
			t.Fatalf("Count of allocated client net.Conns (%d) did not decrease after %d Do / GC iterations.", n, i)
		}
		prevCount = n

		req, err := NewRequest("POST", ts.URL, bytes.NewReader(body))
		if err != nil {
			t.Fatal(err)
		}
		_, err = c.Do(req)
		if err == nil {
			t.Fatal("expected broken connection")
		}

		runtime.GC()
	}
}

The test passes with that CL reverted, but since that CL fixed a significant deadlock (#32336), I intend to fix the leak (hopefully today) rather than reverting the CL.

CC @bradfitz

[bcmills]

Didn't get this fixed today, but I'm close. (I'm worried that the test will either miss a case of the leak or be flaky, so I'm trying to make sure that I actually understand the paths involved.)

[gopherbot]

Change https://golang.org/cl/191964 mentions this issue: net/http: fix wantConnQueue memory leaks in Transport

[bcmills]

@gopherbot, please backport to Go 1.13: this is a regression from 1.12.

[gopherbot]

Backport issue(s) opened: #33877 (for 1.12), #33878 (for 1.13).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

[gopherbot]

Change https://golang.org/cl/191967 mentions this issue: [release-branch.go1.13] net/http: fix wantConnQueue memory leaks in Transport