From 2a30b34e293b6308dd1304dfedc96e0a7858476f Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Mon, 13 Mar 2023 17:36:36 -0400 Subject: [PATCH] net/http: avoid making a request to a closed server in TestServerShutdown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Run-TryBot: Bryan Mills TryBot-Result: Gopher Robot Reviewed-by: Damien Neil --- src/net/http/serve_test.go | 77 +++++++++++++++++++++++++------------- 1 file changed, 50 insertions(+), 27 deletions(-) diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 8ee8107ca9f583..23aab290daee4a 100644 --- a/src/net/http/serve_test.go +++ b/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,12 +5511,6 @@ 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) } @@ -5502,6 +5518,9 @@ 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: