Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net/http/httptest: (*Server).Close races with HTTP/2 request handler #38370

Open
bcmills opened this issue Apr 10, 2020 · 5 comments
Open

net/http/httptest: (*Server).Close races with HTTP/2 request handler #38370

bcmills opened this issue Apr 10, 2020 · 5 comments

Comments

@bcmills
Copy link
Member

@bcmills bcmills commented Apr 10, 2020

~/go/src$ go version
go version devel +24ea77559f Thu Apr 9 11:09:00 2020 -0400 linux/amd64

~/go/src$ go doc httptest.Server.Close
package httptest // import "net/http/httptest"

func (s *Server) Close()
    Close shuts down the server and blocks until all outstanding requests on
    this server have completed.

While investigating #37669, I saw a reported race that led me to believe that, contrary to its documentation, (*httptest.Server).Close was not blocking for requests to complete.

To check that hypothesis, I added the following test function:

diff --git c/src/net/http/httptest/server_test.go w/src/net/http/httptest/server_test.go
index 0aad15c5ed..8b0e0f137f 100644
--- c/src/net/http/httptest/server_test.go
+++ w/src/net/http/httptest/server_test.go
@@ -6,9 +6,12 @@ package httptest

 import (
 	"bufio"
+	"context"
+	"io"
 	"io/ioutil"
 	"net"
 	"net/http"
+	"strings"
 	"testing"
 )

@@ -238,3 +241,51 @@ func TestTLSServerWithHTTP2(t *testing.T) {
 		})
 	}
 }
+
+func TestCloseBlocksUntilRequestsCompleted(t *testing.T) {
+	t.Parallel()
+
+	modes := []string{
+		"http1",
+		"http2",
+	}
+
+	for _, mode := range modes {
+		mode := mode
+		t.Run(mode, func(t *testing.T) {
+			t.Parallel()
+
+			ctx, cancel := context.WithCancel(context.Background())
+			closed := false
+
+			cst := NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				cancel()
+
+				io.Copy(w, r.Body)
+
+				<-r.Context().Done()
+				if closed {
+					panic("Close did not wait for handler to return")
+				}
+			}))
+
+			switch mode {
+			case "http1":
+				cst.Start()
+			case "http2":
+				cst.EnableHTTP2 = true
+				cst.StartTLS()
+			}
+			defer func() {
+				cst.Close()
+				closed = true
+			}()
+
+			req, _ := http.NewRequestWithContext(ctx, "POST", cst.URL, strings.NewReader("Hello, server!"))
+			resp, err := cst.Client().Do(req)
+			if err == nil {
+				resp.Body.Close()
+			}
+		})
+	}
+}

It reliably fails under the race detector with even a very modest count, indicating that the server is not waiting for the handler to complete when HTTP/2 is enabled:

~/go/src$ go test -race net/http/httptest -run=TestCloseBlocksUntilRequestsCompleted -count=10
==================
WARNING: DATA RACE
Write at 0x00c000202018 by goroutine 10:
  net/http/httptest.TestCloseBlocksUntilRequestsCompleted.func1.2()
      /usr/local/google/home/bcmills/go/src/net/http/httptest/server_test.go:281 +0x46
  net/http/httptest.TestCloseBlocksUntilRequestsCompleted.func1()
      /usr/local/google/home/bcmills/go/src/net/http/httptest/server_test.go:289 +0x508
  testing.tRunner()
      /usr/local/google/home/bcmills/go/src/testing/testing.go:1022 +0x1eb

Previous read at 0x00c000202018 by goroutine 31:
  net/http/httptest.TestCloseBlocksUntilRequestsCompleted.func1.1()
      /usr/local/google/home/bcmills/go/src/net/http/httptest/server_test.go:267 +0x18b
  net/http.HandlerFunc.ServeHTTP()
      /usr/local/google/home/bcmills/go/src/net/http/server.go:2012 +0x51
  net/http.serverHandler.ServeHTTP()
      /usr/local/google/home/bcmills/go/src/net/http/server.go:2808 +0xc4
  net/http.initALPNRequest.ServeHTTP()
      /usr/local/google/home/bcmills/go/src/net/http/server.go:3380 +0xfc
  net/http.(*initALPNRequest).ServeHTTP()
      <autogenerated>:1 +0xa6
  net/http.Handler.ServeHTTP-fm()
      /usr/local/google/home/bcmills/go/src/net/http/server.go:87 +0x64
  net/http.(*http2serverConn).runHandler()
      /usr/local/google/home/bcmills/go/src/net/http/h2_bundle.go:5712 +0xc0

Goroutine 10 (running) created at:
  testing.(*T).Run()
      /usr/local/google/home/bcmills/go/src/testing/testing.go:1073 +0x6a3
  net/http/httptest.TestCloseBlocksUntilRequestsCompleted()
      /usr/local/google/home/bcmills/go/src/net/http/httptest/server_test.go:255 +0x13e
  testing.tRunner()
      /usr/local/google/home/bcmills/go/src/testing/testing.go:1022 +0x1eb

Goroutine 31 (finished) created at:
  net/http.(*http2serverConn).processHeaders()
      /usr/local/google/home/bcmills/go/src/net/http/h2_bundle.go:5446 +0x8fb
  net/http.(*http2serverConn).processFrame()
      /usr/local/google/home/bcmills/go/src/net/http/h2_bundle.go:4975 +0x44a
  net/http.(*http2serverConn).processFrameFromReader()
      /usr/local/google/home/bcmills/go/src/net/http/h2_bundle.go:4933 +0x750
  net/http.(*http2serverConn).serve()
      /usr/local/google/home/bcmills/go/src/net/http/h2_bundle.go:4432 +0x1485
  net/http.(*http2Server).ServeConn()
      /usr/local/google/home/bcmills/go/src/net/http/h2_bundle.go:4033 +0xd88
  net/http.http2ConfigureServer.func1()
      /usr/local/google/home/bcmills/go/src/net/http/h2_bundle.go:3859 +0x117
  net/http.(*conn).serve()
      /usr/local/google/home/bcmills/go/src/net/http/server.go:1805 +0x1c77
==================
--- FAIL: TestCloseBlocksUntilRequestsCompleted (0.00s)
    --- FAIL: TestCloseBlocksUntilRequestsCompleted/http2 (0.02s)
        testing.go:937: race detected during execution of test
2020/04/10 16:35:14 http2: panic serving 127.0.0.1:46044: Close did not wait for handler to return
goroutine 132 [running]:
net/http.(*http2serverConn).runHandler.func1(0xc0000ae068, 0xc00022af4f, 0xc00031a180)
        /usr/local/google/home/bcmills/go/src/net/http/h2_bundle.go:5705 +0x231
panic(0x8b5220, 0x9c14f0)
        /usr/local/google/home/bcmills/go/src/runtime/panic.go:975 +0x3b3
net/http/httptest.TestCloseBlocksUntilRequestsCompleted.func1.1(0x9cd0c0, 0xc0000ae068, 0xc0000ac600)
        /usr/local/google/home/bcmills/go/src/net/http/httptest/server_test.go:268 +0x1e2
net/http.HandlerFunc.ServeHTTP(0xc000096740, 0x9cd0c0, 0xc0000ae068, 0xc0000ac600)
        /usr/local/google/home/bcmills/go/src/net/http/server.go:2012 +0x52
net/http.serverHandler.ServeHTTP(0xc0000a8460, 0x9cd0c0, 0xc0000ae068, 0xc0000ac600)
        /usr/local/google/home/bcmills/go/src/net/http/server.go:2808 +0xc5
net/http.initALPNRequest.ServeHTTP(0x9cd940, 0xc000282b70, 0xc00029a380, 0xc0000a8460, 0x9cd0c0, 0xc0000ae068, 0xc0000ac600)
        /usr/local/google/home/bcmills/go/src/net/http/server.go:3380 +0xfd
net/http.(*http2serverConn).runHandler(0xc00031a180, 0xc0000ae068, 0xc0000ac600, 0xc0000977c0)
        /usr/local/google/home/bcmills/go/src/net/http/h2_bundle.go:5712 +0xc1
created by net/http.(*http2serverConn).processHeaders
        /usr/local/google/home/bcmills/go/src/net/http/h2_bundle.go:5446 +0x8fc
FAIL
FAIL    net/http/httptest       0.202s
FAIL

CC @bradfitz @tombergan @nerd2

@bcmills bcmills added this to the Go1.15 milestone Apr 10, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Apr 10, 2020

Change https://golang.org/cl/227924 mentions this issue: net/http/httptest: add a regression test for #38370

@bcmills
Copy link
Member Author

@bcmills bcmills commented Apr 10, 2020

See previously #12262.

@bcmills
Copy link
Member Author

@bcmills bcmills commented Apr 10, 2020

Curiously, I'm not able to get the test to fail without the race detector.

I suspect that httptest may actually be synchronizing itself via the network socket in a way that is hidden from the race detector.

@bcmills bcmills changed the title net/http/httptest: (*Server).Close fails to block until outstanding HTTP/2 requests have completed net/http/httptest: (*Server).Close races with HTTP/2 request handler Apr 10, 2020
@nwidger
Copy link
Contributor

@nwidger nwidger commented May 27, 2020

@bcmills Could this be related to the same issue as #36946? Looks like the httptest server's Close method calls SetKeepAlivesEnabled(false):

https://golang.org/src/net/http/httptest/server.go?s=5496:5520#L190

I believe that might explain why the test server is not waiting for the handler to complete when HTTP/2 is enabled.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jun 26, 2020

Too late for 1.15, and not entirely clear that there is a real problem. Moving to Backlog.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.15, Backlog Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.