From bb00a8d97faa70bf7a1cbdd4a43e95347a9c8709 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 14 Nov 2016 22:11:05 +0000 Subject: [PATCH] net/http: update bundled http2, add TestServerKeepAlivesEnabled h1/h2 tests Updates x/net/http2 to x/net git rev 6dfeb344 for: http2: make Server respect http1 Server's SetKeepAlivesEnabled https://golang.org/cl/33153 And adds a test in std. Fixes #17717 Change-Id: I3ba000abb6f3f682261e105d8a4bb93bde6609fe Reviewed-on: https://go-review.googlesource.com/33231 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Tom Bergan --- src/net/http/clientserver_test.go | 13 ++++++++++ src/net/http/h2_bundle.go | 40 ++++++++++++++++++++++++------- src/net/http/serve_test.go | 26 ++++++++++++++++++++ 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/src/net/http/clientserver_test.go b/src/net/http/clientserver_test.go index 3c4b7773a1..e736e7c7dd 100644 --- a/src/net/http/clientserver_test.go +++ b/src/net/http/clientserver_test.go @@ -44,6 +44,19 @@ func (t *clientServerTest) close() { t.ts.Close() } +func (t *clientServerTest) getURL(u string) string { + res, err := t.c.Get(u) + if err != nil { + t.t.Fatal(err) + } + defer res.Body.Close() + slurp, err := ioutil.ReadAll(res.Body) + if err != nil { + t.t.Fatal(err) + } + return string(slurp) +} + func (t *clientServerTest) scheme() string { if t.h2 { return "https" diff --git a/src/net/http/h2_bundle.go b/src/net/http/h2_bundle.go index 20178dadf1..085a6fab54 100644 --- a/src/net/http/h2_bundle.go +++ b/src/net/http/h2_bundle.go @@ -3549,7 +3549,7 @@ func (sc *http2serverConn) serve() { return case <-gracefulShutdownCh: gracefulShutdownCh = nil - sc.goAwayIn(http2ErrCodeNo, 0) + sc.startGracefulShutdown() case <-sc.shutdownTimerCh: sc.vlogf("GOAWAY close timer fired; closing conn from %v", sc.conn.RemoteAddr()) return @@ -3834,6 +3834,13 @@ func (sc *http2serverConn) scheduleFrameWrite() { sc.inFrameScheduleLoop = false } +// startGracefulShutdown sends a GOAWAY with ErrCodeNo to tell the +// client we're gracefully shutting down. The connection isn't closed +// until all current streams are done. +func (sc *http2serverConn) startGracefulShutdown() { + sc.goAwayIn(http2ErrCodeNo, 0) +} + func (sc *http2serverConn) goAway(code http2ErrCode) { sc.serveG.check() var forceCloseIn time.Duration @@ -4028,12 +4035,15 @@ func (sc *http2serverConn) closeStream(st *http2stream, err error) { } else { sc.curClientStreams-- } - if sc.curClientStreams+sc.curPushedStreams == 0 { - sc.setConnState(StateIdle) - } delete(sc.streams, st.id) - if len(sc.streams) == 0 && sc.srv.IdleTimeout != 0 { - sc.idleTimer.Reset(sc.srv.IdleTimeout) + if len(sc.streams) == 0 { + sc.setConnState(StateIdle) + if sc.srv.IdleTimeout != 0 { + sc.idleTimer.Reset(sc.srv.IdleTimeout) + } + if http2h1ServerKeepAlivesDisabled(sc.hs) { + sc.startGracefulShutdown() + } } if p := st.body; p != nil { @@ -4177,7 +4187,7 @@ func (sc *http2serverConn) processGoAway(f *http2GoAwayFrame) error { } else { sc.vlogf("http2: received GOAWAY %+v, starting graceful shutdown", f) } - sc.goAwayIn(http2ErrCodeNo, 0) + sc.startGracefulShutdown() sc.pushEnabled = false return nil @@ -5181,7 +5191,7 @@ func (sc *http2serverConn) startPush(msg http2startPushRequest) { } if sc.maxPushPromiseID+2 >= 1<<31 { - sc.goAwayIn(http2ErrCodeNo, 0) + sc.startGracefulShutdown() return 0, http2ErrPushLimitReached } sc.maxPushPromiseID += 2 @@ -5326,6 +5336,20 @@ func http2h1ServerShutdownChan(hs *Server) <-chan struct{} { // optional test hook for h1ServerShutdownChan. var http2testh1ServerShutdownChan func(hs *Server) <-chan struct{} +// h1ServerKeepAlivesDisabled reports whether hs has its keep-alives +// disabled. See comments on h1ServerShutdownChan above for why +// the code is written this way. +func http2h1ServerKeepAlivesDisabled(hs *Server) bool { + var x interface{} = hs + type I interface { + doKeepAlives() bool + } + if hs, ok := x.(I); ok { + return !hs.doKeepAlives() + } + return false +} + const ( // transportDefaultConnFlow is how many connection-level flow control // tokens we give the server at start-up, past the default 64k. diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 0c5af6bca4..54b02a8b28 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -4992,3 +4992,29 @@ func TestServerCloseDeadlock(t *testing.T) { s.Close() s.Close() } + +// Issue 17717: tests that Server.SetKeepAlivesEnabled is respected by +// both HTTP/1 and HTTP/2. +func TestServerKeepAlivesEnabled_h1(t *testing.T) { testServerKeepAlivesEnabled(t, h1Mode) } +func TestServerKeepAlivesEnabled_h2(t *testing.T) { testServerKeepAlivesEnabled(t, h2Mode) } +func testServerKeepAlivesEnabled(t *testing.T, h2 bool) { + setParallel(t) + defer afterTest(t) + cst := newClientServerTest(t, h2, HandlerFunc(func(w ResponseWriter, r *Request) { + fmt.Fprintf(w, "%v", r.RemoteAddr) + })) + defer cst.close() + srv := cst.ts.Config + srv.SetKeepAlivesEnabled(false) + a := cst.getURL(cst.ts.URL) + if !waitCondition(2*time.Second, 10*time.Millisecond, srv.ExportAllConnsIdle) { + t.Fatalf("test server has active conns") + } + b := cst.getURL(cst.ts.URL) + if a == b { + t.Errorf("got same connection between first and second requests") + } + if !waitCondition(2*time.Second, 10*time.Millisecond, srv.ExportAllConnsIdle) { + t.Fatalf("test server has active conns") + } +} -- 2.50.0