database/sql: Fix idle connection reuse

Fix idle connection reuse so that ConnMaxIdleTime clears down excessive
idle connections.

This now ensures that db.freeConn is ordered by returnedAt and that
connections that have been idle for the shortest period are reused
first.

In addition connectionCleanerRunLocked updates the next check deadline
based on idle and maximum life time information so that we avoid waiting
up to double MaxIdleTime to close connections.

Corrected the calling timer of connectionCleaner.

Fixes #39471

Change-Id: I6d26b3542179ef35aa13e5265a89bc0f08ba7fa1
Reviewed-on: https://go-review.googlesource.com/c/go/+/237337
Reviewed-by: Tamás Gulácsi <tgulacsi78@gmail.com>
Reviewed-by: Daniel Theophanes <kardianos@gmail.com>
Trust: Ian Lance Taylor <iant@golang.org>

1 files changed, 58 insertions, 28 deletions

diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go
index b40b5c8fe4..e4a5a225b0 100644
--- a/src/database/sql/sql.go
+++ b/src/database/sql/sql.go
@@ -464,8 +464,8 @@ type DB struct {
 	// connections in Stmt.css.
 	numClosed uint64
 
-	mu           sync.Mutex // protects following fields
-	freeConn     []*driverConn
+	mu           sync.Mutex    // protects following fields
+	freeConn     []*driverConn // free connections ordered by returnedAt oldest to newest
 	connRequests map[uint64]chan connRequest
 	nextRequest  uint64 // Next key to use in connRequests.
 	numOpen      int    // number of opened and pending open connections
@@ -1079,7 +1079,7 @@ func (db *DB) connectionCleaner(d time.Duration) {
 			return
 		}
 
-		closing := db.connectionCleanerRunLocked()
+		d, closing := db.connectionCleanerRunLocked(d)
 		db.mu.Unlock()
 		for _, c := range closing {
 			c.Close()
@@ -1088,45 +1088,74 @@ func (db *DB) connectionCleaner(d time.Duration) {
 		if d < minInterval {
 			d = minInterval
 		}
+
+		if !t.Stop() {
+			select {
+			case <-t.C:
+			default:
+			}
+		}
 		t.Reset(d)
 	}
 }
 
-func (db *DB) connectionCleanerRunLocked() (closing []*driverConn) {
-	if db.maxLifetime > 0 {
-		expiredSince := nowFunc().Add(-db.maxLifetime)
-		for i := 0; i < len(db.freeConn); i++ {
+// connectionCleanerRunLocked removes connections that should be closed from
+// freeConn and returns them along side an updated duration to the next check
+// if a quicker check is required to ensure connections are checked appropriately.
+func (db *DB) connectionCleanerRunLocked(d time.Duration) (time.Duration, []*driverConn) {
+	var idleClosing int64
+	var closing []*driverConn
+	if db.maxIdleTime > 0 {
+		// As freeConn is ordered by returnedAt process
+		// in reverse order to minimise the work needed.
+		idleSince := nowFunc().Add(-db.maxIdleTime)
+		last := len(db.freeConn) - 1
+		for i := last; i >= 0; i-- {
 			c := db.freeConn[i]
-			if c.createdAt.Before(expiredSince) {
-				closing = append(closing, c)
-				last := len(db.freeConn) - 1
-				db.freeConn[i] = db.freeConn[last]
-				db.freeConn[last] = nil
-				db.freeConn = db.freeConn[:last]
-				i--
+			if c.returnedAt.Before(idleSince) {
+				i++
+				closing = db.freeConn[:i]
+				db.freeConn = db.freeConn[i:]
+				idleClosing = int64(len(closing))
+				db.maxIdleTimeClosed += idleClosing
+				break
+			}
+		}
+
+		if len(db.freeConn) > 0 {
+			c := db.freeConn[0]
+			if d2 := c.returnedAt.Sub(idleSince); d2 < d {
+				// Ensure idle connections are cleaned up as soon as
+				// possible.
+				d = d2
 			}
 		}
-		db.maxLifetimeClosed += int64(len(closing))
 	}
 
-	if db.maxIdleTime > 0 {
-		expiredSince := nowFunc().Add(-db.maxIdleTime)
-		var expiredCount int64
+	if db.maxLifetime > 0 {
+		expiredSince := nowFunc().Add(-db.maxLifetime)
 		for i := 0; i < len(db.freeConn); i++ {
 			c := db.freeConn[i]
-			if db.maxIdleTime > 0 && c.returnedAt.Before(expiredSince) {
+			if c.createdAt.Before(expiredSince) {
 				closing = append(closing, c)
-				expiredCount++
+
 				last := len(db.freeConn) - 1
-				db.freeConn[i] = db.freeConn[last]
+				// Use slow delete as order is required to ensure
+				// connections are reused least idle time first.
+				copy(db.freeConn[i:], db.freeConn[i+1:])
 				db.freeConn[last] = nil
 				db.freeConn = db.freeConn[:last]
 				i--
+			} else if d2 := c.createdAt.Sub(expiredSince); d2 < d {
+				// Prevent connections sitting the freeConn when they
+				// have expired by updating our next deadline d.
+				d = d2
 			}
 		}
-		db.maxIdleTimeClosed += expiredCount
+		db.maxLifetimeClosed += int64(len(closing)) - idleClosing
 	}
-	return
+
+	return d, closing
 }
 
 // DBStats contains database statistics.
@@ -1272,11 +1301,12 @@ func (db *DB) conn(ctx context.Context, strategy connReuseStrategy) (*driverConn
 	lifetime := db.maxLifetime
 
 	// Prefer a free connection, if possible.
-	numFree := len(db.freeConn)
-	if strategy == cachedOrNewConn && numFree > 0 {
-		conn := db.freeConn[0]
-		copy(db.freeConn, db.freeConn[1:])
-		db.freeConn = db.freeConn[:numFree-1]
+	last := len(db.freeConn) - 1
+	if strategy == cachedOrNewConn && last >= 0 {
+		// Reuse the lowest idle time connection so we can close
+		// connections which remain idle as soon as possible.
+		conn := db.freeConn[last]
+		db.freeConn = db.freeConn[:last]
 		conn.inUse = true
 		if conn.expired(lifetime) {
 			db.maxLifetimeClosed++