From b2cff7e091ca1f59ceb77c4dd3acaf0c150b5440 Mon Sep 17 00:00:00 2001 From: Daniel Theophanes Date: Thu, 23 Jan 2020 18:18:39 -0800 Subject: 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 --- src/database/sql/sql.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) (limited to 'src/database/sql/sql.go') 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() } -- cgit v1.3