net/http/internal/http2: remove fake connection autowait

Fake connections have an option to "autowait", where the connection
calls synctest.Wait before a read or after a write.
This causes some confusing problems in cases when a connection
might be used elsewhere than the main test goroutine, possibly
resulting in two synctest.Wait calls at the same time.

Drop the autowait feature, and add some more explicit Waits
as required.

For #67810

Change-Id: I3ba96b2af3b3a2f171c44bfe47be1d7f6a6a6964
Reviewed-on: https://go-review.googlesource.com/c/go/+/753841
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Nicholas Husin <nsh@golang.org>
Reviewed-by: Nicholas Husin <husin@google.com>

5 files changed, 31 insertions, 30 deletions

diff --git a/src/net/http/internal/http2/clientconn_test.go b/src/net/http/internal/http2/clientconn_test.go
index 20235f286c..91a1ed50e3 100644
--- a/src/net/http/internal/http2/clientconn_test.go
+++ b/src/net/http/internal/http2/clientconn_test.go
@@ -138,7 +138,6 @@ func newTestClientConnFromClientConn(t testing.TB, tr *Transport, cc *ClientConn
 	}
 
 	srv.SetReadDeadline(time.Now())
-	srv.autoWait = true
 	tc.netconn = srv
 	tc.enc = hpack.NewEncoder(&tc.encbuf)
 	tc.fr = NewFramer(srv, srv)
@@ -182,11 +181,13 @@ func newTestClientConn(t testing.TB, opts ...any) *testClientConn {
 
 // hasFrame reports whether a frame is available to be read.
 func (tc *testClientConn) hasFrame() bool {
+	synctest.Wait()
 	return len(tc.netconn.Peek()) > 0
 }
 
 // isClosed reports whether the peer has closed the connection.
 func (tc *testClientConn) isClosed() bool {
+	synctest.Wait()
 	return tc.netconn.IsClosedByPeer()
 }
 
@@ -380,6 +381,7 @@ func (rt *testRoundTrip) streamID() uint32 {
 
 // done reports whether RoundTrip has returned.
 func (rt *testRoundTrip) done() bool {
+	synctest.Wait()
 	select {
 	case <-rt.donec:
 		return true
@@ -544,6 +546,7 @@ func (tt *testTransport) hasConn() bool {
 
 func (tt *testTransport) getConn() *testClientConn {
 	tt.t.Helper()
+	synctest.Wait()
 	if len(tt.ccs) == 0 {
 		tt.t.Fatalf("no new ClientConns created; wanted one")
 	}
diff --git a/src/net/http/internal/http2/connframes_test.go b/src/net/http/internal/http2/connframes_test.go
index 7cfc2fa503..0590384f13 100644
--- a/src/net/http/internal/http2/connframes_test.go
+++ b/src/net/http/internal/http2/connframes_test.go
@@ -12,6 +12,7 @@ import (
 	"reflect"
 	"slices"
 	"testing"
+	"testing/synctest"
 
 	. "net/http/internal/http2"
 
@@ -28,6 +29,7 @@ type testConnFramer struct {
 // It returns nil if the conn is closed or no frames are available.
 func (tf *testConnFramer) readFrame() Frame {
 	tf.t.Helper()
+	synctest.Wait()
 	fr, err := tf.fr.ReadFrame()
 	if err == io.EOF || err == os.ErrDeadlineExceeded {
 		return nil
@@ -310,6 +312,7 @@ func (tf *testConnFramer) wantWindowUpdate(streamID, incr uint32) {
 
 func (tf *testConnFramer) wantClosed() {
 	tf.t.Helper()
+	synctest.Wait()
 	fr, err := tf.fr.ReadFrame()
 	if err == nil {
 		tf.t.Fatalf("got unexpected frame (want closed connection): %v", fr)
@@ -321,6 +324,7 @@ func (tf *testConnFramer) wantClosed() {
 
 func (tf *testConnFramer) wantIdle() {
 	tf.t.Helper()
+	synctest.Wait()
 	fr, err := tf.fr.ReadFrame()
 	if err == nil {
 		tf.t.Fatalf("got unexpected frame (want idle connection): %v", fr)
diff --git a/src/net/http/internal/http2/netconn_test.go b/src/net/http/internal/http2/netconn_test.go
index 6eba9e2de3..ef318a1abe 100644
--- a/src/net/http/internal/http2/netconn_test.go
+++ b/src/net/http/internal/http2/netconn_test.go
@@ -14,7 +14,6 @@ import (
 	"net/netip"
 	"os"
 	"sync"
-	"testing/synctest"
 	"time"
 )
 
@@ -43,43 +42,28 @@ type synctestNetConn struct {
 	// Reads pull from the local buffer, and writes push to the remote buffer.
 	loc, rem *synctestNetConnHalf
 
-	// When set, group.Wait is automatically called before reads and after writes.
-	autoWait bool
-
 	// peer is the other endpoint.
 	peer *synctestNetConn
 }
 
 // Read reads data from the connection.
 func (c *synctestNetConn) Read(b []byte) (n int, err error) {
-	if c.autoWait {
-		synctest.Wait()
-	}
 	return c.loc.read(b)
 }
 
 // Peek returns the available unread read buffer,
 // without consuming its contents.
 func (c *synctestNetConn) Peek() []byte {
-	if c.autoWait {
-		synctest.Wait()
-	}
 	return c.loc.peek()
 }
 
 // Write writes data to the connection.
 func (c *synctestNetConn) Write(b []byte) (n int, err error) {
-	if c.autoWait {
-		defer synctest.Wait()
-	}
 	return c.rem.write(b)
 }
 
 // IsClosedByPeer reports whether the peer has closed its end of the connection.
 func (c *synctestNetConn) IsClosedByPeer() bool {
-	if c.autoWait {
-		synctest.Wait()
-	}
 	return c.loc.isClosedByPeer()
 }
 
@@ -87,9 +71,6 @@ func (c *synctestNetConn) IsClosedByPeer() bool {
 func (c *synctestNetConn) Close() error {
 	c.loc.setWriteError(errors.New("connection closed by peer"))
 	c.rem.setReadError(io.EOF)
-	if c.autoWait {
-		synctest.Wait()
-	}
 	return nil
 }
 
diff --git a/src/net/http/internal/http2/server_test.go b/src/net/http/internal/http2/server_test.go
index 9137151955..c793cf8444 100644
--- a/src/net/http/internal/http2/server_test.go
+++ b/src/net/http/internal/http2/server_test.go
@@ -201,7 +201,6 @@ func newServerTester(t testing.TB, handler http.HandlerFunc, opts ...interface{}
 
 	cli, srv := synctestNetPipe()
 	cli.SetReadDeadline(time.Now())
-	cli.autoWait = true
 
 	st := &serverTester{
 		t:        t,
@@ -1258,7 +1257,6 @@ func testServer_MaxQueuedControlFrames(t testing.TB) {
 	st.greet()
 
 	st.cc.(*synctestNetConn).SetReadBufferSize(0) // all writes block
-	st.cc.(*synctestNetConn).autoWait = false     // don't sync after every write
 
 	// Send maxQueuedControlFrames pings, plus a few extra
 	// to account for ones that enter the server's write buffer.
@@ -2443,10 +2441,6 @@ func testServer_Rejects_Too_Many_Streams(t testing.TB) {
 	})
 	defer st.Close()
 
-	// Automatically syncing after every write / before every read
-	// slows this test down substantially.
-	st.cc.(*synctestNetConn).autoWait = false
-
 	st.greet()
 	nextStreamID := uint32(1)
 	streamID := func() uint32 {
@@ -4946,7 +4940,10 @@ func testServerRFC9218PrioritySmallPayload(t testing.TB) {
 				f.Flush()
 			}
 		}
+	}, func(s *http.Server) {
+		s.Protocols = protocols("h2c")
 	})
+	st.greet()
 	if syncConn, ok := st.cc.(*synctestNetConn); ok {
 		syncConn.SetReadBufferSize(1)
 	} else {
@@ -4954,7 +4951,6 @@ func testServerRFC9218PrioritySmallPayload(t testing.TB) {
 	}
 	defer st.Close()
 	defer func() { endTest = true }()
-	st.greet()
 
 	// Create 5 streams with urgency of 0, and another 5 streams with urgency
 	// of 7.
@@ -5004,14 +5000,16 @@ func testServerRFC9218Priority(t testing.TB) {
 		if f, ok := w.(http.Flusher); ok {
 			f.Flush()
 		}
+	}, func(s *http.Server) {
+		s.Protocols = protocols("h2c")
 	})
 	defer st.Close()
+	st.greet()
 	if syncConn, ok := st.cc.(*synctestNetConn); ok {
 		syncConn.SetReadBufferSize(1)
 	} else {
 		t.Fatal("Server connection is not synctestNetConn")
 	}
-	st.greet()
 	st.writeWindowUpdate(0, 1<<30)
 	synctest.Wait()
 
@@ -5057,14 +5055,16 @@ func testServerRFC9218PriorityIgnoredWhenProxied(t testing.TB) {
 		if f, ok := w.(http.Flusher); ok {
 			f.Flush()
 		}
+	}, func(s *http.Server) {
+		s.Protocols = protocols("h2c")
 	})
 	defer st.Close()
+	st.greet()
 	if syncConn, ok := st.cc.(*synctestNetConn); ok {
 		syncConn.SetReadBufferSize(1)
 	} else {
 		t.Fatal("Server connection is not synctestNetConn")
 	}
-	st.greet()
 	st.writeWindowUpdate(0, 1<<30)
 	synctest.Wait()
 
@@ -5104,14 +5104,16 @@ func testServerRFC9218PriorityAware(t testing.TB) {
 		if f, ok := w.(http.Flusher); ok {
 			f.Flush()
 		}
+	}, func(s *http.Server) {
+		s.Protocols = protocols("h2c")
 	})
 	defer st.Close()
+	st.greet()
 	if syncConn, ok := st.cc.(*synctestNetConn); ok {
 		syncConn.SetReadBufferSize(1)
 	} else {
 		t.Fatal("Server connection is not synctestNetConn")
 	}
-	st.greet()
 	st.writeWindowUpdate(0, 1<<30)
 	synctest.Wait()
 
diff --git a/src/net/http/internal/http2/transport_test.go b/src/net/http/internal/http2/transport_test.go
index e26c426649..28501c9ca0 100644
--- a/src/net/http/internal/http2/transport_test.go
+++ b/src/net/http/internal/http2/transport_test.go
@@ -2406,6 +2406,7 @@ func testTransportReturnsDataPaddingFlowControl(t testing.TB) {
 	tc.writeDataPadded(rt.streamID(), false, make([]byte, 5000), pad)
 
 	// Padding flow control should have been returned.
+	synctest.Wait()
 	if got, want := tc.inflowWindow(0), initialConnWindow-5000; got != want {
 		t.Errorf("conn inflow window = %v, want %v", got, want)
 	}
@@ -2633,6 +2634,7 @@ func testRoundTripDoesntConsumeRequestBodyEarly(t testing.TB) {
 	tc := newTestClientConn(t)
 	tc.greet()
 	tc.closeWrite()
+	synctest.Wait()
 
 	const body = "foo"
 	req, _ := http.NewRequest("POST", "http://foo.com/", io.NopCloser(strings.NewReader(body)))
@@ -3066,6 +3068,7 @@ func testTransportRetryHasLimit(t testing.TB) {
 		if totalDelay := time.Since(start); totalDelay > 5*time.Minute {
 			t.Fatalf("RoundTrip still retrying after %v, should have given up", totalDelay)
 		}
+		synctest.Wait()
 	}
 	if got, want := count, 5; got < count {
 		t.Errorf("RoundTrip made %v attempts, want at least %v", got, want)
@@ -3258,6 +3261,7 @@ func testTransportRequestsStallAtServerLimit(t *testing.T) {
 			":status", "200",
 		),
 	})
+	synctest.Wait()
 	tc.wantHeaders(wantHeader{
 		streamID:  rts[maxConcurrent+1].streamID(),
 		endStream: true,
@@ -3287,6 +3291,7 @@ func testTransportMaxDecoderHeaderTableSize(t testing.TB) {
 	}
 
 	tc.writeSettings(Setting{SettingHeaderTableSize, resSize})
+	synctest.Wait()
 	if got, want := tc.cc.TestPeerMaxHeaderTableSize(), resSize; got != want {
 		t.Fatalf("peerHeaderTableSize = %d, want %d", got, want)
 	}
@@ -5198,6 +5203,7 @@ func testTransportSendNoMoreThanOnePingWithReset(t testing.TB) {
 	// The client sends a PING frame along with the reset.
 	makeAndResetRequest()
 	pf1 := readFrame[*PingFrame](t, tc) // client sends PING
+	tc.wantIdle()
 
 	// Create another request and cancel it.
 	// We do not send a PING frame along with the reset,
@@ -5215,19 +5221,23 @@ func testTransportSendNoMoreThanOnePingWithReset(t testing.TB) {
 			":status", "200",
 		),
 	})
+	tc.wantIdle()
 
 	// Create yet another request and cancel it.
 	// We still do not send a PING frame along with the reset.
 	// We've received a HEADERS frame, but it came before the response to the PING.
 	makeAndResetRequest()
+	tc.wantIdle()
 
 	// The server responds to our PING.
 	tc.writePing(true, pf1.Data)
+	tc.wantIdle()
 
 	// Create yet another request and cancel it.
 	// Still no PING frame; we got a response to the previous one,
 	// but no HEADERS or DATA.
 	makeAndResetRequest()
+	tc.wantIdle()
 
 	// Server belatedly responds to the second request.
 	tc.writeHeaders(HeadersFrameParam{
@@ -5238,6 +5248,7 @@ func testTransportSendNoMoreThanOnePingWithReset(t testing.TB) {
 			":status", "200",
 		),
 	})
+	tc.wantIdle()
 
 	// One more request.
 	// This time we send a PING frame.