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

fix #141 #147

Closed
wants to merge 3 commits into from
Closed

fix #141 #147

wants to merge 3 commits into from

Conversation

goldwind-ting
Copy link

Is it reasonable to check the connection in build? @djc

@djc
Copy link
Owner

djc commented Jan 5, 2023

This seems more like a workaround than a fix for the issue described in #141 -- AFAICT in this case the connection would still time out, but it would happen at pool instantiation time rather than at pool.get() time?

@goldwind-ting
Copy link
Author

goldwind-ting commented Jan 6, 2023

This seems more like a workaround than a fix for the issue described in #141 -- AFAICT in this case the connection would still time out, but it would happen at pool instantiation time rather than at pool.get() time?

Yes, it would still time out, but I think in this case it is caused possibly by network.
If merged the PR, it will return directly when failed to check configure, e.g. password, database name.
And if you think this is not reasonable, I will try another solution.

@djc
Copy link
Owner

djc commented Jan 6, 2023

Yes, I don't think this is a good solution. I'd like to understand why we're timing out in that case instead of immediately-ish getting an error about the password being wrong.

@goldwind-ting
Copy link
Author

Yes, I don't think this is a good solution. I'd like to understand why we're timing out in that case instead of immediately-ish getting an error about the password being wrong.

Seem that when failed to connect database, it would loop until timeout and do not send guard to waiters channel.

@djc
Copy link
Owner

djc commented Jan 9, 2023

Right, so I think the correct fix would be to check the error returned by connect() in that case and avoid looping if we can determine the problem is likely to be permanent.

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.

None yet

2 participants