Close idle connections after graceful shutdown timeout

[tpng]

Since the related changes in Go stdlib (golang/go#9478, golang/go#14204) is pushed back to Go 1.8 (ETA 2017 Feb), I think it is good time to make this change in Caddy as 0.9 is merged in master.

Closes #809.

[mholt]

Interesting -- if this works, I like it: simple and minimal. It probably deserves a comment with a link to that issue.

Does this actually work though? I've never tried setting the ConnState function when trying to stop the server, I have only set it before starting the server. (Are there any race conditions of concern?)

[tpng]

These are the related issues:
golang/go#9478
golang/go#14204

I have tested before with Chrome and the idle connections are closed with ConnState set inside Stop().
It is racey in nature though, net/http checking ConnState is nil and calling it vs setting it on Caddy side.

[tpng]

To avoid the race we need to setup ConnState before calling Serve() and check if we are shutting down in the ConnState hook, if yes then we close the idle connections.

For the shutdown check, I think we can set s.listener to nil inside Stop() after calling s.listener.Close() and check it inside ConnState.

Something like:

// NewServer creates a new Server instance that will listen on addr
// and will serve the sites configured in group.
func NewServer(addr string, group []*SiteConfig) (*Server, error) {
    s := &Server{
        Server: &http.Server{
            Addr: addr,
            // TODO: Make these values configurable?
            // ReadTimeout:    2 * time.Minute,
            // WriteTimeout:   2 * time.Minute,
            // MaxHeaderBytes: 1 << 16,
        },
        vhosts:      newVHostTrie(),
        sites:       group,
        connTimeout: GracefulTimeout,
    }
    s.Server.Handler = s // this is weird, but whatever
    s.Server.ConnState = func(c net.Conn, cs http.ConnState) {
        if cs == http.StateIdle {
            s.listenerMu.Lock()
            // server closed, reap idle connection
            if s.listener == nil {
                c.Close()
            }
            s.listenerMu.Unlock()
        }
    }

    // codes after
}

// Stop stops s gracefully (or forcefully after timeout) and
// closes its listener.
func (s *Server) Stop() (err error) {
    // codes before

    // Close the listener now; this stops the server without delay
    s.listenerMu.Lock()
    if s.listener != nil {
        err = s.listener.Close()
        s.listener = nil
    }
    s.listenerMu.Unlock()

    // codes after
}

[mholt]

@tpng Ah, this is good! Thank you!! I'm going to merge it in so it will go out on the first beta version coming up.

[FiloSottile]

I'm a bit confused: no connection should transition to state Idle after a call to SetKeepAlivesEnabled.

[tpng]

keepAlives = true:
New -> Active -> Serve the request -> Idle -> New request on same connection -> Active -> ...
keepAlives = false:
New -> Active -> Serve the request -> Closed

From https://golang.org/pkg/net/http/#ConnState:

    // StateIdle represents a connection that has finished
    // handling a request and is in the keep-alive state, waiting
    // for a new request. Connections transition from StateIdle
    // to either StateActive or StateClosed.

[FiloSottile]

Yep, that's my same understanding. So in this PR, SetKeepAlivesEnabled is called first, making transitions to StateIdle impossible, and then listener is set to nil. So the if that fires on both StateIdle and listener == nil should be unreachable, no?

[tpng]

Currently, SetKeepAlivesEnabled only affect new connections to the server (golang/go#9478), which is why this PR is required in the first place.
That if block is for connections we opened before closing the listener.

[FiloSottile]

Ah, ok, that would make sense. But my understanding was that SetKeepAlivesEnabled also affects existing connections that transitioned to StateActive in the meantime.

StateActive -> StateIdle -> SetKeepAlivesEnabled(false) -> StateActive -> StateClosed (not StateIdle)

I guess I'm looking at the source to confirm.

[FiloSottile]

SetKeepAlivesEnabled changes the doKeepAlives return value, which is used in writeHeader for every request. So I think I'm correct saying that SetKeepAlivesEnabled affects all future requests, not connections, and after a SetKeepAlivesEnabled(false) call there won't be any state transitions to StateIdle.

[tpng]

Now I remember why, it's because HTTP2 only keep one and only one persistent connection for all requests and is not affected by SetKeepAlivesEnabled.

[FiloSottile]

Ah, that makes sense, and TIL. There's golang/go#15386 that is related, but I can't find one about HTTP/2 not being affected by SetKeepAlivesEnabled. Is there any particular reason? Should we open one?

[tpng]

I believe golang/go#9478 will fix this issue.