review fixes

* move constant definitions
* always give the client at least quitTimeout to respond to ping,
  even if registerTimeout or quitTimeout are longer than idleTimeout

irc/client.go

@@ -25,12 +25,6 @@ import (
 )
 
 const (
-	// RegisterTimeout is how long clients have to register before we disconnect them
-	RegisterTimeout = time.Minute
-	// IdleTimeout is how long without traffic before a registered client is considered idle.
-	IdleTimeout = time.Minute + time.Second*30
-	// QuitTimeout is how long without traffic before an idle client is disconnected
-	QuitTimeout = time.Minute
 	// IdentTimeoutSeconds is how many seconds before our ident (username) check times out.
 	IdentTimeoutSeconds = 1.5
 )

irc/idletimer.go

@@ -9,6 +9,15 @@ import (
 	"time"
 )
 
+const (
+	// RegisterTimeout is how long clients have to register before we disconnect them
+	RegisterTimeout = time.Minute
+	// IdleTimeout is how long without traffic before a registered client is considered idle.
+	IdleTimeout = time.Minute + time.Second*30
+	// QuitTimeout is how long without traffic before an idle client is disconnected
+	QuitTimeout = time.Minute
+)
+
 // client idleness state machine
 
 type TimerState uint
@@ -49,6 +58,7 @@ func NewIdleTimer(client *Client) *IdleTimer {
 // it will eventually be stopped.
 func (it *IdleTimer) Start() {
 	it.Lock()
+	it.state = TimerUnregistered
 	it.lastSeen = time.Now()
 	it.Unlock()
 	go it.mainLoop()
@@ -66,50 +76,47 @@ func (it *IdleTimer) mainLoop() {
 			return
 		}
 
-		registered := client.Registered()
+		idleTime := time.Now().Sub(lastSeen)
-		now := time.Now()
+		var nextSleep time.Duration
-		idleTime := now.Sub(lastSeen)
-		newState := state
 
-		switch state {
+		if state == TimerUnregistered {
-		case TimerUnregistered:
+			if client.Registered() {
-			if registered {
+				// transition to active, process new deadlines below
-				// transition to TimerActive state
+				state = TimerActive
-				newState = TimerActive
+			} else {
+				nextSleep = it.registerTimeout - idleTime
 			}
-		case TimerActive:
+		} else if state == TimerIdle {
-			if idleTime >= IdleTimeout {
+			if idleTime < it.quitTimeout {
-				newState = TimerIdle
+				// new ping came in after we transitioned to TimerIdle,
-				client.Ping()
+				// transition back to active and process deadlines below
-			}
+				state = TimerActive
-		case TimerIdle:
+			} else {
-			if idleTime < IdleTimeout {
+				nextSleep = 0
-				// new ping came in after we transitioned to TimerIdle
-				newState = TimerActive
 			}
 		}
 
-		it.Lock()
+		if state == TimerActive {
-		it.state = newState
-		it.Unlock()
-
-		var nextSleep time.Duration
-		switch newState {
-		case TimerUnregistered:
-			nextSleep = it.registerTimeout - idleTime
-		case TimerActive:
 			nextSleep = it.idleTimeout - idleTime
-		case TimerIdle:
+			if nextSleep <= 0 {
-			nextSleep = (it.idleTimeout + it.quitTimeout) - idleTime
+				state = TimerIdle
+				client.Ping()
+				// grant the client at least quitTimeout to respond
+				nextSleep = it.quitTimeout
+			}
 		}
 
 		if nextSleep <= 0 {
 			// ran out of time, hang them up
-			client.Quit(it.quitMessage(newState))
+			client.Quit(it.quitMessage(state))
 			client.destroy()
 			return
 		}
 
+		it.Lock()
+		it.state = state
+		it.Unlock()
+
 		time.Sleep(nextSleep)
 	}
 }