- Notifications
You must be signed in to change notification settings - Fork905
fix(cli): fix flakes related to context cancellation when establishing pg connections#18246
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation
cli/clitest/clitest.go Outdated
| // When a test exits it cancels the context. If a transaction is open | ||
| // and a query is being executed, instead of a context.Canceled error | ||
| // we get a "driver: bad connection" error. | ||
| return |
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.
Wouldn't exiting with an open transaction be bugged behaviour? Could this mask someone forgetting to close a connection?
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.
Hmm, I should probably make the comment clearer. It should be
When we cancel the context on a query that's being executed within a transaction, sometimes, instead of a context.Canceled error we get a "driver: bad connection" error.
I don't think this could mask someone forgetting to close a connection. I suppose it might mask someone closing a connection while a transaction is being executed? I'm not sure how the database would handle this scenario.
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.
seems to also be reported here:lib/pq#1137
cf0daf4 to3864e86Compare623dcd9 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Since#18195 was merged, we started running CLI tests with postgres instead of just dbmem. This surfaced errors related to context cancellation while establishing postgres connections.
This PR shouldfixcoder/internal#672. Related to#15109.