Implement Pinger interface

[photon3108]

Description

Implement Pinger interface #505

Checklist

• Code compiles correctly
• Created tests which fail without the change (if possible)
• All tests passing
• Extended the README / documentation, if necessary
• Added myself / the copyright holder to the AUTHORS file

[julienschmidt]

Why do both the buffer and the mysqlConn need a Context?

[photon3108]

buffer.fill() sets the timeout by SetReadDeadline() and mysqlConn.writePacket() sets the timeout by SetWriteDeadline. Context.Deadline() may be earlier than those two timeouts. So Context.Deadline() must be passed into these two functions. There are two ways to pass it. The first one is to pass it as a parameter of a function. The second one is to pass it as a member of a struct. If I chose the first one, I would have to modify every functions from mysqlConn.Ping() to buffer.fill() and mysqlConn.writePacket(). The second one is much easier and there is no need to wrap Context by something like context.withCancel() in this case.

The real context is stored at mysqlConn.ctx and the buffer.ctx is just a wrapper/handler which points to the previous one. I use wrapper/handler because copying a value everywhere is not a good idea. People may modify some and forget the others.

[methane]

Context.Deadline() may be earlier than those two timeouts. So Context.Deadline() must be passed into these two functions.

How about calling .Close() instead of .SetDeadline() when <- ctx.Done()?
Cancelled connection is mostly unusable furthermore.

[photon3108]

There are two reasons why I didn't use netConn.Close() when <- ctx.Done(). One is even if ctx.Done() happens and no matter it calls netConn.Close() or netConn.SetDeadline(), mysqlConn.pingImpl() may still send nil error to line 65 before these two functions interrupt it. In this situation, netConn doesn't have to be closed and it's safe to reuse this connection. BTW, mysqlConn.Ping() checks the received err at line 72. If the err shows the connection timed out, it will returns ErrBadConn and let caller close it.

The another reason is about the life cycle of mysqlConn. I think It is created and managed in database/sql/sql.go. It should be destroyed in the same level/place. Actually it does in DB.putConn() if Pinger.Ping() returns ErrBadConn. Let DB.putConn() does what its duty is and mysqlConn.Ping() should not make DB.putConn() close it again.

[methane]

One is even if ctx.Done() happens and no matter it calls netConn.Close() or netConn.SetDeadline(), mysqlConn.pingImpl() may still send nil error to line 65 before these two functions interrupt it. In this situation, netConn doesn't have to be closed and it's safe to reuse this connection.

Cancelleration is rare situation. Code simplicity (maintenance cost) is more important than how cancellation efficient.

The another reason is about the life cycle of mysqlConn. I think It is created and managed in database/sql/sql.go. It should be destroyed in the same level/place.

But TCP lifetime can be shorter than mysqlConn's lifetime.

[photon3108]

Cancelleration is rare situation. Code simplicity (maintenance cost) is more important than how cancellation efficient.

I like KISS principle and also think simplicity is important but from my point of view, I still think there is a short and blurry distance between simplicity and correctness in this case. The key point is that force to close any connections no matter they are normal or unusable is good here? I prefer that let normal one reuse again and let caller close the unusable one. It's more logical. Actually netConn.SetDeadline() and UpdateDeadlineLoop are not so hard to understand. I wrote the comments. But this is from my perspective. If you ask me to use netConn.Close() and remove UpdateDeadlineLoop, I could do it for you.

But TCP lifetime can be shorter than mysqlConn's lifetime.

Technically, yes, but mysqlConn.netConn, a TCP connection, will be closed in mysqlConn.Close() and mysqlConn.Close() will be called by DB.putConn() if Pinger.Ping() returns ErrBadConn. In other words, Golang database/sql/sql.go has already provided the mechanism to close a driver.Conn(mysql.mysqlConn), and indirectly destroy its underlying resource. Why should we do the same job again at each corner and duplicate the codes and the logics? Are there any bugs about compatibility from the earlier versions of Golang?

[methane]

if Pinger.Ping() returns ErrBadConn. In other words, Golang database/sql/sql.go has already provided the mechanism to close a driver.Conn(mysql.mysqlConn), and indirectly destroy its underlying resource.

It's true only in Ping. We can't use ErrBadConn for QueryContext() or other APIs to close connection from caller soon.
I want to use same pattern as possible for Ping, QueryContext() and other APIs.

https://golang.org/pkg/database/sql/driver/
To prevent duplicate operations, ErrBadConn should NOT be returned if there's a possibility that the database server might have performed the operation. Even if the server sends back an error, you shouldn't return ErrBadConn.

[photon3108]

I want to use same pattern as possible for Ping, QueryContext() and other APIs.

I see what you concern. How about still letting normal connection reuse agagin but replacing line 72~80 with these:

if err == nil {
  return nil
}
if mc.netConn != nil {
  errClose := mc.netConn.Close()
  if errClose != nil {
    errLog.Print(errClose)
  }
  mc.netConn = nil
}
return err

[methane]

Would ctx.Done() return nil?
Example code in this document doesn't check nil.

[photon3108]

Yes, context.Background().Done() would returns nil. [1][2]

[methane]

Context.Deadline() may be earlier than those two timeouts. So Context.Deadline() must be passed into these two functions.

How about calling .Close() instead of .SetDeadline() when <- ctx.Done()?
Cancelled connection is mostly unusable furthermore.