net/http/httptest: close conns in StateNew on Server close

This part got dropped when we were debating between two solutions
in https://golang.org/cl/15151

Fixes #13032

Change-Id: I820b94f6c0c102ccf9342abf957328ea01f49a26
Reviewed-on: https://go-review.googlesource.com/16313
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

diff --git a/src/net/http/httptest/server.go b/src/net/http/httptest/server.go
index b5f1149259655db1148c23d0c000d3654a23f5f4..4a45b2b9402362d98d74b6b88b308cf3f181963a 100644 (file)
--- a/src/net/http/httptest/server.go
+++ b/src/net/http/httptest/server.go
@@ -150,7 +150,25 @@ func (s *Server) Close() {
                s.Listener.Close()
                s.Config.SetKeepAlivesEnabled(false)
                for c, st := range s.conns {
-                       if st == http.StateIdle {
+                       // Force-close any idle connections (those between
+                       // requests) and new connections (those which connected
+                       // but never sent a request). StateNew connections are
+                       // super rare and have only been seen (in
+                       // previously-flaky tests) in the case of
+                       // socket-late-binding races from the http Client
+                       // dialing this server and then getting an idle
+                       // connection before the dial completed.  There is thus
+                       // a connected connection in StateNew with no
+                       // associated Request. We only close StateIdle and
+                       // StateNew because they're not doing anything. It's
+                       // possible StateNew is about to do something in a few
+                       // milliseconds, but a previous CL to check again in a
+                       // few milliseconds wasn't liked (early versions of
+                       // https://golang.org/cl/15151) so now we just
+                       // forcefully close StateNew. The docs for Server.Close say
+                       // we wait for "oustanding requests", so we don't close things
+                       // in StateActive.
+                       if st == http.StateIdle || st == http.StateNew {
                                s.closeConn(c)
                        }
                }

diff --git a/src/net/http/httptest/server_test.go b/src/net/http/httptest/server_test.go
index 90901ceb76170dc95a25222190ffc38340aa6846..6ffc671e575241ac332af94148eba2da24b33e77 100644 (file)
--- a/src/net/http/httptest/server_test.go
+++ b/src/net/http/httptest/server_test.go
@@ -5,7 +5,9 @@
 package httptest
 
 import (
+       "bufio"
        "io/ioutil"
+       "net"
        "net/http"
        "testing"
 )
@@ -54,3 +56,31 @@ func TestGetAfterClose(t *testing.T) {
                t.Fatalf("Unexected response after close: %v, %v, %s", res.Status, res.Header, body)
        }
 }
+
+func TestServerCloseBlocking(t *testing.T) {
+       ts := NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+               w.Write([]byte("hello"))
+       }))
+       dial := func() net.Conn {
+               c, err := net.Dial("tcp", ts.Listener.Addr().String())
+               if err != nil {
+                       t.Fatal(err)
+               }
+               return c
+       }
+
+       // Keep one connection in StateNew (connected, but not sending anything)
+       cnew := dial()
+       defer cnew.Close()
+
+       // Keep one connection in StateIdle (idle after a request)
+       cidle := dial()
+       defer cidle.Close()
+       cidle.Write([]byte("HEAD / HTTP/1.1\r\nHost: foo\r\n\r\n"))
+       _, err := http.ReadResponse(bufio.NewReader(cidle), nil)
+       if err != nil {
+               t.Fatal(err)
+       }
+
+       ts.Close() // test we don't hang here forever.
+}