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: conn(cachedOrNewConn) errs unnecessarily when maxLifetime is set #17000

Closed
mikewilliamson opened this issue Sep 6, 2016 · 2 comments
Assignees
Milestone

Comments

@mikewilliamson
Copy link

@mikewilliamson mikewilliamson commented Sep 6, 2016

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

1.6

What operating system and processor architecture are you using (go env)?

Linux x86

What did you do?

Set db connections to expire:

db.SetConnMaxLifetime(5 * time.Minute)

Create some connections, use them, and occasionally call db.Ping().

What did you expect to see?

Ping returning nil.

What did you see instead?

Ping returning ErrBadConn.

There are two problems here. The major problem is that conn() with strategy cachedOrNewConn isn't very tolerant of expired connections in the pool, and returns an error if the first connection happens to be expired. I think conn() should loop through all connections in the pool looking for an unexpired one, and then fall back to creating a new one if there are no unexpired ones available.

The secondary problem is that Ping() isn't very tolerant of conn(cachedOrNewConn) failures. Most other functions that call this (e.g. Prepare, Exec, Query) do so in a loop for maxBadConnRetries, but Ping doesn't have this retry logic. Therefore users of Ping() are more susceptible to spurious errors.

@quentinmit quentinmit added this to the Go1.8 milestone Sep 6, 2016
@quentinmit quentinmit added the NeedsFix label Oct 6, 2016
@rsc rsc modified the milestones: Go1.9, Go1.8 Nov 11, 2016
@kardianos kardianos self-assigned this Nov 28, 2016
@kardianos
Copy link
Contributor

@kardianos kardianos commented Nov 28, 2016

The second problem has actually been taken care of. Ping now will retry just like the other methods do.

I agree the cache strategy could be better.

@kardianos
Copy link
Contributor

@kardianos kardianos commented Mar 30, 2017

Go1.8 has been released now. Please refer to the code in question now looks like:

https://go.googlesource.com/go/+/bc972e8ef870471c3c4ba95b92c5194b37ba2871/src/database/sql/sql.go#588

This issue is at least partially solved. If you think there are further issues here, please open a new issue and refer to me in it. Thanks.

@kardianos kardianos closed this Mar 30, 2017
@golang golang locked and limited conversation to collaborators Mar 30, 2018
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
5 participants
You can’t perform that action at this time.