database/sql: check conn expiry when returning to pool, not when handing it out

With the original connection reuse strategy, it was possible that
when a new connection was requested, the pool would wait for an
an existing connection to return for re-use in a full connection
pool, and then it would check if the returned connection was expired.
If the returned connection expired while awaiting re-use, it would
return an error to the location requestiong the new connection.
The existing call sites requesting a new connection was often the last
attempt at returning a connection for a query. This would then
result in a failed query.

This change ensures that we perform the expiry check right
before a connection is inserted back in to the connection pool
for while requesting a new connection. If requesting a new connection
it will no longer fail due to the connection expiring.

Fixes #32530

Change-Id: If16379befe0e14d90160219c0c9396243fe062f7
Reviewed-on: https://go-review.googlesource.com/c/go/+/216197
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>

1 files changed, 12 insertions, 1 deletions

diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go
index 4093ffe1bb..2fae0f02ff 100644
--- a/src/database/sql/sql.go
+++ b/src/database/sql/sql.go
@@ -1261,7 +1261,13 @@ func (db *DB) conn(ctx context.Context, strategy connReuseStrategy) (*driverConn
 			if !ok {
 				return nil, errDBClosed
 			}
-			if ret.err == nil && ret.conn.expired(lifetime) {
+			// Only check if the connection is expired if the strategy is cachedOrNewConns.
+			// If we require a new connection, just re-use the connection without looking
+			// at the expiry time. If it is expired, it will be checked when it is placed
+			// back into the connection pool.
+			// This prioritizes giving a valid connection to a client over the exact connection
+			// lifetime, which could expire exactly after this point anyway.
+			if strategy == cachedOrNewConn && ret.err == nil && ret.conn.expired(lifetime) {
 				ret.conn.Close()
 				return nil, driver.ErrBadConn
 			}
@@ -1338,11 +1344,16 @@ func (db *DB) putConn(dc *driverConn, err error, resetSession bool) {
 	}
 	db.mu.Lock()
 	if !dc.inUse {
+		db.mu.Unlock()
 		if debugGetPut {
 			fmt.Printf("putConn(%v) DUPLICATE was: %s\n\nPREVIOUS was: %s", dc, stack(), db.lastPut[dc])
 		}
 		panic("sql: connection returned that was never out")
 	}
+
+	if err != driver.ErrBadConn && dc.expired(db.maxLifetime) {
+		err = driver.ErrBadConn
+	}
 	if debugGetPut {
 		db.lastPut[dc] = stack()
 	}