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>

diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go
index 4093ffe1bb064559958111c1598e8d4f278fce85..2fae0f02ff4ad8b84bb97c8e679843fd4a7b6f97 100644 (file)
--- 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 f08eba93b3c51a1705b993a695f06567bb672eaa..e9b5c8a228df70145c3b543e4e5a2367de9ffb12 100644 (file)
--- 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) {