diff options
author | Alberto Garc?a Hierro <alberto@garciahierro.com> | 2013-10-16 09:22:57 -0700 |
---|---|---|
committer | Alberto Garc?a Hierro <alberto@garciahierro.com> | 2013-10-16 09:22:57 -0700 |
commit | 8d8eb072d56e14c893d8b0bf328d0858842d6b27 (patch) | |
tree | 6e2af58af5450edec571dda1d83937307d9f7386 /src | |
parent | cbc904224a66e02b50185ec37f03b10f6d5e12f8 (diff) | |
download | go-8d8eb072d56e14c893d8b0bf328d0858842d6b27.tar.gz |
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>
Diffstat (limited to 'src')
-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 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) |