Implement driver.Pinger #565
Conversation
The Pinger interface was added in Go 1.8. This implementation ignores the context parameter to Ping.
|
But it seems useless and it may puzzle people who don't know that the ctx isn't used actually |
|
I don't think it's useless: there are cases where a connection to MySQL can be established, but it's not actually ready to start processing commands yet. I agree it's not ideal, but the work in #551 seems to have stalled. Probably because the pull request is so large. This seems like a marginal improvement on what's there now, and it can be built upon. Let's start small. |
connection_go18.go
Outdated
| ) | ||
|
|
||
| func (mc *mysqlConn) Ping(ctx context.Context) error { | ||
| return mc.writeCommandPacket(comPing) |
There was a problem hiding this comment.
https://dev.mysql.com/doc/internals/en/com-ping.html
You should read OK packet after send COM_PING.
Additionally, cancellation should be implemented too.
When Ping is cancelled between PING and OK, the connection should be disposed.
|
Updated. Thanks for the review. |
connection_go18.go
Outdated
| default: | ||
| } | ||
|
|
||
| _, err = mc.readResultOK() |
There was a problem hiding this comment.
This code didn't cancel waiting OK package when ctx is canceled.
There was a problem hiding this comment.
I'm not sure what you mean. Are you saying we should still read the OK result after the Done channel is closed? If so, what's the point if we're closing the connection anyway? And if not, please explain more.
There was a problem hiding this comment.
No. readResultOK() waits OK packet from MySQL server. It may take long time.
While waiting OK packet, context can be cancelled. Then, driver should abandon waiting OK.
There was a problem hiding this comment.
So are you looking for something like this, then?
ch := make(chan err)
go func() {
_, err = mc.readResultOK()
ch <- err
}()
select {
case <-ctx.Done():
mc.Close()
return ctx.Err()
case err := <- ch:
return err
}There was a problem hiding this comment.
FYI, this is how to interrupt conn.Read().
https://github.com/cybozu-go/cmd/blob/21e6f5953122316b450289d4e20fb664d84ffe41/http.go#L226
(thanks to @ymmt2005)
|
Updated again! Hopefully for the final time? 😀 |
| go func() { | ||
| _, err := mc.readResultOK() | ||
| ch <- err | ||
| }() |
There was a problem hiding this comment.
This will cause goroutine leak.
https://gist.github.com/methane/8da51c119193f49e990f501a44155557
| select { | ||
| case <-ctx.Done(): | ||
| mc.netConn.SetReadDeadline(time.Now()) | ||
| return ctx.Err() |
There was a problem hiding this comment.
This is dangerous.
- mc is not thread safe.
mc.readResultOK()is still working.- After
Pingreturn,database/sql.DBwill use this connection again. race may be happen.
|
I think Please stop updating this pull request. |
|
Seems clear that this is not a path forward. Closing. |
The
Pingerinterface was added in Go 1.8.This implementation ignores the context parameter to
Pingbecause wiring it inwould be a much larger change.
Fixes #505.