net/http: avoid making a request to a closed server in TestServerShut…

…down

As soon as the test server closes its listener, its port may be reused
for another test server. On some platforms port reuse takes a long
time, because they cycle through the available ports before reusing an
old one. However, other platforms reuse ports much more aggressively.
net/http shouldn't know or care which kind of platform it is on —
dialing wild connections is risky and can interfere with other tests
no matter what platform we do it on.

Instead of making the second request after the server has completely
shut down, we can start (and finish!) the entire request while we are
certain that the listener has been closed but the port is still open
serving an existing request. If everything synchronizes as we expect,
that should guarantee that the second request fails.

Fixes #56421.

Change-Id: I56add243bb9f76ee04ead8f643118f9448fd1280
Reviewed-on: https://go-review.googlesource.com/c/go/+/476036
Auto-Submit: Bryan Mills <[email protected]>
Run-TryBot: Bryan Mills <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Damien Neil <[email protected]>

src/net/http/serve_test.go

@@ -5454,32 +5454,54 @@ func testServerSetKeepAlivesEnabledClosesConns(t *testing.T, mode testMode) {
 
 func TestServerShutdown(t *testing.T) { run(t, testServerShutdown) }
 func testServerShutdown(t *testing.T, mode testMode) {
-	var doShutdown func() // set later
-	var doStateCount func()
-	var shutdownRes = make(chan error, 1)
-	var statesRes = make(chan map[ConnState]int, 1)
-	var gotOnShutdown = make(chan struct{}, 1)
+	var cst *clientServerTest
+
+	var once sync.Once
+	statesRes := make(chan map[ConnState]int, 1)
+	shutdownRes := make(chan error, 1)
+	gotOnShutdown := make(chan struct{})
 	handler := HandlerFunc(func(w ResponseWriter, r *Request) {
-		doStateCount()
-		go doShutdown()
-		// Shutdown is graceful, so it should not interrupt
-		// this in-flight response. Add a tiny sleep here to
-		// increase the odds of a failure if shutdown has
-		// bugs.
-		time.Sleep(20 * time.Millisecond)
+		first := false
+		once.Do(func() {
+			statesRes <- cst.ts.Config.ExportAllConnsByState()
+			go func() {
+				shutdownRes <- cst.ts.Config.Shutdown(context.Background())
+			}()
+			first = true
+		})
+
+		if first {
+			// Shutdown is graceful, so it should not interrupt this in-flight response
+			// but should reject new requests. (Since this request is still in flight,
+			// the server's port should not be reused for another server yet.)
+			<-gotOnShutdown
+			// TODO(#59038): The HTTP/2 server empirically does not always reject new
+			// requests. As a workaround, loop until we see a failure.
+			for !t.Failed() {
+				res, err := cst.c.Get(cst.ts.URL)
+				if err != nil {
+					break
+				}
+				out, _ := io.ReadAll(res.Body)
+				res.Body.Close()
+				if mode == http2Mode {
+					t.Logf("%v: unexpected success (%q). Listener should be closed before OnShutdown is called.", cst.ts.URL, out)
+					t.Logf("Retrying to work around https://go.dev/issue/59038.")
+					continue
+				}
+				t.Errorf("%v: unexpected success (%q). Listener should be closed before OnShutdown is called.", cst.ts.URL, out)
+			}
+		}
+
 		io.WriteString(w, r.RemoteAddr)
 	})
-	cst := newClientServerTest(t, mode, handler, func(srv *httptest.Server) {
-		srv.Config.RegisterOnShutdown(func() { gotOnShutdown <- struct{}{} })
+
+	cst = newClientServerTest(t, mode, handler, func(srv *httptest.Server) {
+		srv.Config.RegisterOnShutdown(func() { close(gotOnShutdown) })
 	})
 
-	doShutdown = func() {
-		shutdownRes <- cst.ts.Config.Shutdown(context.Background())
-	}
-	doStateCount = func() {
-		statesRes <- cst.ts.Config.ExportAllConnsByState()
-	}
-	get(t, cst.c, cst.ts.URL) // calls t.Fail on failure
+	out := get(t, cst.c, cst.ts.URL) // calls t.Fail on failure
+	t.Logf("%v: %q", cst.ts.URL, out)
 
 	if err := <-shutdownRes; err != nil {
 		t.Fatalf("Shutdown: %v", err)
@@ -5489,19 +5511,16 @@ func testServerShutdown(t *testing.T, mode testMode) {
 	if states := <-statesRes; states[StateActive] != 1 {
 		t.Errorf("connection in wrong state, %v", states)
 	}
-
-	res, err := cst.c.Get(cst.ts.URL)
-	if err == nil {
-		res.Body.Close()
-		t.Fatal("second request should fail. server should be shut down")
-	}
 }
 
 func TestServerShutdownStateNew(t *testing.T) { run(t, testServerShutdownStateNew) }
 func testServerShutdownStateNew(t *testing.T, mode testMode) {
 	if testing.Short() {
 		t.Skip("test takes 5-6 seconds; skipping in short mode")
 	}
+	// The run helper runs the test in parallel only in short mode by default.
+	// Since this test has a very long latency, always run it in parallel.
+	t.Parallel()
 
 	var connAccepted sync.WaitGroup
 	ts := newClientServerTest(t, mode, HandlerFunc(func(w ResponseWriter, r *Request) {
@@ -5538,7 +5557,11 @@ func testServerShutdownStateNew(t *testing.T, mode testMode) {
 		readRes <- err
 	}()
 
+	// TODO(#59037): This timeout is hard-coded in closeIdleConnections.
+	// It is undocumented, and some users may find it surprising.
+	// Either document it, or switch to a less surprising behavior.
 	const expectTimeout = 5 * time.Second
+
 	t0 := time.Now()
 	select {
 	case got := <-shutdownRes: