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 39c02827897793dce0ade911647b40b8d4aee72e..2ed1364759b9e1406a866782970d48dbd1702915 100644 (file)
--- a/src/pkg/database/sql/fakedb_test.go
+++ b/src/pkg/database/sql/fakedb_test.go
@@ -38,6 +38,8 @@ type fakeDriver struct {
        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 @@ func (d *fakeDriver) Open(dsn string) (driver.Conn, error) {
        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 fe46ff3781fce407f8d04922428997ccd6a7c424..3047735acc414a60a3be092739b24c637f3a5982 100644 (file)
--- a/src/pkg/database/sql/sql.go
+++ b/src/pkg/database/sql/sql.go
@@ -593,9 +593,12 @@ func (db *DB) openNewConnection() {
                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 32605ce761391f44d38ca438a5923d0219a69056..093c0d64caceecc170153abf96d500ccf7372097 100644 (file)
--- a/src/pkg/database/sql/sql_test.go
+++ b/src/pkg/database/sql/sql_test.go
@@ -1677,6 +1677,57 @@ func TestConcurrency(t *testing.T) {
        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)