diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go index 4093ffe1bb0..2fae0f02ff4 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() } diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go index f08eba93b3c..e9b5c8a228d 100644 --- a/src/database/sql/sql_test.go +++ b/src/database/sql/sql_test.go @@ -2707,6 +2707,95 @@ func TestManyErrBadConn(t *testing.T) { } } +// Issue32530 encounters an issue where a connection may +// expire right after it comes out of a used connection pool +// even when a new connection is requested. +func TestConnExpiresFreshOutOfPool(t *testing.T) { + execCases := []struct { + expired bool + badReset bool + }{ + {false, false}, + {true, false}, + {false, true}, + } + + t0 := time.Unix(1000000, 0) + offset := time.Duration(0) + offsetMu := sync.RWMutex{} + + nowFunc = func() time.Time { + offsetMu.RLock() + defer offsetMu.RUnlock() + return t0.Add(offset) + } + defer func() { nowFunc = time.Now }() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + db := newTestDB(t, "magicquery") + defer closeDB(t, db) + + db.SetMaxOpenConns(1) + + for _, ec := range execCases { + ec := ec + name := fmt.Sprintf("expired=%t,badReset=%t", ec.expired, ec.badReset) + t.Run(name, func(t *testing.T) { + db.clearAllConns(t) + + db.SetMaxIdleConns(1) + db.SetConnMaxLifetime(10 * time.Second) + + conn, err := db.conn(ctx, alwaysNewConn) + if err != nil { + t.Fatal(err) + } + + afterPutConn := make(chan struct{}) + waitingForConn := make(chan struct{}) + + go func() { + conn, err := db.conn(ctx, alwaysNewConn) + if err != nil { + t.Fatal(err) + } + db.putConn(conn, err, false) + close(afterPutConn) + }() + go func() { + for { + db.mu.Lock() + ct := len(db.connRequests) + db.mu.Unlock() + if ct > 0 { + close(waitingForConn) + return + } + time.Sleep(10 * time.Millisecond) + } + }() + + <-waitingForConn + + offsetMu.Lock() + if ec.expired { + offset = 11 * time.Second + } else { + offset = time.Duration(0) + } + offsetMu.Unlock() + + conn.ci.(*fakeConn).stickyBad = ec.badReset + + db.putConn(conn, err, true) + + <-afterPutConn + }) + } +} + // TestIssue20575 ensures the Rows from query does not block // closing a transaction. Ensure Rows is closed while closing a trasaction. func TestIssue20575(t *testing.T) {