diff options
author | Alberto GarcĂa Hierro <alberto@garciahierro.com> | 2013-10-16 09:22:57 -0700 |
---|---|---|
committer | Brad Fitzpatrick <bradfitz@golang.org> | 2013-10-16 09:22:57 -0700 |
commit | 37db8804691f0a5e618cbc041909895c9709263c (patch) | |
tree | b8e0c739848eec66cdc69f4636f15751370c191b | |
parent | 478f4b67543824c039d2f7afec6af88a59148db2 (diff) | |
download | go-37db8804691f0a5e618cbc041909895c9709263c.tar.gz go-37db8804691f0a5e618cbc041909895c9709263c.zip |
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
-rw-r--r-- | src/pkg/database/sql/fakedb_test.go | 6 | ||||
-rw-r--r-- | src/pkg/database/sql/sql.go | 9 | ||||
-rw-r--r-- | src/pkg/database/sql/sql_test.go | 51 |
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 39c0282789..2ed1364759 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 fe46ff3781..3047735acc 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 32605ce761..093c0d64ca 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) |