database/sql: fix double connection free on Stmt.Query error
In a transaction, on a Stmt.Query error, it was possible for a
connection to be added to a db's freelist twice. Should use
the local releaseConn function instead.
Thanks to Gwenael Treguier for the failing test.
Also in this CL: propagate driver errors through releaseConn
into *DB.putConn, which conditionally ignores the freelist
addition if the driver signaled ErrBadConn, introduced in a
previous CL.
R=golang-dev, gary.burd
CC=golang-dev
https://golang.org/cl/5798049
diff --git a/src/pkg/database/sql/sql.go b/src/pkg/database/sql/sql.go
index afee275..c00425d 100644
--- a/src/pkg/database/sql/sql.go
+++ b/src/pkg/database/sql/sql.go
@@ -262,6 +262,9 @@
return nil, false
}
+// putConnHook is a hook for testing.
+var putConnHook func(*DB, driver.Conn)
+
// putConn adds a connection to the db's free pool.
// err is optionally the last error that occured on this connection.
func (db *DB) putConn(c driver.Conn, err error) {
@@ -270,6 +273,9 @@
return
}
db.mu.Lock()
+ if putConnHook != nil {
+ putConnHook(db, c)
+ }
if n := len(db.freeConn); !db.closed && n < db.maxIdleConns() {
db.freeConn = append(db.freeConn, c)
db.mu.Unlock()
@@ -654,7 +660,7 @@
if err != nil {
return nil, err
}
- defer releaseConn()
+ defer releaseConn(nil)
// -1 means the driver doesn't know how to count the number of
// placeholders, so we won't sanity check input here and instead let the
@@ -717,7 +723,7 @@
// connStmt returns a free driver connection on which to execute the
// statement, a function to call to release the connection, and a
// statement bound to that connection.
-func (s *Stmt) connStmt() (ci driver.Conn, releaseConn func(), si driver.Stmt, err error) {
+func (s *Stmt) connStmt() (ci driver.Conn, releaseConn func(error), si driver.Stmt, err error) {
if err = s.stickyErr; err != nil {
return
}
@@ -736,7 +742,7 @@
if err != nil {
return
}
- releaseConn = func() { s.tx.releaseConn() }
+ releaseConn = func(error) { s.tx.releaseConn() }
return ci, releaseConn, s.txsi, nil
}
@@ -776,7 +782,7 @@
}
conn := cs.ci
- releaseConn = func() { s.db.putConn(conn, nil) }
+ releaseConn = func(err error) { s.db.putConn(conn, err) }
return conn, releaseConn, cs.si, nil
}
@@ -800,7 +806,7 @@
}
rowsi, err := si.Query(sargs)
if err != nil {
- s.db.putConn(ci, err)
+ releaseConn(err)
return nil, err
}
// Note: ownership of ci passes to the *Rows, to be freed
@@ -878,7 +884,7 @@
type Rows struct {
db *DB
ci driver.Conn // owned; must call putconn when closed to release
- releaseConn func()
+ releaseConn func(error)
rowsi driver.Rows
closed bool
@@ -990,7 +996,7 @@
}
rs.closed = true
err := rs.rowsi.Close()
- rs.releaseConn()
+ rs.releaseConn(err)
if rs.closeStmt != nil {
rs.closeStmt.Close()
}
