From 1d798045337ff46c7925151c5de79b3322105cab Mon Sep 17 00:00:00 2001 From: Julien Schmidt Date: Sat, 10 Jun 2017 03:05:51 +0800 Subject: [PATCH 1/3] Add atomic wrappers for bool and error Improves #608 --- connection.go | 48 +++++++++------------------- connection_go18.go | 2 +- packets.go | 6 ++-- statement.go | 6 ++-- transaction.go | 4 +-- utils.go | 64 +++++++++++++++++++++++++++++++++++++ utils_test.go | 78 ++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 165 insertions(+), 43 deletions(-) diff --git a/connection.go b/connection.go index 89a4f464a..2630f5211 100644 --- a/connection.go +++ b/connection.go @@ -14,17 +14,15 @@ import ( "net" "strconv" "strings" - "sync" - "sync/atomic" "time" ) -// a copy of context.Context for Go 1.7 and later. +// a copy of context.Context for Go 1.7 and earlier type mysqlContext interface { Done() <-chan struct{} Err() error - // They are defined in context.Context, but go-mysql-driver does not use them. + // defined in context.Context, but not used in this driver: // Deadline() (deadline time.Time, ok bool) // Value(key interface{}) interface{} } @@ -44,18 +42,13 @@ type mysqlConn struct { parseTime bool strict bool - // for context support (From Go 1.8) + // for context support (Go 1.8+) watching bool watcher chan<- mysqlContext closech chan struct{} finished chan<- struct{} - - // set non-zero when conn is closed, before closech is closed. - // accessed atomically. - closed int32 - - mu sync.Mutex // guards following fields - canceledErr error // set non-nil if conn is canceled + canceled atomicError // set non-nil if conn is canceled + closed atomicBool // set when conn is closed, before closech is closed } // Handles parameters set in DSN after the connection is established @@ -89,7 +82,7 @@ func (mc *mysqlConn) handleParams() (err error) { } func (mc *mysqlConn) Begin() (driver.Tx, error) { - if mc.isBroken() { + if mc.closed.IsSet() { errLog.Print(ErrInvalidConn) return nil, driver.ErrBadConn } @@ -103,7 +96,7 @@ func (mc *mysqlConn) Begin() (driver.Tx, error) { func (mc *mysqlConn) Close() (err error) { // Makes Close idempotent - if !mc.isBroken() { + if !mc.closed.IsSet() { err = mc.writeCommandPacket(comQuit) } @@ -117,7 +110,7 @@ func (mc *mysqlConn) Close() (err error) { // is called before auth or on auth failure because MySQL will have already // closed the network connection. func (mc *mysqlConn) cleanup() { - if atomic.SwapInt32(&mc.closed, 1) != 0 { + if !mc.closed.TrySet(true) { return } @@ -131,13 +124,9 @@ func (mc *mysqlConn) cleanup() { } } -func (mc *mysqlConn) isBroken() bool { - return atomic.LoadInt32(&mc.closed) != 0 -} - func (mc *mysqlConn) error() error { - if mc.isBroken() { - if err := mc.canceled(); err != nil { + if mc.closed.IsSet() { + if err := mc.canceled.Value(); err != nil { return err } return ErrInvalidConn @@ -146,7 +135,7 @@ func (mc *mysqlConn) error() error { } func (mc *mysqlConn) Prepare(query string) (driver.Stmt, error) { - if mc.isBroken() { + if mc.closed.IsSet() { errLog.Print(ErrInvalidConn) return nil, driver.ErrBadConn } @@ -300,7 +289,7 @@ func (mc *mysqlConn) interpolateParams(query string, args []driver.Value) (strin } func (mc *mysqlConn) Exec(query string, args []driver.Value) (driver.Result, error) { - if mc.isBroken() { + if mc.closed.IsSet() { errLog.Print(ErrInvalidConn) return nil, driver.ErrBadConn } @@ -361,7 +350,7 @@ func (mc *mysqlConn) Query(query string, args []driver.Value) (driver.Rows, erro } func (mc *mysqlConn) query(query string, args []driver.Value) (*textRows, error) { - if mc.isBroken() { + if mc.closed.IsSet() { errLog.Print(ErrInvalidConn) return nil, driver.ErrBadConn } @@ -436,19 +425,10 @@ func (mc *mysqlConn) getSystemVar(name string) ([]byte, error) { // finish is called when the query has canceled. func (mc *mysqlConn) cancel(err error) { - mc.mu.Lock() - mc.canceledErr = err - mc.mu.Unlock() + mc.canceled.Set(err) mc.cleanup() } -// canceled returns non-nil if the connection was closed due to context cancelation. -func (mc *mysqlConn) canceled() error { - mc.mu.Lock() - defer mc.mu.Unlock() - return mc.canceledErr -} - // finish is called when the query has succeeded. func (mc *mysqlConn) finish() { if !mc.watching || mc.finished == nil { diff --git a/connection_go18.go b/connection_go18.go index 384603a9e..c4ca4c33f 100644 --- a/connection_go18.go +++ b/connection_go18.go @@ -19,7 +19,7 @@ import ( // Ping implements driver.Pinger interface func (mc *mysqlConn) Ping(ctx context.Context) error { - if mc.isBroken() { + if mc.closed.IsSet() { errLog.Print(ErrInvalidConn) return driver.ErrBadConn } diff --git a/packets.go b/packets.go index 0bc120c0e..a76292488 100644 --- a/packets.go +++ b/packets.go @@ -30,7 +30,7 @@ func (mc *mysqlConn) readPacket() ([]byte, error) { // read packet header data, err := mc.buf.readNext(4) if err != nil { - if cerr := mc.canceled(); cerr != nil { + if cerr := mc.canceled.Value(); cerr != nil { return nil, cerr } errLog.Print(err) @@ -66,7 +66,7 @@ func (mc *mysqlConn) readPacket() ([]byte, error) { // read packet body [pktLen bytes] data, err = mc.buf.readNext(pktLen) if err != nil { - if cerr := mc.canceled(); cerr != nil { + if cerr := mc.canceled.Value(); cerr != nil { return nil, cerr } errLog.Print(err) @@ -134,7 +134,7 @@ func (mc *mysqlConn) writePacket(data []byte) error { mc.cleanup() errLog.Print(ErrMalformPkt) } else { - if cerr := mc.canceled(); cerr != nil { + if cerr := mc.canceled.Value(); cerr != nil { return cerr } mc.cleanup() diff --git a/statement.go b/statement.go index 5855f943f..ae6d33b72 100644 --- a/statement.go +++ b/statement.go @@ -23,7 +23,7 @@ type mysqlStmt struct { } func (stmt *mysqlStmt) Close() error { - if stmt.mc == nil || stmt.mc.isBroken() { + if stmt.mc == nil || stmt.mc.closed.IsSet() { // driver.Stmt.Close can be called more than once, thus this function // has to be idempotent. // See also Issue #450 and golang/go#16019. @@ -45,7 +45,7 @@ func (stmt *mysqlStmt) ColumnConverter(idx int) driver.ValueConverter { } func (stmt *mysqlStmt) Exec(args []driver.Value) (driver.Result, error) { - if stmt.mc.isBroken() { + if stmt.mc.closed.IsSet() { errLog.Print(ErrInvalidConn) return nil, driver.ErrBadConn } @@ -93,7 +93,7 @@ func (stmt *mysqlStmt) Query(args []driver.Value) (driver.Rows, error) { } func (stmt *mysqlStmt) query(args []driver.Value) (*binaryRows, error) { - if stmt.mc.isBroken() { + if stmt.mc.closed.IsSet() { errLog.Print(ErrInvalidConn) return nil, driver.ErrBadConn } diff --git a/transaction.go b/transaction.go index 5d88c0399..417d72793 100644 --- a/transaction.go +++ b/transaction.go @@ -13,7 +13,7 @@ type mysqlTx struct { } func (tx *mysqlTx) Commit() (err error) { - if tx.mc == nil || tx.mc.isBroken() { + if tx.mc == nil || tx.mc.closed.IsSet() { return ErrInvalidConn } err = tx.mc.exec("COMMIT") @@ -22,7 +22,7 @@ func (tx *mysqlTx) Commit() (err error) { } func (tx *mysqlTx) Rollback() (err error) { - if tx.mc == nil || tx.mc.isBroken() { + if tx.mc == nil || tx.mc.closed.IsSet() { return ErrInvalidConn } err = tx.mc.exec("ROLLBACK") diff --git a/utils.go b/utils.go index bd11c6975..de117479d 100644 --- a/utils.go +++ b/utils.go @@ -16,6 +16,7 @@ import ( "fmt" "io" "strings" + "sync/atomic" "time" ) @@ -740,3 +741,66 @@ func escapeStringQuotes(buf []byte, v string) []byte { return buf[:pos] } + +/****************************************************************************** +* Sync utils * +******************************************************************************/ +// noCopy may be embedded into structs which must not be copied +// after the first use. +// +// See https://github.com/golang/go/issues/8005#issuecomment-190753527 +// for details. +type noCopy struct{} + +// Lock is a no-op used by -copylocks checker from `go vet`. +func (*noCopy) Lock() {} + +// atomicBool is a wrapper around uint32 for usage as a boolean value with +// atomic access. +type atomicBool struct { + _noCopy noCopy + value uint32 +} + +// IsSet returns wether the current boolean value is true +func (ab *atomicBool) IsSet() bool { + return atomic.LoadUint32(&ab.value) > 0 +} + +// Set sets the value of the bool regardless of the previous value +func (ab *atomicBool) Set(value bool) { + if value { + atomic.StoreUint32(&ab.value, 1) + } else { + atomic.StoreUint32(&ab.value, 0) + } +} + +// TrySet sets the value of the bool and returns wether the value changed +func (ab *atomicBool) TrySet(value bool) bool { + if value { + return atomic.SwapUint32(&ab.value, 1) == 0 + } + return atomic.SwapUint32(&ab.value, 0) > 0 +} + +// atomicBool is a wrapper for atomically accessed error values +type atomicError struct { + _noCopy noCopy + value atomic.Value +} + +// Set sets the error value regardless of the previous value. +// The value must not be nil +func (ae *atomicError) Set(value error) { + ae.value.Store(value) +} + +// Value returns the current error value +func (ae *atomicError) Value() error { + if v := ae.value.Load(); v != nil { + // this will panic if the value doesn't implement the error interface + return v.(error) + } + return nil +} diff --git a/utils_test.go b/utils_test.go index 0d6c6684f..a4b912386 100644 --- a/utils_test.go +++ b/utils_test.go @@ -195,3 +195,81 @@ func TestEscapeQuotes(t *testing.T) { expect("foo''bar", "foo'bar") // affected expect("foo\"bar", "foo\"bar") // not affected } + +func TestAtomicBool(t *testing.T) { + var ab atomicBool + if ab.IsSet() { + t.Fatal("Expected value to be false") + } + + ab.Set(true) + if ab.value != 1 { + t.Fatal("Set(true) did not set value to 1") + } + if !ab.IsSet() { + t.Fatal("Expected value to be true") + } + + ab.Set(true) + if !ab.IsSet() { + t.Fatal("Expected value to be true") + } + + ab.Set(false) + if ab.value != 0 { + t.Fatal("Set(false) did not set value to 0") + } + if ab.IsSet() { + t.Fatal("Expected value to be false") + } + + ab.Set(false) + if ab.IsSet() { + t.Fatal("Expected value to be false") + } + if ab.TrySet(false) { + t.Fatal("Expected TrySet(false) to fail") + } + if !ab.TrySet(true) { + t.Fatal("Expected TrySet(true) to succeed") + } + if !ab.IsSet() { + t.Fatal("Expected value to be true") + } + + ab.Set(true) + if !ab.IsSet() { + t.Fatal("Expected value to be true") + } + if ab.TrySet(true) { + t.Fatal("Expected TrySet(true) to fail") + } + if !ab.TrySet(false) { + t.Fatal("Expected TrySet(false) to succeed") + } + if ab.IsSet() { + t.Fatal("Expected value to be false") + } +} + +func TestAtomicError(t *testing.T) { + var ae atomicError + if ae.Value() != nil { + t.Fatal("Expected value to be nil") + } + + ae.Set(ErrMalformPkt) + if v := ae.Value(); v != ErrMalformPkt { + if v == nil { + t.Fatal("Value is still nil") + } + t.Fatal("Error did not match") + } + ae.Set(ErrPktSync) + if ae.Value() == ErrMalformPkt { + t.Fatal("Error still matches old error") + } + if v := ae.Value(); v != ErrPktSync { + t.Fatal("Error did not match") + } +} From 66a7d3e85eed7b5d332d9bdf70b14e3263c230b8 Mon Sep 17 00:00:00 2001 From: Julien Schmidt Date: Sat, 10 Jun 2017 03:12:32 +0800 Subject: [PATCH 2/3] Drop Go 1.2 and Go 1.3 support --- .travis.yml | 2 -- README.md | 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/.travis.yml b/.travis.yml index a551785b8..a4236330b 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,8 +1,6 @@ sudo: false language: go go: - - 1.2 - - 1.3 - 1.4 - 1.5 - 1.6 diff --git a/README.md b/README.md index d39b29c90..5a8937aa7 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,7 @@ A MySQL-Driver for Go's [database/sql](https://golang.org/pkg/database/sql/) pac * Optional placeholder interpolation ## Requirements - * Go 1.2 or higher + * Go 1.4 or higher * MySQL (4.1+), MariaDB, Percona Server, Google CloudSQL or Sphinx (2.2.3+) --------------------------------------- @@ -279,7 +279,7 @@ Default: false `rejectreadOnly=true` causes the driver to reject read-only connections. This is for a possible race condition during an automatic failover, where the mysql -client gets connected to a read-only replica after the failover. +client gets connected to a read-only replica after the failover. Note that this should be a fairly rare case, as an automatic failover normally happens when the primary is down, and the race condition shouldn't happen From f0506281c02d674b136ad3bcbf9d8ee06864075f Mon Sep 17 00:00:00 2001 From: Julien Schmidt Date: Sat, 10 Jun 2017 03:39:55 +0800 Subject: [PATCH 3/3] "test" noCopy.Lock() --- utils_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/utils_test.go b/utils_test.go index a4b912386..0041892db 100644 --- a/utils_test.go +++ b/utils_test.go @@ -250,6 +250,8 @@ func TestAtomicBool(t *testing.T) { if ab.IsSet() { t.Fatal("Expected value to be false") } + + ab._noCopy.Lock() // we've "tested" it ¯\_(ツ)_/¯ } func TestAtomicError(t *testing.T) {