net/http/httptest: (*Server).Close races with Serve #12789
Description
See https://go-review.googlesource.com/#/c/15047/, which works around this race in one particular instance - you can reproduce this error prior to that change by running context/ctxhttp/ctxhttp_test under the race detector.
Close makes this sequence of calls:
// Close shuts down the server and blocks until all outstanding
// requests on this server have completed.
func (s *Server) Close() {
s.Listener.Close()
s.wg.Wait()
The call to s.Listener.Close prevents new connections from being established, but it does not do anything to stop existing connections.
Calling Start on the server calls (*Server).wrapHandler, which injects a waitGroupHandler. Its ServeHTTP method calls s.wg.Add:
func (h *waitGroupHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
h.s.wg.Add(1)
defer h.s.wg.Done()
h.h.ServeHTTP(w, r)
}
ServeHTTP is called after the listener's Accept has already returned, so Close may already have been called in the meantime and the call to h.s.wg.Add races with the s.wg.Wait there.
I believe that the simplest fix (which is unfortunately not all that simple) is to wrap the net.Listener, not just the http.Handler - that is, waitGroupHandler should actually be a {net.Listener, http.Handler} pair. The presence of any outstanding Accept call should block Close from returning until the wg.Add call has completed (e.g. by locking a sync.Mutex before calling Accept on the wrapped listener and locking that same mutex before returning from Close). It will take some care to avoid introducing deadlocks or further races.
(*waitGroupHandler).ServeHTTP can't do the synchronization itself: httptest guarantees that "Close shuts down the server and blocks until all outstanding requests on this server have completed", and allowing a ServeHTTP call to proceed after Close (even to write an arbitrary "already closed" error) would violate that guarantee.