code review 14642044: database/sql: Fix connection leaks and numOpen count

src/pkg/database/sql/sql.go

 // Copyright 2011 The Go Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style
 // license that can be found in the LICENSE file.
 
 // Package sql provides a generic interface around SQL (or SQL-like)
 // databases.
 //
 // The sql package must be used in conjunction with a database driver.
 // See http://golang.org/s/sqldrivers for a list of drivers.
 package sql
@@ skipping 486 matching lines @@
 
 // SetMaxIdleConns sets the maximum number of connections in the idle
 // connection pool.
 //
 // If MaxOpenConns is greater than 0 but less than the new MaxIdleConns
 // then the new MaxIdleConns will be reduced to match the MaxOpenConns limit
 //
 // 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 {
                 // No idle connections.
                 db.maxIdle = -1
         }
         // Make sure maxIdle doesn't exceed maxOpen
         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()
         }
 }
 
 // SetMaxOpenConns sets the maximum number of open connections to the database.
 //
 // If MaxIdleConns is greater than 0 and the new MaxOpenConns is less than
 // MaxIdleConns, then MaxIdleConns will be reduced to match the new
 // MaxOpenConns limit
 //
 // If n <= 0, then there is no limit on the number of open connections.
@@ skipping 203 matching lines @@
         dc.inUse = false
 
         for _, fn := range dc.onPut {
                 fn()
         }
         dc.onPut = nil
 
         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()
                 return
         }
         if putConnHook != nil {
                 putConnHook(db, dc)
         }
         added := db.putConnDBLocked(dc, nil)
         db.mu.Unlock()
@@ skipping 842 matching lines @@
         // the Rows in our defer, when we return from this function.
         // the contract with the driver.Next(...) interface is that it
         // can return slices into read-only temporary memory that's
         // only valid until the next Scan/Close.  But the TODO is that
         // for a lot of drivers, this copy will be unnecessary.  We
         // should provide an optional interface for drivers to
         // implement to say, "don't worry, the []bytes that I return
         // 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
         }
         err := r.rows.Scan(dest...)
         if err != nil {
                 return err
         }
 
         return nil
 }
@@ skipping 25 matching lines @@
         var buf [2 << 10]byte
         return string(buf[:runtime.Stack(buf[:], false)])
 }
 
 // withLock runs while holding lk.
 func withLock(lk sync.Locker, fn func()) {
         lk.Lock()
         fn()
         lk.Unlock()
 }