From 74f99d0933d5c201fc17d90ab612cd1a9c7d425f Mon Sep 17 00:00:00 2001 From: Steven Hartland Date: Tue, 9 Jun 2020 08:58:08 +0100 Subject: database/sql: Fix idle connection reuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Daniel Theophanes Trust: Ian Lance Taylor --- src/database/sql/sql.go | 86 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 58 insertions(+), 28 deletions(-) (limited to 'src/database/sql/sql.go') 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++ -- cgit v1.3