Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove errBadConnNoWrite and markBadConn #1583

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

remove errBadConnNoWrite and markBadConn #1583

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aea98e2
remove errBadConnNoWrite and markBadConn
methane May 16, 2024
7c77aef
Merge branch 'master' into bye-errBadConnNoWrite
methane Jun 28, 2024
3e23eef
Update packets.go
methane Jun 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 7 additions & 15 deletions connection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,15 +111,6 @@ func (mc *mysqlConn) handleParams() (err error) {
return
}

// markBadConn replaces errBadConnNoWrite with driver.ErrBadConn.
// This function is used to return driver.ErrBadConn only when safe to retry.
func (mc *mysqlConn) markBadConn(err error) error {
if err == errBadConnNoWrite {
return driver.ErrBadConn
}
return err
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function and errBadConnNoWrite were completely meaningless. There is no need to use errBadConnNoWrite once and convert it to ErrBadConn, as you can just use ErrBadConn directly if you are not sending anything yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed my mind.
markBadConn() is allowlist for ErrBadConn.

If writePacket() returned driver.ErrBadConn instead of errBadConnNoWrite, all error check need to suppress ErrBadConn.

So until we stop returning ErrBadConn, markBadConn() should alive.

// current
if err := f1(); err != nil {
  return nil, mc.markBadConn(err)
}
if err := f2(); err != nil {
  return nil, err
}
if err := f3(); err != nil {
  return nil, err
}
...

// if we removed markBadConn()
if err := f1(); err != nil {
  return nil, err
}
if err := f2(); err != nil {
  // not safe to retry
  if err == errBadConnNoWrite {
    err = InvalidConn
  }
  return nil, err
}
if err := f3(); err != nil {
  if err == errBadConnNoWrite {
    err = InvalidConn
  }
  return nil, err
}
...


func (mc *mysqlConn) Begin() (driver.Tx, error) {
return mc.begin(false)
}
Expand All @@ -138,7 +129,7 @@ func (mc *mysqlConn) begin(readOnly bool) (driver.Tx, error) {
if err == nil {
return &mysqlTx{mc}, err
}
return nil, mc.markBadConn(err)
return nil, err
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure proper error handling and propagation.

The error handling in the begin method should be consistent and clear. Consider adding more specific error messages or handling for different error scenarios.

}

func (mc *mysqlConn) Close() (err error) {
Expand Down Expand Up @@ -340,15 +331,15 @@ func (mc *mysqlConn) Exec(query string, args []driver.Value) (driver.Result, err
copied := mc.result
return &copied, err
}
return nil, mc.markBadConn(err)
return nil, err
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review the error handling in the Exec method.

The error handling in the Exec method should be consistent and clear. Consider adding more specific error messages or handling for different error scenarios.

}

// Internal function to execute commands
func (mc *mysqlConn) exec(query string) error {
handleOk := mc.clearResult()
// Send command
if err := mc.writeCommandPacketStr(comQuery, query); err != nil {
return mc.markBadConn(err)
return err
}

// Read Result
Expand Down Expand Up @@ -378,10 +369,10 @@ func (mc *mysqlConn) Query(query string, args []driver.Value) (driver.Rows, erro

func (mc *mysqlConn) query(query string, args []driver.Value) (*textRows, error) {
handleOk := mc.clearResult()

if mc.closed.Load() {
return nil, driver.ErrBadConn
}

if len(args) != 0 {
if !mc.cfg.InterpolateParams {
return nil, driver.ErrSkip
Expand All @@ -393,10 +384,11 @@ func (mc *mysqlConn) query(query string, args []driver.Value) (*textRows, error)
}
query = prepared
}

// Send command
err := mc.writeCommandPacketStr(comQuery, query)
if err != nil {
return nil, mc.markBadConn(err)
return nil, err
}

// Read Result
Expand Down Expand Up @@ -487,7 +479,7 @@ func (mc *mysqlConn) Ping(ctx context.Context) (err error) {

handleOk := mc.clearResult()
if err = mc.writeCommandPacket(comPing); err != nil {
return mc.markBadConn(err)
return err
}

return handleOk.readResultOK()
Expand Down
8 changes: 4 additions & 4 deletions connection_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,8 @@ func TestPingMarkBadConnection(t *testing.T) {

err := mc.Ping(context.Background())

if err != driver.ErrBadConn {
t.Errorf("expected driver.ErrBadConn, got %#v", err)
if !errors.Is(err, nc.err) {
t.Errorf("expected %v, got %#v", nc.err, err)
}
}

Expand All @@ -186,8 +186,8 @@ func TestPingErrInvalidConn(t *testing.T) {

err := mc.Ping(context.Background())

if err != nc.err {
t.Errorf("expected %#v, got %#v", nc.err, err)
if !errors.Is(err, nc.err) {
t.Errorf("expected %v, got %#v", nc.err, err)
}
}

Expand Down
6 changes: 0 additions & 6 deletions errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,6 @@ var (
ErrPktSyncMul = errors.New("commands out of sync. Did you run multiple statements at once?")
ErrPktTooLarge = errors.New("packet for query is too large. Try adjusting the `Config.MaxAllowedPacket`")
ErrBusyBuffer = errors.New("busy buffer")

// errBadConnNoWrite is used for connection errors where nothing was sent to the database yet.
// If this happens first in a function starting a database interaction, it should be replaced by driver.ErrBadConn
// to trigger a resend. Use mc.markBadConn(err) to do this.
// See https://github.com/go-sql-driver/mysql/pull/302
errBadConnNoWrite = errors.New("bad connection")
)

var defaultLogger = Logger(log.New(os.Stderr, "[mysql] ", log.Ldate|log.Ltime))
Expand Down
38 changes: 16 additions & 22 deletions packets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,17 +125,11 @@ func (mc *mysqlConn) writePacket(data []byte) error {

n, err := mc.netConn.Write(data[:4+size])
if err != nil {
mc.cleanup()
if cerr := mc.canceled.Value(); cerr != nil {
return cerr
}
mc.cleanup()
if n == 0 && pktLen == len(data)-4 {
// only for the first loop iteration when nothing was written yet
mc.log(err)
return errBadConnNoWrite
} else {
return err
}
return err
}
if n != 4+size {
// io.Writer(b) must return a non-nil error if it cannot write len(b) bytes.
Expand Down Expand Up @@ -305,8 +299,8 @@ func (mc *mysqlConn) writeHandshakeResponsePacket(authResp []byte, plugin string
data, err := mc.buf.takeBuffer(pktLen + 4)
if err != nil {
// cannot take the buffer. Something must be wrong with the connection
mc.log(err)
return errBadConnNoWrite
mc.cleanup()
return err
Comment on lines +302 to +303
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor repeated error handling patterns.

There is a repeated pattern where a buffer acquisition fails and mc.cleanup() is called before returning the error. This pattern could be abstracted into a helper method to reduce code duplication and improve maintainability.

- mc.cleanup()
- return err
+ return handleBufferAcquisitionError(mc, err)

And then define the helper function:

func handleBufferAcquisitionError(mc *mysqlConn, err error) error {
    mc.cleanup()
    return err
}

Also applies to: 391-392, 411-412, 430-431, 451-452

}

// ClientFlags [32 bit]
Expand Down Expand Up @@ -394,8 +388,8 @@ func (mc *mysqlConn) writeAuthSwitchPacket(authData []byte) error {
data, err := mc.buf.takeSmallBuffer(pktLen)
if err != nil {
// cannot take the buffer. Something must be wrong with the connection
mc.log(err)
return errBadConnNoWrite
mc.cleanup()
return err
}

// Add the auth data [EOF]
Expand All @@ -414,8 +408,8 @@ func (mc *mysqlConn) writeCommandPacket(command byte) error {
data, err := mc.buf.takeSmallBuffer(4 + 1)
if err != nil {
// cannot take the buffer. Something must be wrong with the connection
mc.log(err)
return errBadConnNoWrite
mc.cleanup()
return err
}

// Add command byte
Expand All @@ -433,8 +427,8 @@ func (mc *mysqlConn) writeCommandPacketStr(command byte, arg string) error {
data, err := mc.buf.takeBuffer(pktLen + 4)
if err != nil {
// cannot take the buffer. Something must be wrong with the connection
mc.log(err)
return errBadConnNoWrite
mc.cleanup()
return err
}

// Add command byte
Expand All @@ -454,8 +448,8 @@ func (mc *mysqlConn) writeCommandPacketUint32(command byte, arg uint32) error {
data, err := mc.buf.takeSmallBuffer(4 + 1 + 4)
if err != nil {
// cannot take the buffer. Something must be wrong with the connection
mc.log(err)
return errBadConnNoWrite
mc.cleanup()
return err
}

// Add command byte
Expand Down Expand Up @@ -997,8 +991,8 @@ func (stmt *mysqlStmt) writeExecutePacket(args []driver.Value) error {
}
if err != nil {
// cannot take the buffer. Something must be wrong with the connection
mc.log(err)
return errBadConnNoWrite
mc.cleanup()
return err
Comment on lines +994 to +995
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handle buffer acquisition failures more gracefully.

When buffer acquisition fails, the method writeExecutePacket simply cleans up and returns the error. Consider implementing a retry mechanism or a more informative error message to help diagnose issues related to buffer allocation failures.

}

// command [1 byte]
Expand Down Expand Up @@ -1196,8 +1190,8 @@ func (stmt *mysqlStmt) writeExecutePacket(args []driver.Value) error {
if valuesCap != cap(paramValues) {
data = append(data[:pos], paramValues...)
if err = mc.buf.store(data); err != nil {
mc.log(err)
return errBadConnNoWrite
mc.cleanup()
return err
}
}

Expand Down
4 changes: 2 additions & 2 deletions statement.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func (stmt *mysqlStmt) Exec(args []driver.Value) (driver.Result, error) {
// Send command
err := stmt.writeExecutePacket(args)
if err != nil {
return nil, stmt.mc.markBadConn(err)
return nil, err
}

mc := stmt.mc
Expand Down Expand Up @@ -99,7 +99,7 @@ func (stmt *mysqlStmt) query(args []driver.Value) (*binaryRows, error) {
// Send command
err := stmt.writeExecutePacket(args)
if err != nil {
return nil, stmt.mc.markBadConn(err)
return nil, err
}

mc := stmt.mc
Expand Down
Loading