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.

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

Detect closed connection using non-blocking read on socket #821

Closed
bgrainger opened this issue May 21, 2020 · 3 comments
Closed

Detect closed connection using non-blocking read on socket #821

bgrainger opened this issue May 21, 2020 · 3 comments

Comments

@bgrainger
Copy link
Member

From https://github.blog/2020-05-20-three-bugs-in-the-go-mysql-driver/

There are two ways to check the health of that connection: at the MySQL level, or at the TCP level. A MySQL health check involves sending a PING packet and waiting for its response. It’s always safe to return driver.ErrBadConn because the ping packet doesn’t perform write operations in MySQL (one would hope). However, there is the drawback of adding arbitrary latency to the first operation performed on a connection fresh off the pool.

Performing this check is very inexpensive, all we have to do is a non-blocking read on our connection before we attempt any writes. If the server has closed its side of the connection, we'll get an EOF right away. If the connection is still open, the read also returns immediately but with an EWOULDBLOCK error, signaling that no data exists to be read (yet).

Investigate whether this approach can also be used in MySqlConnector to avoid the latency of sending the Ping packet. If so, the ConnectionIdlePingTime connection string option can be ignored/deprecated/removed.

@comex
Copy link

comex commented May 21, 2020

Hi, just an HN reader poking in. Fair warning: I‘m not actually a user of your library or even of MySQL, so please correct me if I’m mistaken. But just based on the description, this approach seems somewhat dangerous.

When the server receives a ping packet, it presumably resets the connection’s idle timer. So once you successfully complete a ping, the connection is then guaranteed not to time out before the query itself arrives, unless the time between sending the ping and sending the query is itself comparable to the timeout value (but presumably it should be orders of magnitude lower).

A non-blocking read doesn’t have that guarantee. It only checks whether the client has gotten a TCP RST at the time of the read, but it obviously can’t reset the server’s timer since it doesn’t contact the server. So the client could still receive an RST at any point between performing the non-blocking read and sending the actual query. Presumably that would result in the query failing, even though there’s nothing wrong with the server.

So you’d have a rare source of random spurious query failures. That might not be the end of the world; for most applications, random query failures are already a possibility due to factors like network issues, the server being restarted, etc. But it certainly sounds surprising.

@bgrainger
Copy link
Member Author

When the server receives a ping packet, it presumably resets the connection’s idle timer.

Yes, this is true (and I hadn't been thinking of it as a useful side-effect, but it is).

So you’d have a rare source of random spurious query failures.

Agreed, and it could depend a lot on how much time elapses (in user code) between calling connection.Open() (which retrieves an idle connection from the pool) and user code executing the first query.

The current behaviour may be much more reliable for most users (at the cost of a slight increase in latency when opening).

@bgrainger
Copy link
Member Author

This would be a very niche optimisation because (until #178 is implemented) all connections are reset (which already requires a server roundtrip) when they're retrieved from the pool (unless ConnectionReset=false).

This might be worth doing (as an alternative to pinging) once that's implemented; closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants