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 issue 6593

R=golang-dev, bradfitz, tad.glines, bketelsen
CC=golang-dev
https://codereview.appspot.com/14611045

Committer: Brad Fitzpatrick <bradfitz@golang.org>

3 files changed, 63 insertions, 3 deletions

diff --git a/src/pkg/database/sql/fakedb_test.go b/src/pkg/database/sql/fakedb_test.go
index 39c028278..2ed136475 100644
--- 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 fe46ff378..3047735ac 100644
--- 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 32605ce76..093c0d64c 100644
--- 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)