database/sql: prevent closes slices from assigning to free conn

In function connectionCleanerRunLocked append to closing slice affects db.freeConns and vise versa. Sometimes valid connections are closed and some invalid not.

Change-Id: I5282f15be3e549533b7d994b17b2060db3c0e7da
GitHub-Last-Rev: b3eb3ab6f49c036519f777fc7189e9507010c166
GitHub-Pull-Request: golang/go#49429
Reviewed-on: https://go-review.googlesource.com/c/go/+/362214
Reviewed-by: Daniel Theophanes <kardianos@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go
index 5131c08b511f4d14c22ebf0f8014b351dcf224f0..c5b4f50aa744004c66d792310366e0f79f0cfbac 100644 (file)
--- a/src/database/sql/sql.go
+++ b/src/database/sql/sql.go
@@ -1115,7 +1115,7 @@ func (db *DB) connectionCleanerRunLocked(d time.Duration) (time.Duration, []*dri
                        c := db.freeConn[i]
                        if c.returnedAt.Before(idleSince) {
                                i++
-                               closing = db.freeConn[:i]
+                               closing = db.freeConn[:i:i]
                                db.freeConn = db.freeConn[i:]
                                idleClosing = int64(len(closing))
                                db.maxIdleTimeClosed += idleClosing

diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go
index 889adc3164ed4d6294fb70912af5df7622ee918c..b887b40d713606d630b9ae7ee2243cff5547222a 100644 (file)
--- a/src/database/sql/sql_test.go
+++ b/src/database/sql/sql_test.go
@@ -3910,6 +3910,10 @@ func testUseConns(t *testing.T, count int, tm time.Time, db *DB) time.Time {
        conns := make([]*Conn, count)
        ctx := context.Background()
        for i := range conns {
+               tm = tm.Add(time.Nanosecond)
+               nowFunc = func() time.Time {
+                       return tm
+               }
                c, err := db.Conn(ctx)
                if err != nil {
                        t.Error(err)
@@ -3917,12 +3921,12 @@ func testUseConns(t *testing.T, count int, tm time.Time, db *DB) time.Time {
                conns[i] = c
        }
 
-       for _, c := range conns {
+       for i := len(conns) - 1; i >= 0; i-- {
                tm = tm.Add(time.Nanosecond)
                nowFunc = func() time.Time {
                        return tm
                }
-               if err := c.Close(); err != nil {
+               if err := conns[i].Close(); err != nil {
                        t.Error(err)
                }
        }
@@ -3934,18 +3938,46 @@ func TestMaxIdleTime(t *testing.T) {
        usedConns := 5
        reusedConns := 2
        list := []struct {
-               wantMaxIdleTime time.Duration
-               wantNextCheck   time.Duration
-               wantIdleClosed  int64
-               timeOffset      time.Duration
+               wantMaxIdleTime   time.Duration
+               wantMaxLifetime   time.Duration
+               wantNextCheck     time.Duration
+               wantIdleClosed    int64
+               wantMaxIdleClosed int64
+               timeOffset        time.Duration
+               secondTimeOffset  time.Duration
        }{
                {
                        time.Millisecond,
+                       0,
                        time.Millisecond - time.Nanosecond,
                        int64(usedConns - reusedConns),
+                       int64(usedConns - reusedConns),
+                       10 * time.Millisecond,
+                       0,
+               },
+               {
+                       // Want to close some connections via max idle time and one by max lifetime.
+                       time.Millisecond,
+                       // nowFunc() - MaxLifetime should be 1 * time.Nanosecond in connectionCleanerRunLocked.
+                       // This guarantees that first opened connection is to be closed.
+                       // Thus it is timeOffset + secondTimeOffset + 3 (+2 for Close while reusing conns and +1 for Conn).
+                       10*time.Millisecond + 100*time.Nanosecond + 3*time.Nanosecond,
+                       time.Nanosecond,
+                       // Closed all not reused connections and extra one by max lifetime.
+                       int64(usedConns - reusedConns + 1),
+                       int64(usedConns - reusedConns),
                        10 * time.Millisecond,
+                       // Add second offset because otherwise connections are expired via max lifetime in Close.
+                       100 * time.Nanosecond,
                },
-               {time.Hour, time.Second, 0, 10 * time.Millisecond},
+               {
+                       time.Hour,
+                       0,
+                       time.Second,
+                       0,
+                       0,
+                       10 * time.Millisecond,
+                       0},
        }
        baseTime := time.Unix(0, 0)
        defer func() {
@@ -3962,18 +3994,23 @@ func TestMaxIdleTime(t *testing.T) {
                        db.SetMaxOpenConns(usedConns)
                        db.SetMaxIdleConns(usedConns)
                        db.SetConnMaxIdleTime(item.wantMaxIdleTime)
-                       db.SetConnMaxLifetime(0)
+                       db.SetConnMaxLifetime(item.wantMaxLifetime)
 
                        preMaxIdleClosed := db.Stats().MaxIdleTimeClosed
 
                        // Busy usedConns.
-                       tm := testUseConns(t, usedConns, baseTime, db)
+                       testUseConns(t, usedConns, baseTime, db)
 
-                       tm = baseTime.Add(item.timeOffset)
+                       tm := baseTime.Add(item.timeOffset)
 
                        // Reuse connections which should never be considered idle
                        // and exercises the sorting for issue 39471.
-                       testUseConns(t, reusedConns, tm, db)
+                       tm = testUseConns(t, reusedConns, tm, db)
+
+                       tm = tm.Add(item.secondTimeOffset)
+                       nowFunc = func() time.Time {
+                               return tm
+                       }
 
                        db.mu.Lock()
                        nc, closing := db.connectionCleanerRunLocked(time.Second)
@@ -4001,7 +4038,7 @@ func TestMaxIdleTime(t *testing.T) {
 
                        st := db.Stats()
                        maxIdleClosed := st.MaxIdleTimeClosed - preMaxIdleClosed
-                       if g, w := maxIdleClosed, item.wantIdleClosed; g != w {
+                       if g, w := maxIdleClosed, item.wantMaxIdleClosed; g != w {
                                t.Errorf("got: %d; want %d max idle closed conns", g, w)
                        }
                })