Skip to content

Commit

Permalink
http2: remove arbitrary timeouts in server_test.go
Browse files Browse the repository at this point in the history 
If the test gets completely stuck at these points, we will want a
goroutine dump in order to debug the hang, which log.Fatalf will
not produce (but a test timeout will).

If the test does not get completely stuck (as on a slow or overloaded
builder), then we should let it continue to run until the overall test
timeout, which (unlike hard-coded constants) should already take the
speed of the builder into account.

As a side-effect, this also moves some t.Fatalf calls out of
background goroutines and into the main test-function goroutines where
they belong (see golang/go#24678).

Fixes golang/go#52051.

Change-Id: I37504081e6fdf0b4c244305fc83c575e30b7b453
Reviewed-on: https://go-review.googlesource.com/c/net/+/410096
Run-TryBot: Bryan Mills <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Auto-Submit: Bryan Mills <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
  • Loading branch information
@gopherbot
Bryan C. Mills authored and gopherbot committed Jun 7, 2022
1 parent 1855be7 commit aa149f2
Showing 1 changed file with 51 additions and 123 deletions.
174 changes: 51 additions & 123 deletions server_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -640,11 +640,7 @@ func TestServer(t *testing.T) {
EndHeaders: true,
})

select {
case <-gotReq:
case <-time.After(2 * time.Second):
t.Error("timeout waiting for request")
}
<-gotReq
}

func TestServer_Request_Get(t *testing.T) {
Expand Down Expand Up @@ -1368,13 +1364,9 @@ func testServerPostUnblock(t *testing.T,
})
<-inHandler
fn(st)
select {
case err := <-errc:
if checkErr != nil {
checkErr(err)
}
case <-time.After(5 * time.Second):
t.Fatal("timeout waiting for Handler to return")
err := <-errc
if checkErr != nil {
checkErr(err)
}
}

Expand Down Expand Up @@ -1440,11 +1432,7 @@ func testServer_RSTStream_Unblocks_Header_Write(t *testing.T) {
}
wroteRST <- true
st.awaitIdle()
select {
case <-headerWritten:
case <-time.After(2 * time.Second):
t.Error("timeout waiting for header write")
}
<-headerWritten
unblockHandler <- true
}

Expand Down Expand Up @@ -1685,21 +1673,13 @@ func testServerRejectsConn(t *testing.T, writeReq func(*serverTester)) {
writeReq(st)

st.wantGoAway()
errc := make(chan error, 1)
go func() {
fr, err := st.fr.ReadFrame()
if err == nil {
err = fmt.Errorf("got frame of type %T", fr)
}
errc <- err
}()
select {
case err := <-errc:
if err != io.EOF {
t.Errorf("ReadFrame = %v; want io.EOF", err)
}
case <-time.After(2 * time.Second):
t.Error("timeout waiting for disconnect")

fr, err := st.fr.ReadFrame()
if err == nil {
t.Errorf("ReadFrame got frame of type %T; want io.EOF", fr)
}
if err != io.EOF {
t.Errorf("ReadFrame = %v; want io.EOF", err)
}
}

Expand Down Expand Up @@ -1729,12 +1709,7 @@ func testServerRequest(t *testing.T, writeReq func(*serverTester), checkReq func

st.greet()
writeReq(st)

select {
case <-gotReq:
case <-time.After(2 * time.Second):
t.Error("timeout waiting for request")
}
<-gotReq
}

func getSlash(st *serverTester) { st.bodylessReq1() }
Expand Down Expand Up @@ -2216,20 +2191,11 @@ func TestServer_Response_RST_Unblocks_LargeWrite(t *testing.T) {
const maxFrameSize = 16 << 10
testServerResponse(t, func(w http.ResponseWriter, r *http.Request) error {
w.(http.Flusher).Flush()
errc := make(chan error, 1)
go func() {
_, err := w.Write(bytes.Repeat([]byte("a"), size))
errc <- err
}()
select {
case err := <-errc:
if err == nil {
return errors.New("unexpected nil error from Write in handler")
}
return nil
case <-time.After(2 * time.Second):
return errors.New("timeout waiting for Write in handler")
_, err := w.Write(bytes.Repeat([]byte("a"), size))
if err == nil {
return errors.New("unexpected nil error from Write in handler")
}
return nil
}, func(st *serverTester) {
if err := st.fr.WriteSettings(
Setting{SettingInitialWindowSize, 0},
Expand Down Expand Up @@ -2383,11 +2349,7 @@ func TestServer_HandlerWriteErrorOnDisconnect(t *testing.T) {
}
// Close the connection and wait for the handler to (hopefully) notice.
st.cc.Close()
select {
case <-errc:
case <-time.After(5 * time.Second):
t.Error("timeout")
}
_ = <-errc
})
}

Expand Down Expand Up @@ -2453,13 +2415,8 @@ func TestServer_Rejects_Too_Many_Streams(t *testing.T) {
// And now another stream should be able to start:
goodID := streamID()
sendReq(goodID, ":path", testPath)
select {
case got := <-inHandler:
if got != goodID {
t.Errorf("Got stream %d; want %d", got, goodID)
}
case <-time.After(3 * time.Second):
t.Error("timeout waiting for handler")
if got := <-inHandler; got != goodID {
t.Errorf("Got stream %d; want %d", got, goodID)
}
}

Expand Down Expand Up @@ -2552,17 +2509,13 @@ func TestServer_NoCrash_HandlerClose_Then_ClientClose(t *testing.T) {

// Now force the serve loop to end, via closing the connection.
st.cc.Close()
select {
case <-st.sc.doneServing:
// Loop has exited.
panMu.Lock()
got := panicVal
panMu.Unlock()
if got != nil {
t.Errorf("Got panic: %v", got)
}
case <-time.After(5 * time.Second):
t.Error("timeout")
<-st.sc.doneServing

panMu.Lock()
got := panicVal
panMu.Unlock()
if got != nil {
t.Errorf("Got panic: %v", got)
}
})
}
Expand Down Expand Up @@ -2724,38 +2677,26 @@ func testServerWithCurl(t *testing.T, permitProhibitedCipherSuites bool) {
t.Logf("Running test server for curl to hit at: %s", ts.URL)
container := curl(t, "--silent", "--http2", "--insecure", "-v", ts.URL)
defer kill(container)
resc := make(chan interface{}, 1)
go func() {
res, err := dockerLogs(container)
if err != nil {
resc <- err
} else {
resc <- res
}
}()
select {
case res := <-resc:
if err, ok := res.(error); ok {
t.Fatal(err)
}
body := string(res.([]byte))
// Search for both "key: value" and "key:value", since curl changed their format
// Our Dockerfile contains the latest version (no space), but just in case people
// didn't rebuild, check both.
if !strings.Contains(body, "foo: Bar") && !strings.Contains(body, "foo:Bar") {
t.Errorf("didn't see foo: Bar header")
t.Logf("Got: %s", body)
}
if !strings.Contains(body, "client-proto: HTTP/2") && !strings.Contains(body, "client-proto:HTTP/2") {
t.Errorf("didn't see client-proto: HTTP/2 header")
t.Logf("Got: %s", res)
}
if !strings.Contains(string(res.([]byte)), msg) {
t.Errorf("didn't see %q content", msg)
t.Logf("Got: %s", res)
}
case <-time.After(3 * time.Second):
t.Errorf("timeout waiting for curl")
res, err := dockerLogs(container)
if err != nil {
t.Fatal(err)
}

body := string(res)
// Search for both "key: value" and "key:value", since curl changed their format
// Our Dockerfile contains the latest version (no space), but just in case people
// didn't rebuild, check both.
if !strings.Contains(body, "foo: Bar") && !strings.Contains(body, "foo:Bar") {
t.Errorf("didn't see foo: Bar header")
t.Logf("Got: %s", body)
}
if !strings.Contains(body, "client-proto: HTTP/2") && !strings.Contains(body, "client-proto:HTTP/2") {
t.Errorf("didn't see client-proto: HTTP/2 header")
t.Logf("Got: %s", res)
}
if !strings.Contains(string(res), msg) {
t.Errorf("didn't see %q content", msg)
t.Logf("Got: %s", res)
}

if atomic.LoadInt32(&gotConn) == 0 {
Expand Down Expand Up @@ -3640,11 +3581,8 @@ func TestServerHandleCustomConn(t *testing.T) {
req = r
}),
}})
select {
case <-clientDone:
case <-time.After(5 * time.Second):
t.Fatal("timeout waiting for handler")
}
<-clientDone

if req.TLS == nil {
t.Fatalf("Request.TLS is nil. Got: %#v", req)
}
Expand Down Expand Up @@ -3825,18 +3763,12 @@ func TestUnreadFlowControlReturned_Server(t *testing.T) {
unblock := make(chan bool, 1)
defer close(unblock)

timeOut := time.NewTimer(5 * time.Second)
defer timeOut.Stop()
st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
// Don't read the 16KB request body. Wait until the client's
// done sending it and then return. This should cause the Server
// to then return those 16KB of flow control to the client.
tt.reqFn(r)
select {
case <-unblock:
case <-timeOut.C:
t.Fatal(tt.name, "timedout")
}
<-unblock
}, optOnlyServer)
defer st.Close()

Expand Down Expand Up @@ -4123,11 +4055,7 @@ func TestServerGracefulShutdown(t *testing.T) {
st.greet()
st.bodylessReq1()

select {
case <-handlerDone:
case <-time.After(5 * time.Second):
t.Fatalf("server did not shutdown?")
}
<-handlerDone
hf := st.wantHeaders()
goth := st.decodeHeader(hf.HeaderBlockFragment())
wanth := [][2]string{
Expand Down

0 comments on commit aa149f2

Please sign in to comment.