Close connection on ErrPktSync and ErrPktSyncMul (#1473)

An `ErrPktSync` or `ErrPktSyncMul` error always means that a packet
header has been read, but since the sequence ID was not correct then the
packet payload has not been read. This results in the connection being
left in a broken state, since any future operations will always result
in a "busy buffer" error. Keeping such connections alive leads to them
being repeatedly returned to the pool in this state, which can in turn
result in a large number of failures due to these "busy buffer" errors.

This commit fixes this problem by simply closing the connection before
returning either `ErrPktSync` or `ErrPktSyncMul`. This ensures that the
connection won't be returned to the pool, preventing it from causing any
further errors.

Author: owbone

AUTHORS

@@ -77,6 +77,7 @@ Maciej Zimnoch <maciej.zimnoch at codilime.com>
 Michael Woolnough <michael.woolnough at gmail.com>
 Nathanial Murphy <nathanial.murphy at gmail.com>
 Nicola Peduzzi <thenikso at gmail.com>
+Oliver Bone <owbone at github.com>
 Olivier Mengué <dolmen at cpan.org>
 oscarzhao <oscarzhaosl at gmail.com>
 Paul Bonser <misterpib at gmail.com>

packets.go

@@ -44,6 +44,7 @@ func (mc *mysqlConn) readPacket() ([]byte, error) {
 
 		// check packet sync [8 bit]
 		if data[3] != mc.sequence {
+			mc.Close()
 			if data[3] > mc.sequence {
 				return nil, ErrPktSyncMul
 			}

packets_test.go

@@ -133,30 +133,34 @@ func TestReadPacketSingleByte(t *testing.T) {
 }
 
 func TestReadPacketWrongSequenceID(t *testing.T) {
-	conn := new(mockConn)
-	mc := &mysqlConn{
-		buf: newBuffer(conn),
-	}
-
-	// too low sequence id
-	conn.data = []byte{0x01, 0x00, 0x00, 0x00, 0xff}
-	conn.maxReads = 1
-	mc.sequence = 1
-	_, err := mc.readPacket()
-	if err != ErrPktSync {
-		t.Errorf("expected ErrPktSync, got %v", err)
-	}
-
-	// reset
-	conn.reads = 0
-	mc.sequence = 0
-	mc.buf = newBuffer(conn)
-
-	// too high sequence id
-	conn.data = []byte{0x01, 0x00, 0x00, 0x42, 0xff}
-	_, err = mc.readPacket()
-	if err != ErrPktSyncMul {
-		t.Errorf("expected ErrPktSyncMul, got %v", err)
+	for _, testCase := range []struct {
+		ClientSequenceID byte
+		ServerSequenceID byte
+		ExpectedErr      error
+	}{
+		{
+			ClientSequenceID: 1,
+			ServerSequenceID: 0,
+			ExpectedErr:      ErrPktSync,
+		},
+		{
+			ClientSequenceID: 0,
+			ServerSequenceID: 0x42,
+			ExpectedErr:      ErrPktSyncMul,
+		},
+	} {
+		conn, mc := newRWMockConn(testCase.ClientSequenceID)
+
+		conn.data = []byte{0x01, 0x00, 0x00, testCase.ServerSequenceID, 0xff}
+		_, err := mc.readPacket()
+		if err != testCase.ExpectedErr {
+			t.Errorf("expected %v, got %v", testCase.ExpectedErr, err)
+		}
+
+		// connection should not be returned to the pool in this state
+		if mc.IsValid() {
+			t.Errorf("expected IsValid() to be false")
+		}
 	}
 }