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

database/sql: why not check connection timeout in putConn #27241

Closed
treeinlake opened this issue Aug 26, 2018 · 8 comments
Closed

database/sql: why not check connection timeout in putConn #27241

treeinlake opened this issue Aug 26, 2018 · 8 comments

Comments

@treeinlake
Copy link

@treeinlake treeinlake commented Aug 26, 2018

What did you see instead?

In function putConn, there is no check driverConn timeout (which is set in db.SetConnMaxLifetime)

And in fuction conn there is conn.expired(lifetime)

What did you expect to see?

Why not check the connection timeout before put it into the idle connection pool?
Maybe it will be late to check it when we really get it from pool and return a driver.ErrBadConn.

@agnivade
Copy link
Contributor

@agnivade agnivade commented Aug 27, 2018

@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 27, 2018

Drivers should implement ResetConnection, if only to check the connection status before use.

I don't remember the reason off the top of my head.

@treeinlake
Copy link
Author

@treeinlake treeinlake commented Aug 27, 2018

[mysql] ... packets.go:36: read tcp ip:port->ip2:port2: i/o timeout
err: invalid connection
@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 27, 2018

Looks like it is called ResetSession and is implemented here: https://github.com/go-sql-driver/mysql/blob/749ddf1598b47e3cd909414bda735fe790ef3d30/connection.go#L649 .

From my humble opinion, I think the current way of Sql.go is easy to trigger the driver.ErrBadConn (https://github.com/golang/go/blob/master/src/database/sql/sql.go#L1105),
and may let the driver ended in sth like below ( under settings like SetMaxOpenConns(1000) && set SetMaxIdleConns(1000) && set SetConnMaxLifetime(30)):

I'm not sure what you are trying to say here. Are you trying to avoid messages like "err: invalid connection"? Are you seeing this in your logs?

@treeinlake
Copy link
Author

@treeinlake treeinlake commented Aug 28, 2018

  • @kardianos yes, I saw this in my logs and I am trying to avoid it, is there another better way?
  • What I want to say is when a connection exceeds the ConnMaxLifetime at the end of a time-consuming database operation, if we put it into the pool without checking its lifetime, then a driver.ErrBadConn will happen. Why not we avoid this situation instead of letting the user handle it? Thanks for your attention.
  • @dolmen Thank you too.
@kardianos
Copy link
Contributor

@kardianos kardianos commented Aug 28, 2018

You're chasing the wrong tree I think, if you are trying to get around [mysql] ... packets.go:36: read tcp ip:port->ip2:port2: i/o timeout

Try to reproduce it locally in a test case. Try to adjust the sql.DB pool's settings, then try to adjust the MySQL server settings. It probably has to do with the connection string you are using would be my guess.

@FiloSottile FiloSottile changed the title database/sql/Sql.go - Why not check connection timeout before put it into the pool database/sql: why not check connection timeout in putConn Aug 30, 2018
@FiloSottile FiloSottile added this to the Unplanned milestone Aug 30, 2018
@methane
Copy link
Contributor

@methane methane commented Oct 27, 2018

ConnMaxLifetime and i/o timeout are totally unrelated.
You used readTimeout, or writeTimeout or timeout of mysql. That caused the i/o error.
And because of database/sql design, we can not return ErrBadConn for the i/o error. (Otherwise, database/sql may execute non-idempotant queries again. It's very bad bug.)

The connection returning i/o error is in bad state. We can not reuse the connection.
That cause "err: invalid connection" log and ErrBadConn is returned.

I think this issue should be closed.

@kardianos kardianos closed this Oct 27, 2018
@golang golang locked and limited conversation to collaborators Oct 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.