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
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@treeinlake
Copy link

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

/cc @kardianos

@kardianos
Copy link
Contributor

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 commented Aug 27, 2018

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

@kardianos
Copy link
Contributor

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?

@dolmen
Copy link
Contributor

dolmen commented Aug 27, 2018

@treeinlake
Copy link
Author

  • @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

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 30, 2018
@FiloSottile FiloSottile added this to the Unplanned milestone Aug 30, 2018
@methane
Copy link
Contributor

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.

@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.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants