From: Brad Fitzpatrick Date: Thu, 28 Jun 2018 21:01:45 +0000 (+0000) Subject: net/http: make Server.Shutdown treat new connections as idle after 5 seconds X-Git-Tag: go1.11beta2~259 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=a79fe5354fad08086c0a2018661f06a5f2e6eaa3;p=gostls13.git net/http: make Server.Shutdown treat new connections as idle after 5 seconds The Server distinguishes "new" vs "idle" connections. A TCP connection from which no bytes have yet been written is "new". A connection that has previously served a request and is in "keep-alive" state while waiting for a second or further request is "idle". The graceful Server.Shutdown historically only shut down "idle" connections, with the assumption that a "new" connection was about to read its request and would then shut down on its own afterwards. But apparently some clients spin up connections and don't end up using them, so we have something that's "new" to us, but browsers or other clients are treating as "idle" to them. This CL tweaks our heuristic to treat a StateNew connection as StateIdle if it's been stuck in StateNew for over 5 seconds. Fixes #22682 Change-Id: I01ba59a6ab67755ca5ab567041b1f54aa7b7da6f Reviewed-on: https://go-review.googlesource.com/121419 Run-TryBot: Brad Fitzpatrick Reviewed-by: Ian Lance Taylor TryBot-Result: Gobot Gobot --- diff --git a/src/net/http/export_test.go b/src/net/http/export_test.go index e0ceb40021..7c7b5d5667 100644 --- a/src/net/http/export_test.go +++ b/src/net/http/export_test.go @@ -198,8 +198,8 @@ func (s *Server) ExportAllConnsIdle() bool { s.mu.Lock() defer s.mu.Unlock() for c := range s.activeConn { - st, ok := c.curState.Load().(ConnState) - if !ok || st != StateIdle { + st, unixSec := c.getState() + if unixSec == 0 || st != StateIdle { return false } } diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index de76f5eab0..5ab17a649e 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -14,6 +14,7 @@ import ( "crypto/tls" "encoding/json" "errors" + "flag" "fmt" "internal/testenv" "io" @@ -5562,6 +5563,64 @@ func testServerShutdown(t *testing.T, h2 bool) { } } +var slowTests = flag.Bool("slow", false, "run slow tests") + +func TestServerShutdownStateNew(t *testing.T) { + if !*slowTests { + t.Skip("skipping slow test without -slow flag") + } + setParallel(t) + defer afterTest(t) + + ts := httptest.NewServer(HandlerFunc(func(w ResponseWriter, r *Request) { + // nothing. + })) + defer ts.Close() + + // Start a connection but never write to it. + c, err := net.Dial("tcp", ts.Listener.Addr().String()) + if err != nil { + t.Fatal(err) + } + defer c.Close() + + shutdownRes := make(chan error, 1) + go func() { + shutdownRes <- ts.Config.Shutdown(context.Background()) + }() + readRes := make(chan error, 1) + go func() { + _, err := c.Read([]byte{0}) + readRes <- err + }() + + const expectTimeout = 5 * time.Second + t0 := time.Now() + select { + case got := <-shutdownRes: + d := time.Since(t0) + if got != nil { + t.Fatalf("shutdown error after %v: %v", d, err) + } + if d < expectTimeout/2 { + t.Errorf("shutdown too soon after %v", d) + } + case <-time.After(expectTimeout * 3 / 2): + t.Fatalf("timeout waiting for shutdown") + } + + // Wait for c.Read to unblock; should be already done at this point, + // or within a few milliseconds. + select { + case err := <-readRes: + if err == nil { + t.Error("expected error from Read") + } + case <-time.After(2 * time.Second): + t.Errorf("timeout waiting for Read to unblock") + } +} + // Issue 17878: tests that we can call Close twice. func TestServerCloseDeadlock(t *testing.T) { var s Server diff --git a/src/net/http/server.go b/src/net/http/server.go index fc3106d38d..5349c39c61 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -283,7 +283,7 @@ type conn struct { curReq atomic.Value // of *response (which has a Request in it) - curState atomic.Value // of ConnState + curState struct{ atomic uint64 } // packed (unixtime<<8|uint8(ConnState)) // mu guards hijackedv mu sync.Mutex @@ -1679,21 +1679,19 @@ func (c *conn) setState(nc net.Conn, state ConnState) { case StateHijacked, StateClosed: srv.trackConn(c, false) } - c.curState.Store(connStateInterface[state]) + if state > 0xff || state < 0 { + panic("internal error") + } + packedState := uint64(time.Now().Unix()<<8) | uint64(state) + atomic.StoreUint64(&c.curState.atomic, packedState) if hook := srv.ConnState; hook != nil { hook(nc, state) } } -// connStateInterface is an array of the interface{} versions of -// ConnState values, so we can use them in atomic.Values later without -// paying the cost of shoving their integers in an interface{}. -var connStateInterface = [...]interface{}{ - StateNew: StateNew, - StateActive: StateActive, - StateIdle: StateIdle, - StateHijacked: StateHijacked, - StateClosed: StateClosed, +func (c *conn) getState() (state ConnState, unixSec int64) { + packedState := atomic.LoadUint64(&c.curState.atomic) + return ConnState(packedState & 0xff), int64(packedState >> 8) } // badRequestError is a literal string (used by in the server in HTML, @@ -2624,8 +2622,16 @@ func (s *Server) closeIdleConns() bool { defer s.mu.Unlock() quiescent := true for c := range s.activeConn { - st, ok := c.curState.Load().(ConnState) - if !ok || st != StateIdle { + st, unixSec := c.getState() + // Issue 22682: treat StateNew connections as if + // they're idle if we haven't read the first request's + // header in over 5 seconds. + if st == StateNew && unixSec < time.Now().Unix()-5 { + st = StateIdle + } + if st != StateIdle || unixSec == 0 { + // Assume unixSec == 0 means it's a very new + // connection, without state set yet. quiescent = false continue }