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

Respect timeouts specified per query #172

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Respect timeouts specified per query #172

wants to merge 2 commits into from

Conversation

hogjosh
Copy link
Contributor

@hogjosh hogjosh commented Feb 22, 2017

Intended to fix #162, but I might be barking up the wrong tree! Maybe I can at least get the ball rolling...

@coveralls
Copy link

coveralls commented Feb 22, 2017

Coverage Status

Coverage increased (+0.05%) to 66.104% when pulling 36e8734 on hogjosh:respect_query_timeouts into 452a42d on xerions:master.

Copy link
Contributor

@fishcakez fishcakez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good except the set charset query will have :infinity timeout during handshake and we don't want to allow that.

In postgrex this is handled by always using an :infinity timeout and then doing this: https://github.com/elixir-ecto/postgrex/blob/2c026cb29c4f8e62ce1b2fec46f444fac440ff4d/lib/postgrex/protocol.ex#L487-L510

@coveralls
Copy link

coveralls commented Jun 14, 2017

Coverage Status

Coverage increased (+0.05%) to 66.211% when pulling 9a10be9 on hogjosh:respect_query_timeouts into 2972cb7 on xerions:master.

@hogjosh
Copy link
Contributor Author

hogjosh commented Jun 14, 2017

I fumbled through this. I'm happy to try to make additional changes.

If I'm understanding correctly, there's a recursive call to handshake_recv/2 and I wasn't sure if the timeout should be for the duration of the entire handshake or per call to handshake_recv/2

I also have no idea if the "shutdown" processing is what we had in mind.

@fishcakez
Copy link
Contributor

I also have no idea if the "shutdown" processing is what we had in mind.

I think we can close socket, and let existing code handle socket closing error.

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.

per query timeouts not respected
3 participants