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

Reading the actual request may trigger CONNRESET on a cached connection #62

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

Conversation

smurfix
Copy link

@smurfix smurfix commented Jul 13, 2017

This moves the block that reads the data up into the try: block.

I hit this issue while testing aioamqp with pyrabbit. The sequence of events is:

  • send request without auth header
  • get an error
  • … meanwhile, the server starts thinking about what you just did
    (like doing a DNS lookup for its error log)
  • re-send the request with auth header
  • … the server gets around to deciding that it wants to close your connection, and does so
  • get an ECONNRESET error reading the reply. :-(

This can also happen when you're re-using a cached connection after some time and hit the server's decision that it has waited long enough and will now close the connection.

Thus, please apply.

(This is a copy/rebase of jcgregorio/httplib2#320)

(if the connection was cached)

thus move that code inside the try:.
Handle ECONNRESET and EPIPE correctly

and simplify the code a bit (pull up conn.close()
while we're at it
@temoto
Copy link
Member

temoto commented Aug 16, 2017

Thank you very much. Patch seems fine.

Dear users and @httplib2/maintainers is it possible to give this one a bit of field testing? To compensate automated tests.

pip install https://github.com/httplib2/httplib2/archive/b1b47957f4a3402392bd9897ca15d8566d0d614d.zip

@temoto temoto force-pushed the master branch 7 times, most recently from 2a3a5f5 to 70fb0c8 Compare March 30, 2018 02:22
@weyou
Copy link

weyou commented Mar 16, 2021

Suggest adding the connection retry for errno.ECONNABORTED.

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

3 participants