Skip to content

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

Open
smurfix wants to merge 2 commits intohttplib2:masterfrom
smurfix:upstream
Open

Reading the actual request may trigger CONNRESET on a cached connection#62
smurfix wants to merge 2 commits intohttplib2:masterfrom
smurfix:upstream

Conversation

@smurfix
Copy link
Copy Markdown

@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)

smurfix added 2 commits July 13, 2017 12:26
(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
Copy Markdown
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
Copy Markdown

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.

3 participants