database/sql: Tx.Commit and Tx.Rollback do not remove bad connections from pool

[ChrisHines]

go version go1.4.2 windows/amd64

This issue was originally discovered while using github.com/alexbrainman/odbc, but it does not seem to be a problem specific to that driver. It is minimally reproducible with the below test program.

The program runs a query inside a transaction five times. The commit (or rollback) of the second attempt returns driver.ErrBadConn.

I expect the bad connection to get removed from the pool and subsequent transactions to occur on a fresh connection.

Instead database/sql keeps the bad connection in the pool and subsequent transactions reuse the bad connection and also fail.

package main

import (
    "database/sql"
    "database/sql/driver"
    "log"
    "reflect"
    "runtime"
)

var opened, closed int

type MockDriver struct{}

func (d *MockDriver) Open(name string) (driver.Conn, error) {
    opened++
    return &MockConn{}, NextError()
}

type MockConn struct{ err error }

// NOTE: that this always succeeds, even if c.err != nil this simulates a
// driver that doesn't need to communicate the beginning of a transaction and
// therefore may not be able to discover a bad connection at this point.
func (c *MockConn) Begin() (driver.Tx, error) { return &MockTx{c}, nil }

func (c *MockConn) Close() error                              { closed++; return c.Err() }
func (c *MockConn) Prepare(query string) (driver.Stmt, error) { return &MockStmt{c}, c.Err() }
func (c *MockConn) Err() error {
    if c.err == nil {
        c.err = NextError()
    }
    return c.err
}

type MockTx struct{ c *MockConn }

func (tx *MockTx) Commit() error   { return tx.c.Err() }
func (tx *MockTx) Rollback() error { return tx.c.Err() }

type MockStmt struct{ c *MockConn }

func (s *MockStmt) Close() error  { return s.c.Err() }
func (s *MockStmt) NumInput() int { return -1 }
func (s *MockStmt) Exec(args []driver.Value) (driver.Result, error) {
    return driver.ResultNoRows, s.c.Err()
}
func (s *MockStmt) Query(args []driver.Value) (driver.Rows, error) { return &MockRows{s}, s.c.Err() }

type MockRows struct{ s *MockStmt }

func (r *MockRows) Columns() []string              { return []string{} }
func (r *MockRows) Close() error                   { return r.s.c.Err() }
func (r *MockRows) Next(dest []driver.Value) error { return r.s.c.Err() }

var nextError error

func NextError() (err error) {
    err, nextError = nextError, nil
    return
}

func main() {
    drv := &MockDriver{}
    sql.Register("mock", drv)
    db, err := sql.Open("mock", "")
    checkErr(0, err, nil)
    for i := 0; i < 5; i++ {
        // this always succeeds because that's the way some drivers may work
        tx, err := db.Begin()
        checkErr(i, err, nil)
        if err != nil {
            continue
        }
        rows, err := tx.Query("query")
        checkErr(i, err, nil)
        if err != nil {
            err = tx.Rollback()
            checkErr(i, err, nil)
            continue
        }
        rows.Close()
        var want error
        if i == 1 {
            nextError, want = driver.ErrBadConn, driver.ErrBadConn
        }
        // fails when i == 1, but the connection is not removed from the pool
        err = tx.Commit()
        checkErr(i, err, want)
        log.Printf("i(%d), opened: %d, closed: %d", i, opened, closed)
    }
    log.Printf("final, opened: %d, closed: %d", opened, closed)
}

func checkErr(i int, got, want error) {
    if !reflect.DeepEqual(got, want) {
        _, _, line, _ := runtime.Caller(1)
        log.Printf("i(%d), line %d: got %v, want %v", i, line, got, want)
    }
}

[ianlancetaylor]

CC @bradfitz

[ChrisHines]

I have written a test and a fix for this in https://golang.org/cl/13912.

Please review. This is my first CL for Go, so let me know if I have made any missteps.

[gopherbot]

CL https://golang.org/cl/13912 mentions this issue.

[pkieltyka]

wow... can we get this in 1.5.1 fixed please..?

[johto]

wow... can we get this in 1.5.1 fixed please..?

It's been like this forever. I'm not sure what the rush is.

[pkieltyka]

@johto sorry, but I don't see how that is a good reason to leave such a basic, and destructive bug because it's been "like this forever". 1.5.2, sure, I can understand that and be patient, but having to wait 6 months for this class of bug to be fixed saddens and worries me. I hope we can strive for a higher standard.

[johto]

@johto sorry, but I don't see how that is a good reason to leave such a basic, and destructive bug because it's been "like this forever". 1.5.2, sure, I can understand that and be patient, but having to wait 6 months for this class of bug to be fixed saddens and worries me. I hope we can strive for a higher standard.

My point is that since it's been like this forever, I expect most drivers to have worked around the issue already. For new drivers it feels unreasonable to expect everyone to be on 1.5.1 or later, so even the authors of such projects will need to work around the issue.

What this bug needs more than anything in my view is publicity, not rushed fixes.

[pkieltyka]

I contribute to both sqlx and upper/db, and this is the first time I've heard of this bug.

[ChrisHines]

I suspect that the most popular sql drivers have worked around this issue or are blissfully unaffected because Tx.Begin notices the bad connection.

github.com/lib/pq indirectly mentions the issue in a comment in it's driver.Conn.Commit implementation.

github.com/go-sql-driver/mysql actively initiates a transaction and will likely catch the bad connection at that point.

github.com/mattn/go-sqlite3 doesn't appear to ever return driver.ErrBadConn, which probably makes sense for local DBs.

Meanwhile, github.com/alexbrainman/odbc just added the ability to handle bad connections recently. Prior to that commit a bad connection required the application to close its *sql.DB to make sure no bad connections were in the pool. Now it works correctly sometimes, but not when using transactions.

The work around at the driver level seems to be that the driver must store additional state in the connection to allow returning driver.ErrBadConn from the next call to driver.Conn.Prepare, driver.Conn.Begin, or any of the optional interface methods it may implement.

The work around at the application level is much worse. As mentioned above, the application must call sql.DB.Close and get a new connection pool with sql.Open, which of course goes against the design intentions of database/sql.

Since this bug has been around for a long time it does not seem to fit the criteria that Russ recently laid out for inclusion in a point release.

Perhaps the issue has been around for so long that drivers are essentially required to handle it on their own. In that case the fix for this issue would be documenting the required behavior in the database/sql/driver package.

Another possibility is that returning driver.ErrBadConn from Tx.Commit or Tx.Rollback is a broken idea all together. The documentation for ErrBadConn states:

To prevent duplicate operations, ErrBadConn should NOT be returned if there's a possibility that the database server might have performed the operation. Even if the server sends back an error, you shouldn't return ErrBadConn.

But of course database/sql doesn't retry commits or rollbacks anyway, so it's not clear if these instructions apply in this case.