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

Add deadline for connection #394

Closed
wants to merge 1 commit into from
Closed

Add deadline for connection #394

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 12, 2015

#375

#392

Signed-off-by: ZhiFeng Hu hufeng1987@gmail.com

Signed-off-by: ZhiFeng Hu <hufeng1987@gmail.com>
@@ -71,6 +72,9 @@ func (d MySQLDriver) Open(dsn string) (driver.Conn, error) {

// Enable TCP Keepalives on TCP connections
if tc, ok := mc.netConn.(*net.TCPConn); ok {
timeout := time.Now().Add(mc.cfg.timeout)
tc.SetReadDeadline(timeout)
tc.SetDeadline(timeout)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's total bad place to set deadline.
Additionaly, why do you set both of ReadDeadline and Deadline?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Control the timeout. if connection broken. exit early. then we can reconnect again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you really tested your code?

@ghost ghost closed this Dec 12, 2015
@ghost
Copy link
Author

ghost commented Dec 12, 2015

I had tested for my production apps. thanks for your attentions. i choose to close this pr. sorry for any uncomfortable noise.

@methane
Copy link
Member

methane commented Dec 12, 2015

I think #372 is what you want to do.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants