From 48f1cde942959e2fc3c56973a2986c24d554c82c Mon Sep 17 00:00:00 2001
From: Pavel <kositsyn.pa@phystech.edu>
Date: Mon, 8 Nov 2021 14:29:16 +0000
Subject: [PATCH] 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>
---
 src/database/sql/sql.go      |  2 +-
 src/database/sql/sql_test.go | 61 +++++++++++++++++++++++++++++-------
 2 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go
index 5131c08b51..c5b4f50aa7 100644
--- 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 889adc3164..b887b40d71 100644
--- 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)
 			}
 		})
-- 
2.50.0