database/sql: Fix connection leak and potential deadlock
CL 10726044 introduced a race condition which causes connections
to be leaked under certain circumstances. If SetMaxOpenConns is
used, the application eventually deadlocks. Otherwise, the number
of open connections just keep growing indefinitely.
Fixes #6593
R=golang-dev, bradfitz, tad.glines, bketelsen
CC=golang-dev
https://golang.org/cl/14611045
diff --git a/src/pkg/database/sql/fakedb_test.go b/src/pkg/database/sql/fakedb_test.go
index 39c0282..2ed1364 100644
--- a/src/pkg/database/sql/fakedb_test.go
+++ b/src/pkg/database/sql/fakedb_test.go
@@ -38,6 +38,8 @@
mu sync.Mutex // guards 3 following fields
openCount int // conn opens
closeCount int // conn closes
+ waitCh chan struct{}
+ waitingCh chan struct{}
dbs map[string]*fakeDB
}
@@ -146,6 +148,10 @@
if len(parts) >= 2 && parts[1] == "badConn" {
conn.bad = true
}
+ if d.waitCh != nil {
+ d.waitingCh <- struct{}{}
+ <-d.waitCh
+ }
return conn, nil
}
diff --git a/src/pkg/database/sql/sql.go b/src/pkg/database/sql/sql.go
index fe46ff3..3047735 100644
--- a/src/pkg/database/sql/sql.go
+++ b/src/pkg/database/sql/sql.go
@@ -593,9 +593,12 @@
db: db,
ci: ci,
}
- db.addDepLocked(dc, dc)
- db.numOpen++
- db.putConnDBLocked(dc, err)
+ if db.putConnDBLocked(dc, err) {
+ db.addDepLocked(dc, dc)
+ db.numOpen++
+ } else {
+ ci.Close()
+ }
}
// connRequest represents one request for a new connection
diff --git a/src/pkg/database/sql/sql_test.go b/src/pkg/database/sql/sql_test.go
index 32605ce..093c0d6 100644
--- a/src/pkg/database/sql/sql_test.go
+++ b/src/pkg/database/sql/sql_test.go
@@ -1677,6 +1677,57 @@
doConcurrentTest(t, new(concurrentRandomTest))
}
+func TestConnectionLeak(t *testing.T) {
+ db := newTestDB(t, "people")
+ defer closeDB(t, db)
+ // Start by opening defaultMaxIdleConns
+ rows := make([]*Rows, defaultMaxIdleConns)
+ // We need to SetMaxOpenConns > MaxIdleConns, so the DB can open
+ // a new connection and we can fill the idle queue with the released
+ // connections.
+ db.SetMaxOpenConns(len(rows) + 1)
+ for ii := range rows {
+ r, err := db.Query("SELECT|people|name|")
+ if err != nil {
+ t.Fatal(err)
+ }
+ r.Next()
+ if err := r.Err(); err != nil {
+ t.Fatal(err)
+ }
+ rows[ii] = r
+ }
+ // Now we have defaultMaxIdleConns busy connections. Open
+ // a new one, but wait until the busy connections are released
+ // before returning control to DB.
+ drv := db.driver.(*fakeDriver)
+ drv.waitCh = make(chan struct{}, 1)
+ drv.waitingCh = make(chan struct{}, 1)
+ var wg sync.WaitGroup
+ wg.Add(1)
+ go func() {
+ r, err := db.Query("SELECT|people|name|")
+ if err != nil {
+ t.Fatal(err)
+ }
+ r.Close()
+ wg.Done()
+ }()
+ // Wait until the goroutine we've just created has started waiting.
+ <-drv.waitingCh
+ // Now close the busy connections. This provides a connection for
+ // the blocked goroutine and then fills up the idle queue.
+ for _, v := range rows {
+ v.Close()
+ }
+ // At this point we give the new connection to DB. This connection is
+ // now useless, since the idle queue is full and there are no pending
+ // requests. DB should deal with this situation without leaking the
+ // connection.
+ drv.waitCh <- struct{}{}
+ wg.Wait()
+}
+
func BenchmarkConcurrentDBExec(b *testing.B) {
b.ReportAllocs()
ct := new(concurrentDBExecTest)
