-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Implement driver.Pinger #565
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
Conversation
The Pinger interface was added in Go 1.8. This implementation ignores the context parameter to Ping.
caibirdme
commented
Apr 3, 2017
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code didn't cancel waiting OK package when ctx is canceled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will cause goroutine leak.
https://gist.github.com/methane/8da51c119193f49e990f501a44155557
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 mc.writeCommandPacket() and mc.readResultOK() should support context,
instead implementing cancellation in mc.Ping().
Please stop updating this pull request.
I need more time to think about internal design. Implementing context support is much harder
than people think.
Seems clear that this is not a path forward. Closing.
Uh oh!
There was an error while loading. Please reload this page.
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.