diff options
author | Daniel Theophanes <kardianos@gmail.com> | 2020-01-23 18:18:39 -0800 |
---|---|---|
committer | Daniel Theophanes <kardianos@gmail.com> | 2020-04-20 17:41:27 +0000 |
commit | b2cff7e091ca1f59ceb77c4dd3acaf0c150b5440 (patch) | |
tree | ee6c91359f1908d6bb94bdf294242a56179dd6f9 /src/database | |
parent | f8ff12d480dcfe0db17648939644d0eeec0ed0fb (diff) | |
download | go-b2cff7e091ca1f59ceb77c4dd3acaf0c150b5440.tar.gz go-b2cff7e091ca1f59ceb77c4dd3acaf0c150b5440.zip |
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>
Diffstat (limited to 'src/database')
-rw-r--r-- | src/database/sql/sql.go | 13 | ||||
-rw-r--r-- | src/database/sql/sql_test.go | 89 |
2 files changed, 101 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() } diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go index f08eba93b3..e9b5c8a228 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) { |