Merge pull request #7833 from jwhonce/issues/7826

Refactor IdleTracker to handle StateIdle transitions

5 files changed, 113 insertions, 91 deletions

diff --git a/pkg/api/handlers/compat/containers_attach.go b/pkg/api/handlers/compat/containers_attach.go
index e20d48d86..4a1196c89 100644
--- a/pkg/api/handlers/compat/containers_attach.go
+++ b/pkg/api/handlers/compat/containers_attach.go
@@ -6,7 +6,7 @@ import (
 	"github.com/containers/podman/v2/libpod"
 	"github.com/containers/podman/v2/libpod/define"
 	"github.com/containers/podman/v2/pkg/api/handlers/utils"
-	"github.com/containers/podman/v2/pkg/api/server/idletracker"
+	"github.com/containers/podman/v2/pkg/api/server/idle"
 	"github.com/gorilla/schema"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -92,7 +92,7 @@ func AttachContainer(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	idleTracker := r.Context().Value("idletracker").(*idletracker.IdleTracker)
+	idleTracker := r.Context().Value("idletracker").(*idle.Tracker)
 	hijackChan := make(chan bool, 1)
 
 	// Perform HTTP attach.
@@ -109,7 +109,7 @@ func AttachContainer(w http.ResponseWriter, r *http.Request) {
 			// We do need to tell the idle tracker that the
 			// connection has been closed, though. We can guarantee
 			// that is true after HTTPAttach exits.
-			idleTracker.TrackHijackedClosed()
+			idleTracker.Close()
 		} else {
 			// A hijack was not successfully completed. We need to
 			// report the error normally.
diff --git a/pkg/api/handlers/compat/exec.go b/pkg/api/handlers/compat/exec.go
index 1db950f85..df51293c2 100644
--- a/pkg/api/handlers/compat/exec.go
+++ b/pkg/api/handlers/compat/exec.go
@@ -10,7 +10,7 @@ import (
 	"github.com/containers/podman/v2/libpod/define"
 	"github.com/containers/podman/v2/pkg/api/handlers"
 	"github.com/containers/podman/v2/pkg/api/handlers/utils"
-	"github.com/containers/podman/v2/pkg/api/server/idletracker"
+	"github.com/containers/podman/v2/pkg/api/server/idle"
 	"github.com/containers/podman/v2/pkg/specgen/generate"
 	"github.com/gorilla/mux"
 	"github.com/pkg/errors"
@@ -174,7 +174,7 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	idleTracker := r.Context().Value("idletracker").(*idletracker.IdleTracker)
+	idleTracker := r.Context().Value("idletracker").(*idle.Tracker)
 	hijackChan := make(chan bool, 1)
 
 	if err := sessionCtr.ExecHTTPStartAndAttach(sessionID, r, w, nil, nil, nil, hijackChan); err != nil {
@@ -186,7 +186,7 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
 			// We do need to tell the idle tracker that the
 			// connection has been closed, though. We can guarantee
 			// that is true after HTTPAttach exits.
-			idleTracker.TrackHijackedClosed()
+			idleTracker.Close()
 		} else {
 			// A hijack was not successfully completed. We need to
 			// report the error normally.
diff --git a/pkg/api/server/idle/tracker.go b/pkg/api/server/idle/tracker.go
new file mode 100644
index 000000000..1b378c492
--- /dev/null
+++ b/pkg/api/server/idle/tracker.go
@@ -0,0 +1,96 @@
+package idle
+
+import (
+	"net"
+	"net/http"
+	"sync"
+	"time"
+
+	"github.com/sirupsen/logrus"
+)
+
+// Tracker holds the state for the server's idle tracking
+type Tracker struct {
+	// Duration is the API idle window
+	Duration time.Duration
+	hijacked int                   // count of active connections managed by handlers
+	managed  map[net.Conn]struct{} // set of active connections managed by http package
+	mux      sync.Mutex            // protect managed map
+	timer    *time.Timer
+	total    int // total number of connections made to this server instance
+}
+
+// NewTracker creates and initializes a new Tracker object
+// For best behavior, duration should be 2x http idle connection timeout
+func NewTracker(idle time.Duration) *Tracker {
+	return &Tracker{
+		managed:  make(map[net.Conn]struct{}),
+		Duration: idle,
+		timer:    time.NewTimer(idle),
+	}
+}
+
+// ConnState is called on HTTP connection state changes.
+// - Once StateHijacked, StateClose is _NOT_ called on that connection
+// - There are two "idle" timeouts, the http idle connection (not to be confused with the TCP/IP idle socket timeout)
+//   and the API idle window.  The caller should set the http idle timeout to 2x the time provided to NewTacker() which
+//   is the API idle window.
+func (t *Tracker) ConnState(conn net.Conn, state http.ConnState) {
+	t.mux.Lock()
+	defer t.mux.Unlock()
+
+	logrus.Debugf("IdleTracker %p:%v %dm+%dh/%dt connection(s)", conn, state, len(t.managed), t.hijacked, t.TotalConnections())
+	switch state {
+	case http.StateNew, http.StateActive:
+		// stop the API timer when the server transitions any connection to an "active" state
+		t.managed[conn] = struct{}{}
+		t.timer.Stop()
+		t.total++
+	case http.StateHijacked:
+		// hijacked connections should call Close() when finished.
+		// Note: If a handler hijack's a connection and then doesn't Close() it,
+		//       the API timer will not fire and the server will _NOT_ timeout.
+		delete(t.managed, conn)
+		t.hijacked++
+	case http.StateIdle:
+		// When any connection goes into the http idle state, we know:
+		// - we have an active connection
+		// - the API timer should not be counting down (See case StateNew/StateActive)
+		break
+	case http.StateClosed:
+		oldActive := t.ActiveConnections()
+
+		// Either the server or a hijacking handler has closed the http connection to a client
+		if _, found := t.managed[conn]; found {
+			delete(t.managed, conn)
+		} else {
+			t.hijacked-- // guarded by t.mux above
+		}
+
+		// Transitioned from any "active" connection to no connections
+		if oldActive > 0 && t.ActiveConnections() == 0 {
+			t.timer.Stop()            // See library source for Reset() issues and why they are not fixed
+			t.timer.Reset(t.Duration) // Restart the API window timer
+		}
+	}
+}
+
+// Close is used to update Tracker that a StateHijacked connection has been closed by handler (StateClosed)
+func (t *Tracker) Close() {
+	t.ConnState(nil, http.StateClosed)
+}
+
+// ActiveConnections returns the number of current managed or StateHijacked connections
+func (t *Tracker) ActiveConnections() int {
+	return len(t.managed) + t.hijacked
+}
+
+// TotalConnections returns total number of connections made to this instance of the service
+func (t *Tracker) TotalConnections() int {
+	return t.total
+}
+
+// Done is called when idle timer has expired
+func (t *Tracker) Done() <-chan time.Time {
+	return t.timer.C
+}
diff --git a/pkg/api/server/idletracker/idletracker.go b/pkg/api/server/idletracker/idletracker.go
deleted file mode 100644
index 1ee905a99..000000000
--- a/pkg/api/server/idletracker/idletracker.go
+++ /dev/null
@@ -1,74 +0,0 @@
-package idletracker
-
-import (
-	"net"
-	"net/http"
-	"sync"
-	"time"
-
-	"github.com/sirupsen/logrus"
-)
-
-type IdleTracker struct {
-	http     map[net.Conn]struct{}
-	hijacked int
-	total    int
-	mux      sync.Mutex
-	timer    *time.Timer
-	Duration time.Duration
-}
-
-func NewIdleTracker(idle time.Duration) *IdleTracker {
-	return &IdleTracker{
-		http:     make(map[net.Conn]struct{}),
-		Duration: idle,
-		timer:    time.NewTimer(idle),
-	}
-}
-
-func (t *IdleTracker) ConnState(conn net.Conn, state http.ConnState) {
-	t.mux.Lock()
-	defer t.mux.Unlock()
-
-	oldActive := t.ActiveConnections()
-	logrus.Debugf("IdleTracker %p:%v %d/%d connection(s)", conn, state, oldActive, t.TotalConnections())
-	switch state {
-	case http.StateNew, http.StateActive:
-		t.http[conn] = struct{}{}
-		// stop the timer if we transitioned from idle
-		if oldActive == 0 {
-			t.timer.Stop()
-		}
-		t.total++
-	case http.StateHijacked:
-		// hijacked connections are handled elsewhere
-		delete(t.http, conn)
-		t.hijacked++
-	case http.StateIdle, http.StateClosed:
-		delete(t.http, conn)
-		// Restart the timer if we've become idle
-		if oldActive > 0 && len(t.http) == 0 {
-			t.timer.Stop()
-			t.timer.Reset(t.Duration)
-		}
-	}
-}
-
-func (t *IdleTracker) TrackHijackedClosed() {
-	t.mux.Lock()
-	defer t.mux.Unlock()
-
-	t.hijacked--
-}
-
-func (t *IdleTracker) ActiveConnections() int {
-	return len(t.http) + t.hijacked
-}
-
-func (t *IdleTracker) TotalConnections() int {
-	return t.total
-}
-
-func (t *IdleTracker) Done() <-chan time.Time {
-	return t.timer.C
-}
diff --git a/pkg/api/server/server.go b/pkg/api/server/server.go
index e7c031234..09a9f6370 100644
--- a/pkg/api/server/server.go
+++ b/pkg/api/server/server.go
@@ -16,7 +16,7 @@ import (
 
 	"github.com/containers/podman/v2/libpod"
 	"github.com/containers/podman/v2/pkg/api/handlers"
-	"github.com/containers/podman/v2/pkg/api/server/idletracker"
+	"github.com/containers/podman/v2/pkg/api/server/idle"
 	"github.com/coreos/go-systemd/v22/activation"
 	"github.com/coreos/go-systemd/v22/daemon"
 	"github.com/gorilla/mux"
@@ -26,14 +26,14 @@ import (
 )
 
 type APIServer struct {
-	http.Server                                 // The  HTTP work happens here
-	*schema.Decoder                             // Decoder for Query parameters to structs
-	context.Context                             // Context to carry objects to handlers
-	*libpod.Runtime                             // Where the real work happens
-	net.Listener                                // mux for routing HTTP API calls to libpod routines
-	context.CancelFunc                          // Stop APIServer
-	idleTracker        *idletracker.IdleTracker // Track connections to support idle shutdown
-	pprof              *http.Server             // Sidecar http server for providing performance data
+	http.Server                      // The  HTTP work happens here
+	*schema.Decoder                  // Decoder for Query parameters to structs
+	context.Context                  // Context to carry objects to handlers
+	*libpod.Runtime                  // Where the real work happens
+	net.Listener                     // mux for routing HTTP API calls to libpod routines
+	context.CancelFunc               // Stop APIServer
+	idleTracker        *idle.Tracker // Track connections to support idle shutdown
+	pprof              *http.Server  // Sidecar http server for providing performance data
 }
 
 // Number of seconds to wait for next request, if exceeded shutdown server
@@ -70,13 +70,13 @@ func newServer(runtime *libpod.Runtime, duration time.Duration, listener *net.Li
 	}
 
 	router := mux.NewRouter().UseEncodedPath()
-	idle := idletracker.NewIdleTracker(duration)
+	idle := idle.NewTracker(duration)
 
 	server := APIServer{
 		Server: http.Server{
 			Handler:           router,
 			ReadHeaderTimeout: 20 * time.Second,
-			IdleTimeout:       duration,
+			IdleTimeout:       duration * 2,
 			ConnState:         idle.ConnState,
 			ErrorLog:          log.New(logrus.StandardLogger().Out, "", 0),
 		},