net/http: test for racing idle conn closure and new requests

TestTransportRemovesH2ConnsAfterIdle is experiencing flaky
failures due to a bug in idle connection handling.
Upon inspection, TestTransportRemovesH2ConnsAfterIdle
is slow and (I think) not currently testing the condition
that it was added to test.

Using the new synctest package, this CL:

- Adds a test for the failure causing flakes in this test.
- Rewrites the existing test to use synctest to avoid sleeps.
- Adds a new test that covers the condition the test was
  intended to examine.

The new TestTransportIdleConnRacesRequest exercises the
scenario where a never-used connection is closed by the
idle-conn timer at the same time as a new request attempts
to use it. In this race, the new request should either
successfully use the old connection (superseding the
idle timer) or should use a new connection; it should not
use the closing connection and fail.

TestTransportRemovesConnsAfterIdle verifies that
a connection is reused before the idle timer expires,
and not reused after.

TestTransportRemovesConnsAfterBroken verifies
that a connection is not reused after it encounters
an error. This exercises the bug fixed in CL 196665,
which introduced TestTransportRemovesH2ConnsAfterIdle.

For #70515

Change-Id: Id23026d2903fb15ef9a831b2df71177ea177b096
Reviewed-on: https://go-review.googlesource.com/c/go/+/631795
Reviewed-by: Jonathan Amsterdam <jba@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Damien Neil <dneil@google.com>

src/net/http/clientserver_test.go

@@ -40,9 +40,10 @@ import (
 type testMode string
 
 const (
-	http1Mode  = testMode("h1")     // HTTP/1.1
-	https1Mode = testMode("https1") // HTTPS/1.1
-	http2Mode  = testMode("h2")     // HTTP/2
+	http1Mode            = testMode("h1")            // HTTP/1.1
+	https1Mode           = testMode("https1")        // HTTPS/1.1
+	http2Mode            = testMode("h2")            // HTTP/2
+	http2UnencryptedMode = testMode("h2unencrypted") // HTTP/2
 )
 
 type testNotParallelOpt struct{}
@@ -132,6 +133,7 @@ type clientServerTest struct {
 	ts *httptest.Server
 	tr *Transport
 	c  *Client
+	li *fakeNetListener
 }
 
 func (t *clientServerTest) close() {
@@ -169,6 +171,8 @@ func optWithServerLog(lg *log.Logger) func(*httptest.Server) {
 	}
 }
 
+var optFakeNet = new(struct{})
+
 // newClientServerTest creates and starts an httptest.Server.
 //
 // The mode parameter selects the implementation to test:
@@ -180,6 +184,9 @@ func optWithServerLog(lg *log.Logger) func(*httptest.Server) {
 //
 //	func(*httptest.Server) // run before starting the server
 //	func(*http.Transport)
+//
+// The optFakeNet option configures the server and client to use a fake network implementation,
+// suitable for use in testing/synctest tests.
 func newClientServerTest(t testing.TB, mode testMode, h Handler, opts ...any) *clientServerTest {
 	if mode == http2Mode {
 		CondSkipHTTP2(t)
@@ -189,9 +196,31 @@ func newClientServerTest(t testing.TB, mode testMode, h Handler, opts ...any) *c
 		h2: mode == http2Mode,
 		h:  h,
 	}
-	cst.ts = httptest.NewUnstartedServer(h)
 
 	var transportFuncs []func(*Transport)
+
+	if idx := slices.Index(opts, any(optFakeNet)); idx >= 0 {
+		opts = slices.Delete(opts, idx, idx+1)
+		cst.li = fakeNetListen()
+		cst.ts = &httptest.Server{
+			Config:   &Server{Handler: h},
+			Listener: cst.li,
+		}
+		transportFuncs = append(transportFuncs, func(tr *Transport) {
+			tr.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
+				return cst.li.connect(), nil
+			}
+		})
+	} else {
+		cst.ts = httptest.NewUnstartedServer(h)
+	}
+
+	if mode == http2UnencryptedMode {
+		p := &Protocols{}
+		p.SetUnencryptedHTTP2(true)
+		cst.ts.Config.Protocols = p
+	}
+
 	for _, opt := range opts {
 		switch opt := opt.(type) {
 		case func(*Transport):
@@ -212,6 +241,9 @@ func newClientServerTest(t testing.TB, mode testMode, h Handler, opts ...any) *c
 		cst.ts.Start()
 	case https1Mode:
 		cst.ts.StartTLS()
+	case http2UnencryptedMode:
+		ExportHttp2ConfigureServer(cst.ts.Config, nil)
+		cst.ts.Start()
 	case http2Mode:
 		ExportHttp2ConfigureServer(cst.ts.Config, nil)
 		cst.ts.TLS = cst.ts.Config.TLSConfig
@@ -221,14 +253,21 @@ func newClientServerTest(t testing.TB, mode testMode, h Handler, opts ...any) *c
 	}
 	cst.c = cst.ts.Client()
 	cst.tr = cst.c.Transport.(*Transport)
-	if mode == http2Mode {
+	if mode == http2Mode || mode == http2UnencryptedMode {
 		if err := ExportHttp2ConfigureTransport(cst.tr); err != nil {
 			t.Fatal(err)
 		}
 	}
 	for _, f := range transportFuncs {
 		f(cst.tr)
 	}
+
+	if mode == http2UnencryptedMode {
+		p := &Protocols{}
+		p.SetUnencryptedHTTP2(true)
+		cst.tr.Protocols = p
+	}
+
 	t.Cleanup(func() {
 		cst.close()
 	})
@@ -246,9 +285,19 @@ func (w testLogWriter) Write(b []byte) (int, error) {
 
 // Testing the newClientServerTest helper itself.
 func TestNewClientServerTest(t *testing.T) {
-	run(t, testNewClientServerTest, []testMode{http1Mode, https1Mode, http2Mode})
+	modes := []testMode{http1Mode, https1Mode, http2Mode}
+	t.Run("realnet", func(t *testing.T) {
+		run(t, func(t *testing.T, mode testMode) {
+			testNewClientServerTest(t, mode)
+		}, modes)
+	})
+	t.Run("synctest", func(t *testing.T) {
+		runSynctest(t, func(t testing.TB, mode testMode) {
+			testNewClientServerTest(t, mode, optFakeNet)
+		}, modes)
+	})
 }
-func testNewClientServerTest(t *testing.T, mode testMode) {
+func testNewClientServerTest(t testing.TB, mode testMode, opts ...any) {
 	var got struct {
 		sync.Mutex
 		proto  string
@@ -260,7 +309,7 @@ func testNewClientServerTest(t *testing.T, mode testMode) {
 		got.proto = r.Proto
 		got.hasTLS = r.TLS != nil
 	})
-	cst := newClientServerTest(t, mode, h)
+	cst := newClientServerTest(t, mode, h, opts...)
 	if _, err := cst.c.Head(cst.ts.URL); err != nil {
 		t.Fatal(err)
 	}

src/net/http/netconn_test.go

@@ -19,9 +19,10 @@ import (
 
 func fakeNetListen() *fakeNetListener {
 	li := &fakeNetListener{
-		setc:   make(chan struct{}, 1),
-		unsetc: make(chan struct{}, 1),
-		addr:   net.TCPAddrFromAddrPort(netip.MustParseAddrPort("127.0.0.1:8000")),
+		setc:    make(chan struct{}, 1),
+		unsetc:  make(chan struct{}, 1),
+		addr:    netip.MustParseAddrPort("127.0.0.1:8000"),
+		locPort: 10000,
 	}
 	li.unsetc <- struct{}{}
 	return li
@@ -31,7 +32,13 @@ type fakeNetListener struct {
 	setc, unsetc chan struct{}
 	queue        []net.Conn
 	closed       bool
-	addr         net.Addr
+	addr         netip.AddrPort
+	locPort      uint16
+
+	onDial func() // called when making a new connection
+
+	trackConns bool // set this to record all created conns
+	conns      []*fakeNetConn
 }
 
 func (li *fakeNetListener) lock() {
@@ -50,10 +57,18 @@ func (li *fakeNetListener) unlock() {
 }
 
 func (li *fakeNetListener) connect() *fakeNetConn {
+	if li.onDial != nil {
+		li.onDial()
+	}
 	li.lock()
 	defer li.unlock()
-	c0, c1 := fakeNetPipe()
+	locAddr := netip.AddrPortFrom(netip.AddrFrom4([4]byte{127, 0, 0, 1}), li.locPort)
+	li.locPort++
+	c0, c1 := fakeNetPipe(li.addr, locAddr)
 	li.queue = append(li.queue, c0)
+	if li.trackConns {
+		li.conns = append(li.conns, c0)
+	}
 	return c1
 }
 
@@ -76,21 +91,24 @@ func (li *fakeNetListener) Close() error {
 }
 
 func (li *fakeNetListener) Addr() net.Addr {
-	return li.addr
+	return net.TCPAddrFromAddrPort(li.addr)
 }
 
 // fakeNetPipe creates an in-memory, full duplex network connection.
 //
 // Unlike net.Pipe, the connection is not synchronous.
 // Writes are made to a buffer, and return immediately.
 // By default, the buffer size is unlimited.
-func fakeNetPipe() (r, w *fakeNetConn) {
-	s1addr := net.TCPAddrFromAddrPort(netip.MustParseAddrPort("127.0.0.1:8000"))
-	s2addr := net.TCPAddrFromAddrPort(netip.MustParseAddrPort("127.0.0.1:8001"))
+func fakeNetPipe(s1ap, s2ap netip.AddrPort) (r, w *fakeNetConn) {
+	s1addr := net.TCPAddrFromAddrPort(s1ap)
+	s2addr := net.TCPAddrFromAddrPort(s2ap)
 	s1 := newSynctestNetConnHalf(s1addr)
 	s2 := newSynctestNetConnHalf(s2addr)
-	return &fakeNetConn{loc: s1, rem: s2},
-		&fakeNetConn{loc: s2, rem: s1}
+	c1 := &fakeNetConn{loc: s1, rem: s2}
+	c2 := &fakeNetConn{loc: s2, rem: s1}
+	c1.peer = c2
+	c2.peer = c1
+	return c1, c2
 }
 
 // A fakeNetConn is one endpoint of the connection created by fakeNetPipe.
@@ -102,6 +120,11 @@ type fakeNetConn struct {
 
 	// When set, synctest.Wait is automatically called before reads and after writes.
 	autoWait bool
+
+	// peer is the other endpoint.
+	peer *fakeNetConn
+
+	onClose func() // called when closing
 }
 
 // Read reads data from the connection.
@@ -143,6 +166,9 @@ func (c *fakeNetConn) IsClosedByPeer() bool {
 
 // Close closes the connection.
 func (c *fakeNetConn) Close() error {
+	if c.onClose != nil {
+		c.onClose()
+	}
 	// Local half of the conn is now closed.
 	c.loc.lock()
 	c.loc.writeErr = net.ErrClosed