database/sql: fix double decrement of numOpen count; test for connection leaks

Add a check at the end of every test to make sure
there are no leaked connections after running a test.

Avoid incorrectly decrementing the number of open connections
when the driver connection ends up it a bad state (numOpen was
decremented twice).

Prevent leaking a Rows struct (which ends up leaking a
connection) in Row.Scan() when a *RawBytes destination is
improperly used.

Close the Rows struct in TestRowsColumns.

Update #6593

R=golang-dev, bradfitz, dave
CC=golang-dev
https://golang.org/cl/14642044

diff --git a/src/pkg/database/sql/sql.go b/src/pkg/database/sql/sql.go
index ae313caf1179d147e2b5f35f6e016765b6e375cc..fe46ff3781fce407f8d04922428997ccd6a7c424 100644 (file)
--- a/src/pkg/database/sql/sql.go
+++ b/src/pkg/database/sql/sql.go
@@ -504,7 +504,6 @@ func (db *DB) maxIdleConnsLocked() int {
 // If n <= 0, no idle connections are retained.
 func (db *DB) SetMaxIdleConns(n int) {
        db.mu.Lock()
-       defer db.mu.Unlock()
        if n > 0 {
                db.maxIdle = n
        } else {
@@ -515,11 +514,16 @@ func (db *DB) SetMaxIdleConns(n int) {
        if db.maxOpen > 0 && db.maxIdleConnsLocked() > db.maxOpen {
                db.maxIdle = db.maxOpen
        }
+       var closing []*driverConn
        for db.freeConn.Len() > db.maxIdleConnsLocked() {
                dc := db.freeConn.Back().Value.(*driverConn)
                dc.listElem = nil
                db.freeConn.Remove(db.freeConn.Back())
-               go dc.Close()
+               closing = append(closing, dc)
+       }
+       db.mu.Unlock()
+       for _, c := range closing {
+               c.Close()
        }
 }
 
@@ -743,8 +747,8 @@ func (db *DB) putConn(dc *driverConn, err error) {
        if err == driver.ErrBadConn {
                // Don't reuse bad connections.
                // Since the conn is considered bad and is being discarded, treat it
-               // as closed. Decrement the open count.
-               db.numOpen--
+               // as closed. Don't decrement the open count here, finalClose will
+               // take care of that.
                db.maybeOpenNewConnections()
                db.mu.Unlock()
                dc.Close()
@@ -1607,13 +1611,13 @@ func (r *Row) Scan(dest ...interface{}) error {
        // from Next will not be modified again." (for instance, if
        // they were obtained from the network anyway) But for now we
        // don't care.
+       defer r.rows.Close()
        for _, dp := range dest {
                if _, ok := dp.(*RawBytes); ok {
                        return errors.New("sql: RawBytes isn't allowed on Row.Scan")
                }
        }
 
-       defer r.rows.Close()
        if !r.rows.Next() {
                return ErrNoRows
        }

diff --git a/src/pkg/database/sql/sql_test.go b/src/pkg/database/sql/sql_test.go
index 435d79c24a91fe1c039710a6062ee7d00346b2fa..32605ce761391f44d38ca438a5923d0219a69056 100644 (file)
--- a/src/pkg/database/sql/sql_test.go
+++ b/src/pkg/database/sql/sql_test.go
@@ -94,6 +94,12 @@ func closeDB(t testing.TB, db *DB) {
        if err != nil {
                t.Fatalf("error closing DB: %v", err)
        }
+       db.mu.Lock()
+       count := db.numOpen
+       db.mu.Unlock()
+       if count != 0 {
+               t.Fatalf("%d connections still open after closing DB", db.numOpen)
+       }
 }
 
 // numPrepares assumes that db has exactly 1 idle conn and returns
@@ -246,6 +252,9 @@ func TestRowsColumns(t *testing.T) {
        if !reflect.DeepEqual(cols, want) {
                t.Errorf("got %#v; want %#v", cols, want)
        }
+       if err := rows.Close(); err != nil {
+               t.Errorf("error closing rows: %s", err)
+       }
 }
 
 func TestQueryRow(t *testing.T) {