Context cancelling cause close connection without sending COMM_QUIT for authenticated connection

[minhquang4334]

Issue description

startWatcher() watches the context provided by the Go client. When context cancellation occurs, this method only cleanup() the connection without sending COMM_QUIT, regardless of whether the connection has already been authenticated or not.

Without COMM_QUIT, MySQL Server will abort authenticated clients, and increase Aborted_clients metric.

Related code

• Current handling when context cancel:

  • mysql/connection.go

    Lines 439 to 441 in c9f41c0

    	func (mc *mysqlConn) cancel(err error) {
    	mc.canceled.Set(err)
    	mc.cleanup()

• But cleanup() is only called before auth or on auth failure

  • mysql/connection.go

    Lines 134 to 138 in c9f41c0

    	// Closes the network connection and unsets internal variables. Do not call this
    	// function after successfully authentication, call Close instead. This function
    	// is called before auth or on auth failure because MySQL will have already
    	// closed the network connection.
    	func (mc *mysqlConn) cleanup() {

• Close() should be called if the connection has been authenticated successfully with sending COMM_QUIT to MySQL server

  • https://github.com/go-sql-driver/mysql/blob/master/connection.go#L119

Error log

If you have an error log, please paste it here.

Configuration

Driver version (or git SHA): 1.8.1

Go version: run go version in your console

Server version: E.g. MySQL 5.6, MariaDB 10.0.20

Server OS: E.g. Debian 8.1 (Jessie), Windows 10

[minhquang4334]

@methane Would you be able to confirm that this is the intended behavior? Is it a bug?

[methane]

It is intended behavior, not a bug.
I do not recommend using context cancel in regular case because it can not stop query.
I recommend max_execution_time hint instead.
https://dev.mysql.com/doc/refman/8.4/en/optimizer-hints.html

On the other hand, I do not against sending COM_QUIT on cancel. I will consider about it is really safe or not.

[minhquang4334]

@methane thanks for reply

It is intended behavior, not a bug.

I think to properly close an authenticated connection, we need to send a COMM_QUIT packet to the MySQL server. This will prevent the MySQL server from treating the connection as aborted, which can negatively impact performance monitoring. (I spend a lot of time investigating frequent MySQL connection aborts.)

On the other hand, I do not against sending COM_QUIT on cancel. I will consider about it is really safe or not.

I agree that the SQL driver should send COMM_QUIT on cancel in a safe and reliable manner.

I do not recommend using context cancel in regular case because it can not stop query.
I recommend max_execution_time hint instead.
https://dev.mysql.com/doc/refman/8.4/en/optimizer-hints.html

Did you mean that the database query will run despite context cancel occurred?

What are the best practices for using context in queries, Would you be able to share any example, the case we should context or not.

[methane]

Did you mean that the database query will run despite context cancel occurred?

Yes. MySQL protocol doesn't provide safe&robust way to cancel running query.

What are the best practices for using context in queries, Would you be able to share any example, the case we should context or not.

• Ensure context cancel doesn't happen in regular cases.
• set max_execution_time on connection variable or optimizer hint to prevent query running unintentionally long time.

I will add some option for context cancellation next year (e.g. ignore context cancel at all, safe for MariaDB, and not-safe & use it your own risk version). But I cannot promise it.
My company doesn't need it at the moment so it is a my spare time project.