code review 69260044: net/http: add optional Server.ConnState callback

src/pkg/net/http/server.go

 // Copyright 2009 The Go Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
 // HTTP server.  See RFC 2616.
 
 package http
 
 import (
         "bufio"
@@ skipping 1061 matching lines @@
 // are blacklisted and can't be overridden with alternate
 // implementations.
 func validNPN(proto string) bool {
         switch proto {
         case "", "http/1.1", "http/1.0":
                 return false
         }
         return true
 }
 
+func (c *conn) setState(nc net.Conn, state ConnectionState) {
+        if hook := c.server.ClientConnState; hook != nil {
+                hook(nc, state)
+        }
+}
+
 // Serve a new connection.
 func (c *conn) serve() {
+        origConn := c.rwc // copy it before it's set nil on Close or Hijack
+        c.setState(origConn, StateNew)
         defer func() {
                 if err := recover(); err != nil {
                         const size = 64 << 10
                         buf := make([]byte, size)
                         buf = buf[:runtime.Stack(buf, false)]
                         log.Printf("http: panic serving %v: %v\n%s", c.remoteAddr, err, buf)
                 }
                 if !c.hijacked() {
                         c.close()
+                        c.setState(origConn, StateClosed)
                 }
         }()
 
         if tlsConn, ok := c.rwc.(*tls.Conn); ok {
                 if d := c.server.ReadTimeout; d != 0 {
                         c.rwc.SetReadDeadline(time.Now().Add(d))
                 }
                 if d := c.server.WriteTimeout; d != 0 {
                         c.rwc.SetWriteDeadline(time.Now().Add(d))
                 }
                 if err := tlsConn.Handshake(); err != nil {
                         return
                 }
                 c.tlsState = new(tls.ConnectionState)
                 *c.tlsState = tlsConn.ConnectionState()
                 if proto := c.tlsState.NegotiatedProtocol; validNPN(proto) {
                         if fn := c.server.TLSNextProto[proto]; fn != nil {
                                 h := initNPNRequest{tlsConn, serverHandler{c.server}}
                                 fn(c.server, tlsConn, h)
                         }
                         return
                 }
         }
 
         for {
                 w, err := c.readRequest()
+                // TODO(bradfitz): could push this StateReading
+                // earlier, but in practice header will be all in one
+                // packet/Read:
+                c.setState(c.rwc, StateReading)
                 if err != nil {
                         if err == errTooLarge {
                                 // Their HTTP client may or may not be
                                 // able to read this if we're
                                 // responding to them and hanging up
                                 // while they're still writing their
                                 // request.  Undefined behavior.
                                 io.WriteString(c.rwc, "HTTP/1.1 413 Request Entity Too Large\r\n\r\n")
                                 c.closeWriteAndWait()
                                 break
@@ skipping 25 matching lines @@
                         break
                 }
 
                 // HTTP cannot have multiple simultaneous active requests.[*]
                 // Until the server replies to this request, it can't read another,
                 // so we might as well run the handler in this goroutine.
                 // [*] Not strictly true: HTTP pipelining.  We could let them all process
                 // in parallel even if their responses need to be serialized.
                 serverHandler{c.server}.ServeHTTP(w, w.req)
                 if c.hijacked() {
+                        c.setState(origConn, StateHijacked)
                         return
                 }
                 w.finishRequest()
                 if w.closeAfterReply {
                         if w.requestBodyLimitHit {
                                 c.closeWriteAndWait()
                         }
                         break
                 }
+                c.setState(c.rwc, StateIdle)
         }
 }
 
 func (w *response) sendExpectationFailed() {
         // TODO(bradfitz): let ServeHTTP handlers handle
         // requests with non-standard expectation[s]? Seems
         // theoretical at best, and doesn't fit into the
         // current ServeHTTP model anyway.  We'd need to
         // make the ResponseWriter an optional
         // "ExpectReplier" interface or something.
@@ skipping 390 matching lines @@
         TLSConfig      *tls.Config   // optional TLS config, used by ListenAndServeTLS
 
         // TLSNextProto optionally specifies a function to take over
         // ownership of the provided TLS connection when an NPN
         // protocol upgrade has occurred.  The map key is the protocol
         // name negotiated. The Handler argument should be used to
         // handle HTTP requests and will initialize the Request's TLS
         // and RemoteAddr if not already set.  The connection is
         // automatically closed when the function returns.
         TLSNextProto map[string]func(*Server, *tls.Conn, Handler)
+
+        // ClientConnState optionally specifies a callback function
+        // to note the state a connection is in.
+        ClientConnState func(net.Conn, ConnectionState)

[josharian, 2014/02/26 23:07:56]

Perhaps just ConnState? Not obvious to me what else it could be confused with.

[bradfitz, 2014/02/26 23:16:43]

I'm okay with that too, but there's also precedent for my naming:
http://golang.org/pkg/crypto/tls/#ConnectionState

But that's a struct anyway. And if we go with ConnState, then maybe we could
re-use it for state-tracking conns in the HTTP client too (unlikely).

[josharian, 2014/02/26 23:41:57]

Sorry, I was unclear. I was suggesting s/ClientConnState/ConnState/ here.

+}
+
+// A ConnectionState represents the

[josharian, 2014/02/26 23:07:56]

The second half of this comment is missing.

+type ConnectionState int

[josharian, 2014/02/26 23:07:56]

Maybe just ConnState, to match net.Conn and ClientConnState above?

[bradfitz, 2014/02/26 23:16:43]

See above.

+
+const (
+        // StateNew represents a new connection that is expected to
+        // send a request immediately.
+        StateNew ConnectionState = iota
+
+        // StateReading represent a connection that is reading its request.

[josharian, 2014/02/26 23:07:56]

s/represent/represents/
s/.//

[bradfitz, 2014/02/26 23:16:43]

Done.

+        // and is about to be in a Handler.
+        StateReading
+

[josharian, 2014/02/26 23:07:56]

Seems like there's a missing state here, StateHandler(?).

// StateHandler represents a connection that has read its request and is in a
Handler.

Unless I'm missing something, without such a state, the connection state watcher
can't distinguish between a state stuck waiting for a request read to complete
and a state stuck in a slow Handler.

[bradfitz, 2014/02/26 23:16:43]

I'd rather not introduce that. That's pretty much what this state already means.
The two seem redundant. I've updated the comment instead.

+        // StateIdle represents a connection that has finished a request

[josharian, 2014/02/26 23:07:56]

s/finished a request/finished handling a request/

[bradfitz, 2014/02/26 23:16:43]

Done.

+        // and is in the keep-alive state, waiting for a new request.
+        StateIdle
+
+        // StateHijacked occurs when a connection is hijacked.
+        // This is a terminal state. It does not transition to
+        // StateClose.

[rcrowley, 2014/02/27 01:44:08]

s/StateClose/StateClosed/

+        StateHijacked
+
+        // StateClosed occurs when a connection is closed.
+        // Hijacked connections do not transition to StateClose.

[rcrowley, 2014/02/27 01:44:08]

s/StateClose/StateClosed/

+        StateClosed
+)
+
+var stateName = map[ConnectionState]string{
+        StateNew:      "new",
+        StateReading:  "reading",
+        StateIdle:     "idle",
+        StateHijacked: "hijacked",
+        StateClosed:   "closed",
+}
+
+func (c ConnectionState) String() string {
+        return stateName[c]
 }
 
 // serverHandler delegates to either the server's Handler or
 // DefaultServeMux and also handles "OPTIONS *" requests.
 type serverHandler struct {
         srv *Server
 }
 
 func (sh serverHandler) ServeHTTP(rw ResponseWriter, req *Request) {
         handler := sh.srv.Handler
@@ skipping 344 matching lines @@
         log.Printf("%s.Read(%d) = %d, %v", c.name, len(p), n, err)
         return
 }
 
 func (c *loggingConn) Close() (err error) {
         log.Printf("%s.Close() = ...", c.name)
         err = c.Conn.Close()
         log.Printf("%s.Close() = %v", c.name, err)
         return
 }