From ee2b2994868a33e03a50b3817fd9a6c4fb66252d Mon Sep 17 00:00:00 2001 From: Vincent Batts Date: Mon, 19 Oct 2015 20:32:59 -0400 Subject: [PATCH] bz1271709 fix from upstream --- golang-1.5.1-a3156aaa12.patch | 309 ++++++++++++++++++++++++++++++++++ golang.spec | 10 +- 2 files changed, 318 insertions(+), 1 deletion(-) create mode 100644 golang-1.5.1-a3156aaa12.patch diff --git a/golang-1.5.1-a3156aaa12.patch b/golang-1.5.1-a3156aaa12.patch new file mode 100644 index 0000000..afff18d --- /dev/null +++ b/golang-1.5.1-a3156aaa12.patch @@ -0,0 +1,309 @@ +commit a3156aaa121446c4136927f8c2139fefe05ba82c +Author: Brad Fitzpatrick +Date: Tue Sep 29 14:26:48 2015 -0700 + + net/http/httptest: change Server to use http.Server.ConnState for accounting + + With this CL, httptest.Server now uses connection-level accounting of + outstanding requests instead of ServeHTTP-level accounting. This is + more robust and results in a non-racy shutdown. + + This is much easier now that net/http.Server has the ConnState hook. + + Fixes #12789 + Fixes #12781 + + Change-Id: I098cf334a6494316acb66cd07df90766df41764b + Reviewed-on: https://go-review.googlesource.com/15151 + Reviewed-by: Andrew Gerrand + Run-TryBot: Brad Fitzpatrick + TryBot-Result: Gobot Gobot + +diff --git a/src/net/http/httptest/server.go b/src/net/http/httptest/server.go +index 96eb0ef..e4f680f 100644 +--- a/src/net/http/httptest/server.go ++++ b/src/net/http/httptest/server.go +@@ -7,13 +7,17 @@ + package httptest + + import ( ++ "bytes" + "crypto/tls" + "flag" + "fmt" ++ "log" + "net" + "net/http" + "os" ++ "runtime" + "sync" ++ "time" + ) + + // A Server is an HTTP server listening on a system-chosen port on the +@@ -34,24 +38,10 @@ type Server struct { + // wg counts the number of outstanding HTTP requests on this server. + // Close blocks until all requests are finished. + wg sync.WaitGroup +-} +- +-// historyListener keeps track of all connections that it's ever +-// accepted. +-type historyListener struct { +- net.Listener +- sync.Mutex // protects history +- history []net.Conn +-} + +-func (hs *historyListener) Accept() (c net.Conn, err error) { +- c, err = hs.Listener.Accept() +- if err == nil { +- hs.Lock() +- hs.history = append(hs.history, c) +- hs.Unlock() +- } +- return ++ mu sync.Mutex // guards closed and conns ++ closed bool ++ conns map[net.Conn]http.ConnState // except terminal states + } + + func newLocalListener() net.Listener { +@@ -103,10 +93,9 @@ func (s *Server) Start() { + if s.URL != "" { + panic("Server already started") + } +- s.Listener = &historyListener{Listener: s.Listener} + s.URL = "http://" + s.Listener.Addr().String() +- s.wrapHandler() +- go s.Config.Serve(s.Listener) ++ s.wrap() ++ s.goServe() + if *serve != "" { + fmt.Fprintln(os.Stderr, "httptest: serving on", s.URL) + select {} +@@ -134,23 +123,10 @@ func (s *Server) StartTLS() { + if len(s.TLS.Certificates) == 0 { + s.TLS.Certificates = []tls.Certificate{cert} + } +- tlsListener := tls.NewListener(s.Listener, s.TLS) +- +- s.Listener = &historyListener{Listener: tlsListener} ++ s.Listener = tls.NewListener(s.Listener, s.TLS) + s.URL = "https://" + s.Listener.Addr().String() +- s.wrapHandler() +- go s.Config.Serve(s.Listener) +-} +- +-func (s *Server) wrapHandler() { +- h := s.Config.Handler +- if h == nil { +- h = http.DefaultServeMux +- } +- s.Config.Handler = &waitGroupHandler{ +- s: s, +- h: h, +- } ++ s.wrap() ++ s.goServe() + } + + // NewTLSServer starts and returns a new Server using TLS. +@@ -161,43 +137,139 @@ func NewTLSServer(handler http.Handler) *Server { + return ts + } + ++type closeIdleTransport interface { ++ CloseIdleConnections() ++} ++ + // 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() +- s.CloseClientConnections() +- if t, ok := http.DefaultTransport.(*http.Transport); ok { ++ s.mu.Lock() ++ if !s.closed { ++ s.closed = true ++ s.Listener.Close() ++ s.Config.SetKeepAlivesEnabled(false) ++ for c, st := range s.conns { ++ if st == http.StateIdle { ++ s.closeConn(c) ++ } ++ } ++ // If this server doesn't shut down in 5 seconds, tell the user why. ++ t := time.AfterFunc(5*time.Second, s.logCloseHangDebugInfo) ++ defer t.Stop() ++ } ++ s.mu.Unlock() ++ ++ // Not part of httptest.Server's correctness, but assume most ++ // users of httptest.Server will be using the standard ++ // transport, so help them out and close any idle connections for them. ++ if t, ok := http.DefaultTransport.(closeIdleTransport); ok { + t.CloseIdleConnections() + } ++ ++ s.wg.Wait() + } + +-// CloseClientConnections closes any currently open HTTP connections +-// to the test Server. ++func (s *Server) logCloseHangDebugInfo() { ++ s.mu.Lock() ++ defer s.mu.Unlock() ++ var buf bytes.Buffer ++ buf.WriteString("httptest.Server blocked in Close after 5 seconds, waiting for connections:\n") ++ for c, st := range s.conns { ++ fmt.Fprintf(&buf, " %T %p %v in state %v\n", c, c, c.RemoteAddr(), st) ++ } ++ log.Print(buf.String()) ++} ++ ++// CloseClientConnections closes any open HTTP connections to the test Server. + func (s *Server) CloseClientConnections() { +- hl, ok := s.Listener.(*historyListener) +- if !ok { +- return ++ s.mu.Lock() ++ defer s.mu.Unlock() ++ for c := range s.conns { ++ s.closeConn(c) + } +- hl.Lock() +- for _, conn := range hl.history { +- conn.Close() ++} ++ ++func (s *Server) goServe() { ++ s.wg.Add(1) ++ go func() { ++ defer s.wg.Done() ++ s.Config.Serve(s.Listener) ++ }() ++} ++ ++// wrap installs the connection state-tracking hook to know which ++// connections are idle. ++func (s *Server) wrap() { ++ oldHook := s.Config.ConnState ++ s.Config.ConnState = func(c net.Conn, cs http.ConnState) { ++ s.mu.Lock() ++ defer s.mu.Unlock() ++ switch cs { ++ case http.StateNew: ++ s.wg.Add(1) ++ if _, exists := s.conns[c]; exists { ++ panic("invalid state transition") ++ } ++ if s.conns == nil { ++ s.conns = make(map[net.Conn]http.ConnState) ++ } ++ s.conns[c] = cs ++ if s.closed { ++ // Probably just a socket-late-binding dial from ++ // the default transport that lost the race (and ++ // thus this connection is now idle and will ++ // never be used). ++ s.closeConn(c) ++ } ++ case http.StateActive: ++ if oldState, ok := s.conns[c]; ok { ++ if oldState != http.StateNew && oldState != http.StateIdle { ++ panic("invalid state transition") ++ } ++ s.conns[c] = cs ++ } ++ case http.StateIdle: ++ if oldState, ok := s.conns[c]; ok { ++ if oldState != http.StateActive { ++ panic("invalid state transition") ++ } ++ s.conns[c] = cs ++ } ++ if s.closed { ++ s.closeConn(c) ++ } ++ case http.StateHijacked, http.StateClosed: ++ s.forgetConn(c) ++ } ++ if oldHook != nil { ++ oldHook(c, cs) ++ } + } +- hl.Unlock() + } + +-// waitGroupHandler wraps a handler, incrementing and decrementing a +-// sync.WaitGroup on each request, to enable Server.Close to block +-// until outstanding requests are finished. +-type waitGroupHandler struct { +- s *Server +- h http.Handler // non-nil ++// closeConn closes c. Except on plan9, which is special. See comment below. ++// s.mu must be held. ++func (s *Server) closeConn(c net.Conn) { ++ if runtime.GOOS == "plan9" { ++ // Go's Plan 9 net package isn't great at unblocking reads when ++ // their underlying TCP connections are closed. Don't trust ++ // that that the ConnState state machine will get to ++ // StateClosed. Instead, just go there directly. Plan 9 may leak ++ // resources if the syscall doesn't end up returning. Oh well. ++ s.forgetConn(c) ++ } ++ go c.Close() + } + +-func (h *waitGroupHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { +- h.s.wg.Add(1) +- defer h.s.wg.Done() // a defer, in case ServeHTTP below panics +- h.h.ServeHTTP(w, r) ++// forgetConn removes c from the set of tracked conns and decrements it from the ++// waitgroup, unless it was previously removed. ++// s.mu must be held. ++func (s *Server) forgetConn(c net.Conn) { ++ if _, ok := s.conns[c]; ok { ++ delete(s.conns, c) ++ s.wg.Done() ++ } + } + + // localhostCert is a PEM-encoded TLS cert with SAN IPs +diff --git a/src/net/http/httptest/server_test.go b/src/net/http/httptest/server_test.go +index 500a9f0..90901ce 100644 +--- a/src/net/http/httptest/server_test.go ++++ b/src/net/http/httptest/server_test.go +@@ -27,3 +27,30 @@ func TestServer(t *testing.T) { + t.Errorf("got %q, want hello", string(got)) + } + } ++ ++// Issue 12781 ++func TestGetAfterClose(t *testing.T) { ++ ts := NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ++ w.Write([]byte("hello")) ++ })) ++ ++ res, err := http.Get(ts.URL) ++ if err != nil { ++ t.Fatal(err) ++ } ++ got, err := ioutil.ReadAll(res.Body) ++ if err != nil { ++ t.Fatal(err) ++ } ++ if string(got) != "hello" { ++ t.Fatalf("got %q, want hello", string(got)) ++ } ++ ++ ts.Close() ++ ++ res, err = http.Get(ts.URL) ++ if err == nil { ++ body, _ := ioutil.ReadAll(res.Body) ++ t.Fatalf("Unexected response after close: %v, %v, %s", res.Status, res.Header, body) ++ } ++} diff --git a/golang.spec b/golang.spec index af418c6..3bdaeff 100644 --- a/golang.spec +++ b/golang.spec @@ -42,7 +42,7 @@ Name: golang Version: 1.5.1 -Release: 0%{?dist} +Release: 1%{?dist} Summary: The Go Programming Language License: BSD @@ -82,6 +82,9 @@ Patch214: go1.5beta2-disable-TestCloneNEWUSERAndRemapNoRootDisableSetgroup # later run `go test -a std`. This makes it only use the zoneinfo.zip where needed in tests. Patch215: ./go1.5-zoneinfo_testing_only.patch +# https://bugzilla.redhat.com/show_bug.cgi?id=1271709 +Patch216: ./golang-1.5.1-a3156aaa12.patch + # Having documentation separate was broken Obsoletes: %{name}-docs < 1.1-4 @@ -206,6 +209,8 @@ Summary: Golang shared object libraries # disable TestCloneNEWUSERAndRemapNoRootDisableSetgroups %patch214 -p1 +%patch216 -p1 + %build # go1.5 bootstrapping. The compiler is written in golang. export GOROOT_BOOTSTRAP=%{goroot} @@ -405,6 +410,9 @@ fi %endif %changelog +* Mon Oct 19 2015 Vincent Batts - 1.5.1-1 +- bz1271709 include patch from upstream fix + * Wed Sep 09 2015 Vincent Batts - 1.5.1-0 - update to go1.5.1