From 74f99d0933d5c201fc17d90ab612cd1a9c7d425f Mon Sep 17 00:00:00 2001 From: Steven Hartland Date: Tue, 9 Jun 2020 08:58:08 +0100 Subject: database/sql: Fix idle connection reuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix idle connection reuse so that ConnMaxIdleTime clears down excessive idle connections. This now ensures that db.freeConn is ordered by returnedAt and that connections that have been idle for the shortest period are reused first. In addition connectionCleanerRunLocked updates the next check deadline based on idle and maximum life time information so that we avoid waiting up to double MaxIdleTime to close connections. Corrected the calling timer of connectionCleaner. Fixes #39471 Change-Id: I6d26b3542179ef35aa13e5265a89bc0f08ba7fa1 Reviewed-on: https://go-review.googlesource.com/c/go/+/237337 Reviewed-by: Tamás Gulácsi Reviewed-by: Daniel Theophanes Trust: Ian Lance Taylor --- src/database/sql/sql_test.go | 87 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 72 insertions(+), 15 deletions(-) (limited to 'src/database/sql/sql_test.go') diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go index f771dee4a9..15c30e0d00 100644 --- a/src/database/sql/sql_test.go +++ b/src/database/sql/sql_test.go @@ -2399,10 +2399,14 @@ func TestConnMaxLifetime(t *testing.T) { tx.Commit() tx2.Commit() - driver.mu.Lock() - opens = driver.openCount - opens0 - closes = driver.closeCount - closes0 - driver.mu.Unlock() + // Give connectionCleaner chance to run. + for i := 0; i < 100 && closes != 1; i++ { + time.Sleep(time.Millisecond) + driver.mu.Lock() + opens = driver.openCount - opens0 + closes = driver.closeCount - closes0 + driver.mu.Unlock() + } if opens != 3 { t.Errorf("opens = %d; want 3", opens) @@ -2410,6 +2414,10 @@ func TestConnMaxLifetime(t *testing.T) { if closes != 1 { t.Errorf("closes = %d; want 1", closes) } + + if s := db.Stats(); s.MaxLifetimeClosed != 1 { + t.Errorf("MaxLifetimeClosed = %d; want 1 %#v", s.MaxLifetimeClosed, s) + } } // golang.org/issue/5323 @@ -3896,14 +3904,48 @@ func TestStatsMaxIdleClosedTen(t *testing.T) { } } +// testUseConns uses count concurrent connections with 1 nanosecond apart. +// Returns the returnedAt time of the final connection. +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 { + c, err := db.Conn(ctx) + if err != nil { + t.Error(err) + } + conns[i] = c + } + + for _, c := range conns { + tm = tm.Add(time.Nanosecond) + nowFunc = func() time.Time { + return tm + } + if err := c.Close(); err != nil { + t.Error(err) + } + } + + return tm +} + func TestMaxIdleTime(t *testing.T) { + usedConns := 5 + reusedConns := 2 list := []struct { wantMaxIdleTime time.Duration + wantNextCheck time.Duration wantIdleClosed int64 timeOffset time.Duration }{ - {time.Nanosecond, 1, 10 * time.Millisecond}, - {time.Hour, 0, 10 * time.Millisecond}, + { + time.Millisecond, + time.Millisecond - time.Nanosecond, + int64(usedConns - reusedConns), + 10 * time.Millisecond, + }, + {time.Hour, time.Second, 0, 10 * time.Millisecond}, } baseTime := time.Unix(0, 0) defer func() { @@ -3917,23 +3959,38 @@ func TestMaxIdleTime(t *testing.T) { db := newTestDB(t, "people") defer closeDB(t, db) - db.SetMaxOpenConns(1) - db.SetMaxIdleConns(1) + db.SetMaxOpenConns(usedConns) + db.SetMaxIdleConns(usedConns) db.SetConnMaxIdleTime(item.wantMaxIdleTime) db.SetConnMaxLifetime(0) preMaxIdleClosed := db.Stats().MaxIdleTimeClosed - if err := db.Ping(); err != nil { - t.Fatal(err) + // Busy usedConns. + tm := testUseConns(t, usedConns, baseTime, db) + + 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) + + db.mu.Lock() + nc, closing := db.connectionCleanerRunLocked(time.Second) + if nc != item.wantNextCheck { + t.Errorf("got %v; want %v next check duration", nc, item.wantNextCheck) } - nowFunc = func() time.Time { - return baseTime.Add(item.timeOffset) + // Validate freeConn order. + var last time.Time + for _, c := range db.freeConn { + if last.After(c.returnedAt) { + t.Error("freeConn is not ordered by returnedAt") + break + } + last = c.returnedAt } - db.mu.Lock() - closing := db.connectionCleanerRunLocked() db.mu.Unlock() for _, c := range closing { c.Close() @@ -3945,7 +4002,7 @@ func TestMaxIdleTime(t *testing.T) { st := db.Stats() maxIdleClosed := st.MaxIdleTimeClosed - preMaxIdleClosed if g, w := maxIdleClosed, item.wantIdleClosed; g != w { - t.Errorf(" got: %d; want %d max idle closed conns", g, w) + t.Errorf("got: %d; want %d max idle closed conns", g, w) } }) } -- cgit v1.3