From de477138d1b11982e86d8d0898e2a471771899b6 Mon Sep 17 00:00:00 2001 From: Ayzat Sadykov Date: Sat, 24 Oct 2020 22:11:51 +0000 Subject: database/sql: fix comment on DB.stop() Previously, 2 goroutines were created in OpenDB and a comment in the DB.close() field indicated that they were canceled. Later, session Resetter () was removed, but the comment remained the same. This commit just fixes this message Change-Id: Ie81026f51d7770e9cf8004818154021f626fb2e8 GitHub-Last-Rev: 38b338a0d1cd713d71fa547aa842d395e6d75484 GitHub-Pull-Request: golang/go#42191 Reviewed-on: https://go-review.googlesource.com/c/go/+/264838 Reviewed-by: Daniel Theophanes Reviewed-by: Brad Fitzpatrick Trust: Brad Fitzpatrick --- src/database/sql/sql.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/database') diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go index e3580698fd..fc7e3e4485 100644 --- a/src/database/sql/sql.go +++ b/src/database/sql/sql.go @@ -434,7 +434,7 @@ type DB struct { maxIdleTimeClosed int64 // Total number of connections closed due to idle time. maxLifetimeClosed int64 // Total number of connections closed due to max connection lifetime limit. - stop func() // stop cancels the connection opener and the session resetter. + stop func() // stop cancels the connection opener. } // connReuseStrategy determines how (*DB).conn returns database connections. -- cgit v1.3 From d4c1ad882973e407ff85b977f4ce5b9435451190 Mon Sep 17 00:00:00 2001 From: Tzu-Chiao Yeh Date: Mon, 24 Aug 2020 22:04:17 +0800 Subject: database/sql: fix tx stmt deadlock when rollback Tx acquires tx.closemu W-lock and then acquires stmt.closemu.W-lock to fully close the transaction and associated prepared statement. Stmt query and execution run in reverse ways - acquires stmt.closemu.R-lock and then acquires tx.closemu.R-lock to grab tx connection, which may cause deadlock. Prevent the lock is held around tx.closePrepared to ensure no deadlock happens. Fixes #40985 Change-Id: If53909822b87bce11861a6e3035ecb9476d2cd17 Reviewed-on: https://go-review.googlesource.com/c/go/+/250178 Run-TryBot: Emmanuel Odeke TryBot-Result: Go Bot Reviewed-by: Daniel Theophanes Trust: Emmanuel Odeke --- src/database/sql/sql.go | 14 +++++++------- src/database/sql/sql_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 7 deletions(-) (limited to 'src/database') diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go index fc7e3e4485..d8f19520c8 100644 --- a/src/database/sql/sql.go +++ b/src/database/sql/sql.go @@ -2087,10 +2087,10 @@ func (tx *Tx) isDone() bool { // that has already been committed or rolled back. var ErrTxDone = errors.New("sql: transaction has already been committed or rolled back") -// closeLocked returns the connection to the pool and +// close returns the connection to the pool and // must only be called by Tx.rollback or Tx.Commit while -// closemu is Locked and tx already canceled. -func (tx *Tx) closeLocked(err error) { +// tx is already canceled and won't be executed concurrently. +func (tx *Tx) close(err error) { tx.releaseConn(err) tx.dc = nil tx.txi = nil @@ -2164,7 +2164,7 @@ func (tx *Tx) Commit() error { // to ensure no other connection has an active query. tx.cancel() tx.closemu.Lock() - defer tx.closemu.Unlock() + tx.closemu.Unlock() var err error withLock(tx.dc, func() { @@ -2173,7 +2173,7 @@ func (tx *Tx) Commit() error { if err != driver.ErrBadConn { tx.closePrepared() } - tx.closeLocked(err) + tx.close(err) return err } @@ -2196,7 +2196,7 @@ func (tx *Tx) rollback(discardConn bool) error { // to ensure no other connection has an active query. tx.cancel() tx.closemu.Lock() - defer tx.closemu.Unlock() + tx.closemu.Unlock() var err error withLock(tx.dc, func() { @@ -2208,7 +2208,7 @@ func (tx *Tx) rollback(discardConn bool) error { if discardConn { err = driver.ErrBadConn } - tx.closeLocked(err) + tx.close(err) return err } diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go index 762d42f54b..8ae6e1339e 100644 --- a/src/database/sql/sql_test.go +++ b/src/database/sql/sql_test.go @@ -2810,6 +2810,36 @@ func TestTxCannotCommitAfterRollback(t *testing.T) { } } +// Issue 40985 transaction statement deadlock while context cancel. +func TestTxStmtDeadlock(t *testing.T) { + db := newTestDB(t, "people") + defer closeDB(t, db) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Millisecond) + defer cancel() + tx, err := db.BeginTx(ctx, nil) + if err != nil { + t.Fatal(err) + } + + stmt, err := tx.Prepare("SELECT|people|name,age|age=?") + if err != nil { + t.Fatal(err) + } + // Run number of stmt queries to reproduce deadlock from context cancel + for i := 0; i < 1e3; i++ { + _, err = stmt.Query(1) + if err != nil { + // Encounter ErrTxDone here is expected due to context cancel + if err != ErrTxDone { + t.Fatalf("unexpected error while executing stmt, err: %v", err) + } + break + } + } + _ = tx.Rollback() +} + // Issue32530 encounters an issue where a connection may // expire right after it comes out of a used connection pool // even when a new connection is requested. -- cgit v1.3 From d8044a6744f9570e964c6e35e12827473ff12cde Mon Sep 17 00:00:00 2001 From: Tzu-Chiao Yeh Date: Thu, 29 Oct 2020 06:03:08 +0800 Subject: database/sql: fix TestTxStmtDeadlock test Drop error check because errors can be not only ErrTxDone for tx stmt executions, and the purpose of the test is just reproducing deadlock. Fixes #42259 Change-Id: I9e7105ada1403ec7064dcc1c3385b36893a1c195 Reviewed-on: https://go-review.googlesource.com/c/go/+/266097 Reviewed-by: Daniel Theophanes Trust: Emmanuel Odeke Run-TryBot: Emmanuel Odeke TryBot-Result: Go Bot --- src/database/sql/sql_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'src/database') diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go index 8ae6e1339e..4b02657b7e 100644 --- a/src/database/sql/sql_test.go +++ b/src/database/sql/sql_test.go @@ -2828,12 +2828,10 @@ func TestTxStmtDeadlock(t *testing.T) { } // Run number of stmt queries to reproduce deadlock from context cancel for i := 0; i < 1e3; i++ { + // Encounter any close related errors (e.g. ErrTxDone, stmt is closed) + // is expected due to context cancel. _, err = stmt.Query(1) if err != nil { - // Encounter ErrTxDone here is expected due to context cancel - if err != ErrTxDone { - t.Fatalf("unexpected error while executing stmt, err: %v", err) - } break } } -- cgit v1.3 From 715d4e2e014fa54f527ee109f26a31f941196381 Mon Sep 17 00:00:00 2001 From: Emmanuel T Odeke Date: Fri, 29 May 2020 17:19:42 -0700 Subject: database/sql, runtime: correct *.Fatal inside goroutines in tests Found by go vet pass "testinggoroutines". Change-Id: I6360af2079617b7aa62dcb9bd7254578ca5d1c1d Reviewed-on: https://go-review.googlesource.com/c/go/+/235527 Run-TryBot: Emmanuel Odeke Reviewed-by: Ian Lance Taylor Trust: Emmanuel Odeke --- src/database/sql/sql_test.go | 20 +++++++++++++++----- src/runtime/syscall_windows_test.go | 12 +++++++----- 2 files changed, 22 insertions(+), 10 deletions(-) (limited to 'src/database') diff --git a/src/database/sql/sql_test.go b/src/database/sql/sql_test.go index 4b02657b7e..c968852ade 100644 --- a/src/database/sql/sql_test.go +++ b/src/database/sql/sql_test.go @@ -2888,20 +2888,26 @@ func TestConnExpiresFreshOutOfPool(t *testing.T) { waitingForConn := make(chan struct{}) go func() { + defer close(afterPutConn) + conn, err := db.conn(ctx, alwaysNewConn) - if err != nil { - t.Fatal(err) + if err == nil { + db.putConn(conn, err, false) + } else { + t.Errorf("db.conn: %v", err) } - db.putConn(conn, err, false) - close(afterPutConn) }() go func() { + defer close(waitingForConn) + for { + if t.Failed() { + return + } db.mu.Lock() ct := len(db.connRequests) db.mu.Unlock() if ct > 0 { - close(waitingForConn) return } time.Sleep(10 * time.Millisecond) @@ -2910,6 +2916,10 @@ func TestConnExpiresFreshOutOfPool(t *testing.T) { <-waitingForConn + if t.Failed() { + return + } + offsetMu.Lock() if ec.expired { offset = 11 * time.Second diff --git a/src/runtime/syscall_windows_test.go b/src/runtime/syscall_windows_test.go index 7705d2a017..3827c6ed83 100644 --- a/src/runtime/syscall_windows_test.go +++ b/src/runtime/syscall_windows_test.go @@ -1143,17 +1143,19 @@ func BenchmarkSyscallToSyscallPing(b *testing.B) { go func() { for i := 0; i < n; i++ { syscall.WaitForSingleObject(event1, syscall.INFINITE) - err := setEvent(event2) - if err != nil { - b.Fatal(err) + if err := setEvent(event2); err != nil { + b.Errorf("Set event failed: %v", err) + return } } }() for i := 0; i < n; i++ { - err := setEvent(event1) - if err != nil { + if err := setEvent(event1); err != nil { b.Fatal(err) } + if b.Failed() { + break + } syscall.WaitForSingleObject(event2, syscall.INFINITE) } } -- cgit v1.3 From 422dc83baa2816ca1d9a0aa3f1aaf4c47c8098ad Mon Sep 17 00:00:00 2001 From: Hein Khant Zaw Date: Thu, 10 Dec 2020 16:45:48 +0000 Subject: database/sql: fix typo in comment Fixes #43116 Change-Id: Ib04fab6ae03f322aa1508ec00523f628d891247a GitHub-Last-Rev: 0a86e665b2c320e0b2aef75ee53bd7281b19b013 GitHub-Pull-Request: golang/go#43122 Reviewed-on: https://go-review.googlesource.com/c/go/+/276992 Reviewed-by: Ian Lance Taylor Reviewed-by: Daniel Theophanes --- src/database/sql/sql.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/database') diff --git a/src/database/sql/sql.go b/src/database/sql/sql.go index d8f19520c8..726aadb899 100644 --- a/src/database/sql/sql.go +++ b/src/database/sql/sql.go @@ -1141,7 +1141,7 @@ func (db *DB) connectionOpener(ctx context.Context) { // Open one new connection func (db *DB) openNewConnection(ctx context.Context) { - // maybeOpenNewConnctions has already executed db.numOpen++ before it sent + // maybeOpenNewConnections has already executed db.numOpen++ before it sent // on db.openerCh. This function must execute db.numOpen-- if the // connection fails or is closed before returning. ci, err := db.connector.Connect(ctx) -- cgit v1.3