From bc5e6eaa6d4fc8a7c8668050c7b1813afeafcabe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Vicent=20Mart=C3=AD?= Date: Fri, 29 Mar 2019 09:29:56 +0100 Subject: [PATCH] check connection liveness before writing query (#934) This commit contains a potential fix to the issue reported in #657. As a summary: when a MySQL server kills a connection on the server-side (either because it is actively pruning connections, or because the connection has hit the server-side timeout), the Go MySQL client does not immediately become aware of the connection being dead. Because of the way TCP works, the client cannot know that the connection has received a RST packet from the server (i.e. the server-side has closed) until it actually reads from it. This causes an unfortunate bug wherein a MySQL idle connection is pulled from the connection pool, a query packet is written to it without error, and then the query fails with an "unexpected EOF" error when trying to read the response packet. Since the initial write to the socket does not fail with an error, it is generally not safe to return `driver.ErrBadConn` when the read fails, because in theory the write could have arrived to the server and could have been committed. Returning `ErrBadConn` could lead to duplicate inserts on the database and data corruption because of the way the Go SQL package performs retries. In order to significantly reduce the circumstances where this "unexpected EOF" error is returned for stale connections, this commit performs a liveness check before writing a new query. When do we check? ----------------- This check is not performed for all writes. Go 1.10 introduced a new `sql/driver` interface called `driver.SessionResetter`, which calls the `ResetSession` method on any connections _when they are returned to the connection pool_. Since performing the liveness check during `ResetSession` is not particularly useful (the connection can spend a long time in the pool before it's checked out again, and become stale), we simply mark the connection with a `reset` flag instead. This `reset` flag is then checked from `mysqlConn.writePacket` to perform the liveness checks. This ensures that the liveness check will only be performed for the first query on a connection that has been checked out of the connection pool. These are pretty much the semantics we want: a fresh connection from the pool is more likely to be stale, and it has not performed any previous writes that could cause data corruption. If a connection is being consistently used by the client (i.e. through an open transaction), we do NOT perform liveness checks. If MySQL Server kills such active connection, we want to bubble up the error to the user because any silent retrying can and will lead to data corruption. Since the `ResetSession` interface is only available in Go 1.10+, the liveness checks will only be performed starting with that Go version. How do we check? ---------------- To perform the actual liveness test on the connection, we use the new `syscall.Conn` interface which is available for all `net.Conn`s since Go 1.9. The `SyscallConn` method returns a `RawConn` that lets us read directly from the connection's file descriptor using syscalls, and skipping the default read pipeline of the Go runtime. When reading directly from the file descriptor using `syscall.Read`, we pass in a 1-length buffer, as passing a 0-length buffer will always result in a 0-length read, and the 1-length buffer will never be filled because we're not expecting any reads from MySQL before we have written any request packets in a fresh connection. All sockets created in the Go runtime are set to non-blocking (O_NONBLOCK). Consequently, we can detect a socket that has been closed on the server-side because the `read` syscall will return a 0-length read _and_ no error. We assume that any other errors returned from the `read` also mean the connection is in a bad state, except for `EAGAIN`/`EWOULDBLOCK`, which is the expected return for a healthy non-blocking socket in this circumstance. Because of the dependency on `syscall.Conn`, liveness checks can only be performed in Go 1.9+. This restriction however overlaps with the fact that we only mark connections as having been reset in Go 1.10+, as explained in the previous section. --- AUTHORS | 1 + conncheck.go | 53 ++++++++++++++++++++++++++++++++++++++++++++ conncheck_test.go | 38 +++++++++++++++++++++++++++++++ conncheck_windows.go | 15 +++++++++++++ connection.go | 3 +++ packets.go | 20 +++++++++++++++++ 6 files changed, 130 insertions(+) create mode 100644 conncheck.go create mode 100644 conncheck_test.go create mode 100644 conncheck_windows.go diff --git a/AUTHORS b/AUTHORS index 146cdffdd..fb6668346 100644 --- a/AUTHORS +++ b/AUTHORS @@ -88,6 +88,7 @@ Zhenye Xie Barracuda Networks, Inc. Counting Ltd. +GitHub Inc. Google Inc. InfoSum Ltd. Keybase Inc. diff --git a/conncheck.go b/conncheck.go new file mode 100644 index 000000000..fa868e84d --- /dev/null +++ b/conncheck.go @@ -0,0 +1,53 @@ +// Go MySQL Driver - A MySQL-Driver for Go's database/sql package +// +// Copyright 2019 The Go-MySQL-Driver Authors. All rights reserved. +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at http://mozilla.org/MPL/2.0/. + +// +build !windows + +package mysql + +import ( + "errors" + "io" + "net" + "syscall" +) + +var errUnexpectedRead = errors.New("unexpected read from socket") + +func connCheck(c net.Conn) error { + var ( + n int + err error + buff [1]byte + ) + + sconn, ok := c.(syscall.Conn) + if !ok { + return nil + } + rc, err := sconn.SyscallConn() + if err != nil { + return err + } + rerr := rc.Read(func(fd uintptr) bool { + n, err = syscall.Read(int(fd), buff[:]) + return true + }) + switch { + case rerr != nil: + return rerr + case n == 0 && err == nil: + return io.EOF + case n > 0: + return errUnexpectedRead + case err == syscall.EAGAIN || err == syscall.EWOULDBLOCK: + return nil + default: + return err + } +} diff --git a/conncheck_test.go b/conncheck_test.go new file mode 100644 index 000000000..b7234b0f5 --- /dev/null +++ b/conncheck_test.go @@ -0,0 +1,38 @@ +// Go MySQL Driver - A MySQL-Driver for Go's database/sql package +// +// Copyright 2013 The Go-MySQL-Driver Authors. All rights reserved. +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at http://mozilla.org/MPL/2.0/. + +// +build go1.10,!windows + +package mysql + +import ( + "testing" + "time" +) + +func TestStaleConnectionChecks(t *testing.T) { + runTests(t, dsn, func(dbt *DBTest) { + dbt.mustExec("SET @@SESSION.wait_timeout = 2") + + if err := dbt.db.Ping(); err != nil { + dbt.Fatal(err) + } + + // wait for MySQL to close our connection + time.Sleep(3 * time.Second) + + tx, err := dbt.db.Begin() + if err != nil { + dbt.Fatal(err) + } + + if err := tx.Rollback(); err != nil { + dbt.Fatal(err) + } + }) +} diff --git a/conncheck_windows.go b/conncheck_windows.go new file mode 100644 index 000000000..3d9e63f66 --- /dev/null +++ b/conncheck_windows.go @@ -0,0 +1,15 @@ +package mysql + +// Go MySQL Driver - A MySQL-Driver for Go's database/sql package +// +// Copyright 2019 The Go-MySQL-Driver Authors. All rights reserved. +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at http://mozilla.org/MPL/2.0/. + +import "net" + +func connCheck(c net.Conn) error { + return nil +} diff --git a/connection.go b/connection.go index fc4ec7597..265fd4e47 100644 --- a/connection.go +++ b/connection.go @@ -22,6 +22,7 @@ import ( type mysqlConn struct { buf buffer netConn net.Conn + rawConn net.Conn // underlying connection when netConn is TLS connection. affectedRows uint64 insertId uint64 cfg *Config @@ -32,6 +33,7 @@ type mysqlConn struct { status statusFlag sequence uint8 parseTime bool + reset bool // set when the Go SQL package calls ResetSession // for context support (Go 1.8+) watching bool @@ -639,5 +641,6 @@ func (mc *mysqlConn) ResetSession(ctx context.Context) error { if mc.closed.IsSet() { return driver.ErrBadConn } + mc.reset = true return nil } diff --git a/packets.go b/packets.go index 5e0853767..70d0d71f5 100644 --- a/packets.go +++ b/packets.go @@ -96,6 +96,25 @@ func (mc *mysqlConn) writePacket(data []byte) error { return ErrPktTooLarge } + // Perform a stale connection check. We only perform this check for + // the first query on a connection that has been checked out of the + // connection pool: a fresh connection from the pool is more likely + // to be stale, and it has not performed any previous writes that + // could cause data corruption, so it's safe to return ErrBadConn + // if the check fails. + if mc.reset { + mc.reset = false + conn := mc.netConn + if mc.rawConn != nil { + conn = mc.rawConn + } + if err := connCheck(conn); err != nil { + errLog.Print("closing bad idle connection: ", err) + mc.Close() + return driver.ErrBadConn + } + } + for { var size int if pktLen >= maxPacketSize { @@ -332,6 +351,7 @@ func (mc *mysqlConn) writeHandshakeResponsePacket(authResp []byte, plugin string if err := tlsConn.Handshake(); err != nil { return err } + mc.rawConn = mc.netConn mc.netConn = tlsConn mc.buf.nc = tlsConn }