qbit/go
1
0
Fork 0
mirror of https://github.com/golang/go synced 2024-11-17 13:04:54 -07:00
Code Releases Activity

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

Browse Source 
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 <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
This commit is contained in:
Bryan C. Mills 2023-03-13 17:36:36 -04:00 committed by Gopher Robot
parent f26bf203ac
commit 2a30b34e29
1 changed files with 50 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

77
src/net/http/serve_test.go
View File

@ -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: