database/sql: close driver Stmt before releasing Conn

From the issue, which describes it as well as I could:

database/sql assumes that driver.Stmt.Close does not need the
connection.

see database/sql/sql.go:1308:

This puts the Rows' connection back into the idle pool, and
then calls the driver.Stmt.Close method of the Stmt it belongs
to.  In the postgresql driver implementation
(https://github.com/lib/pq), Stmt.Close communicates with the
server (on the connection that was just put back into the idle
pool).  Most of the time, this causes no problems, but if
another goroutine makes a query at the right (wrong?) time,
chaos results.

In any case, traffic is being sent on "free" connections
shortly after they are freed, leading to race conditions that
kill the driver code.

Fixes #5283

R=golang-dev, r
CC=golang-dev
https://golang.org/cl/8633044

diff --git a/src/pkg/database/sql/fakedb_test.go b/src/pkg/database/sql/fakedb_test.go
index 24c255f6e08edac49d6915ca80df07b8a093a130..07e7fd242a200f850b7b7cb26b2d5d349487b65f 100644 (file)
--- a/src/pkg/database/sql/fakedb_test.go
+++ b/src/pkg/database/sql/fakedb_test.go
@@ -13,6 +13,7 @@ import (
        "strconv"
        "strings"
        "sync"
+       "testing"
        "time"
 )
 
@@ -240,8 +241,19 @@ func setHookpostCloseConn(fn func(*fakeConn, error)) {
        hookPostCloseConn.fn = fn
 }
 
+var testStrictClose *testing.T
+
+// setStrictFakeConnClose sets the t to Errorf on when fakeConn.Close
+// fails to close. If nil, the check is disabled.
+func setStrictFakeConnClose(t *testing.T) {
+       testStrictClose = t
+}
+
 func (c *fakeConn) Close() (err error) {
        defer func() {
+               if err != nil && testStrictClose != nil {
+                       testStrictClose.Errorf("failed to close a test fakeConn: %v", err)
+               }
                hookPostCloseConn.Lock()
                fn := hookPostCloseConn.fn
                hookPostCloseConn.Unlock()
@@ -443,6 +455,12 @@ func (s *fakeStmt) ColumnConverter(idx int) driver.ValueConverter {
 }
 
 func (s *fakeStmt) Close() error {
+       if s.c == nil {
+               panic("nil conn in fakeStmt.Close")
+       }
+       if s.c.db == nil {
+               panic("in fakeSmt.Close, conn's db is nil (already closed)")
+       }
        if !s.closed {
                s.c.incrStat(&s.c.stmtsClosed)
                s.closed = true

diff --git a/src/pkg/database/sql/sql.go b/src/pkg/database/sql/sql.go
index bd450c7ec9c9869973edcbbcd56e02f057e37d2b..72289407c9367f1bef48bab74c4cb528bea296b5 100644 (file)
--- a/src/pkg/database/sql/sql.go
+++ b/src/pkg/database/sql/sql.go
@@ -1311,10 +1311,10 @@ func (rs *Rows) Close() error {
        }
        rs.closed = true
        err := rs.rowsi.Close()
-       rs.releaseConn(err)
        if rs.closeStmt != nil {
                rs.closeStmt.Close()
        }
+       rs.releaseConn(err)
        return err
 }
 

diff --git a/src/pkg/database/sql/sql_test.go b/src/pkg/database/sql/sql_test.go
index 6b91783784e1d690103dde13b3c019e2c612e522..37fdd2795ecec84aaec49a7ac4b1cfcf20c61506 100644 (file)
--- a/src/pkg/database/sql/sql_test.go
+++ b/src/pkg/database/sql/sql_test.go
@@ -854,6 +854,26 @@ func TestCloseConnBeforeStmts(t *testing.T) {
        }
 }
 
+// golang.org/issue/5283: don't release the Rows' connection in Close
+// before calling Stmt.Close.
+func TestRowsCloseOrder(t *testing.T) {
+       db := newTestDB(t, "people")
+       defer closeDB(t, db)
+
+       db.SetMaxIdleConns(0)
+       setStrictFakeConnClose(t)
+       defer setStrictFakeConnClose(nil)
+
+       rows, err := db.Query("SELECT|people|age,name|")
+       if err != nil {
+               t.Fatal(err)
+       }
+       err = rows.Close()
+       if err != nil {
+               t.Fatal(err)
+       }
+}
+
 func manyConcurrentQueries(t testOrBench) {
        maxProcs, numReqs := 16, 500
        if testing.Short() {