Skip to content

Commit cd56c6f

Browse files
author
KJ Tsanaktsidis
committed
Fix checking cancelled connections back into the connection pool
If * BeginTx is called with a non-default isolation level, * The context is canceled before SET TRANSACTION ISOLATION LEVEL completes, then the connection: * has the cancelled property set to "context cancelled", * has the closed property set to true, and, * BeginTx returns "context canceled" Because of this, the connection gets put back into the connection pool. When it is checked out again, if BeginTx is called on it again _without_ an isolation level, * then we fall into the mc.closed.IsSet() check in begin(), * so we return ErrBadConn, * so the driver kicks the broken connection out of the pool * (and transparently retries to get a new connection that isn't broken too). However, if BeginTx is called on the connection _with_ an isolation level, then we return a context canceled error from the SET TRANSACTION ISOLATION LEVEL call. That means the broken connection will stick around in the pool forever (or until it's checked out for some other operation that correctly returns ErrBadConn). The fix is to check for the connection being closed before executing SET TRANSACTION ISOLATION LEVEL.
1 parent 343c803 commit cd56c6f

File tree

2 files changed

+21
-2
lines changed

2 files changed

+21
-2
lines changed

connection.go

+4
Original file line numberDiff line numberDiff line change
@@ -489,6 +489,10 @@ func (mc *mysqlConn) Ping(ctx context.Context) (err error) {
489489

490490
// BeginTx implements driver.ConnBeginTx interface
491491
func (mc *mysqlConn) BeginTx(ctx context.Context, opts driver.TxOptions) (driver.Tx, error) {
492+
if mc.closed.IsSet() {
493+
return nil, driver.ErrBadConn
494+
}
495+
492496
if err := mc.watchCancel(ctx); err != nil {
493497
return nil, err
494498
}

driver_test.go

+17-2
Original file line numberDiff line numberDiff line change
@@ -2608,7 +2608,12 @@ func TestContextCancelBegin(t *testing.T) {
26082608
runTests(t, dsn, func(dbt *DBTest) {
26092609
dbt.mustExec("CREATE TABLE test (v INTEGER)")
26102610
ctx, cancel := context.WithCancel(context.Background())
2611-
tx, err := dbt.db.BeginTx(ctx, nil)
2611+
conn, err := dbt.db.Conn(ctx)
2612+
if err != nil {
2613+
dbt.Fatal(err)
2614+
}
2615+
defer conn.Close()
2616+
tx, err := conn.BeginTx(ctx, nil)
26122617
if err != nil {
26132618
dbt.Fatal(err)
26142619
}
@@ -2638,7 +2643,17 @@ func TestContextCancelBegin(t *testing.T) {
26382643
dbt.Errorf("expected sql.ErrTxDone or context.Canceled, got %v", err)
26392644
}
26402645

2641-
// Context is canceled, so cannot begin a transaction.
2646+
// The connection is now in an inoperable state - so performing other
2647+
// operations should fail with ErrBadConn
2648+
// Important to exercise isolation level too - it runs SET TRANSACTION ISOLATION
2649+
// LEVEL XXX first, which needs to return ErrBadConn if the connection's context
2650+
// is cancelled
2651+
_, err = conn.BeginTx(context.Background(), &sql.TxOptions{Isolation: sql.LevelReadCommitted})
2652+
if err != driver.ErrBadConn {
2653+
dbt.Errorf("expected driver.ErrBadConn, got %v", err)
2654+
}
2655+
2656+
// cannot begin a transaction (on a different conn) with a canceled context
26422657
if _, err := dbt.db.BeginTx(ctx, nil); err != context.Canceled {
26432658
dbt.Errorf("expected context.Canceled, got %v", err)
26442659
}

0 commit comments

Comments
 (0)