Skip broken pipe errors #1524

robermorales · 2019-01-11T20:51:15Z

In the case the server legitimally close a connection but we are still
sending data (e.g. sending a POST request), the old code prevents the
response to be retrieved.

RFC2616 refers different scenarios where a server can legitimately close the connection.

https://tools.ietf.org/html/rfc2616#section-8.1.2 (8.1.2.1)
https://tools.ietf.org/html/rfc2616#section-10.4.14

With this change, we avoid a broken pipe in the request write process,
to end the read process.

I tested curl behavior when presented to this exact scenario and it is also ignoring the broken pipes and continue parsing the legitimate response.

This behavior often get reached when using urllib3 through requests, so the related issues are from it:

Related issues:

psf/requests#4778
psf/requests#2422

codecov-io · 2019-01-11T21:23:36Z

Codecov Report

Merging #1524 into master will decrease coverage by 0.45%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1524      +/-   ##
==========================================
- Coverage   99.81%   99.35%   -0.46%     
==========================================
  Files          24       24              
  Lines        2183     2187       +4     
==========================================
- Hits         2179     2173       -6     
- Misses          4       14      +10

Impacted Files	Coverage Δ
src/urllib3/connection.py	`100.00% <ø> (ø)`
src/urllib3/connectionpool.py	`100.00% <100.00%> (ø)`
src/urllib3/contrib/_appengine_environ.py	`90.00% <0.00%> (-10.00%)`	⬇️
src/urllib3/util/wait.py	`92.18% <0.00%> (-7.82%)`	⬇️
src/urllib3/util/ssl_.py	`97.77% <0.00%> (-2.23%)`	⬇️
src/urllib3/response.py	`99.45% <0.00%> (-0.55%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3472ec...2335967. Read the comment docs.

sethmlarson

Thanks for opening this! Could we add a test that fails with the current master but passes with this branch?

src/urllib3/connectionpool.py

robermorales · 2019-01-14T13:57:16Z

I made the changes you suggested. I cannot add the test right now, but will try add it and commit to the branch soon.

Thanks,

sethmlarson

Looks like there's a lint error on Travis, could you take a look at this?

I need to try running that testcase against an earlier version of urllib3 to verify it works correctly.

test/with_dummyserver/test_connectionpool.py

sethmlarson

One more comment then this looks good for a final review.

test/with_dummyserver/test_connectionpool.py

sethmlarson · 2019-01-20T01:24:24Z

So I ran this code with the old urllib3 without the exception handling and it passed fine, are we sure we're triggering the pipe error with that testcase?

robermorales · 2019-01-21T09:13:24Z

So I ran this code with the old urllib3 without the exception handling and it passed fine, are we sure we're triggering the pipe error with that testcase?

@sethmlarson I tried to reproduce a test case that fails with the old implementation and passes with the new one. I coudn't mimic a broken pipe in the test application, but I found a test that passes only with the change. What do you think that could be a good way to mimic the behaviour that we are covering? I am open to implement a new test that better covers it.

sethmlarson · 2019-01-21T14:42:27Z

I'd definitely like to trigger a pipe error as least, maybe we need a lower-level interface than what Tornado is giving us to call a socket.close() before reading the body? Try looking in test/with_dummyserver/test_socketlevel.py, maybe we move this testcase to that file instead?

robermorales · 2019-01-21T16:19:38Z

@sethmlarson cool idea! I will return with some implementation. Thanks for all your help!

sethmlarson · 2019-01-21T16:21:27Z

Of course! :) Thank you for all your effort tackling this issue!

bbbco · 2019-03-01T20:01:51Z

Any updates on this?

sethmlarson · 2019-03-01T20:16:30Z

Nope, the original author appears to be MIA.

robermorales · 2019-03-05T20:15:08Z

Sorry for the delay, I will return to the development of this soon. Thanks!

In the case the server legitimally close a connection but we are still sending data (e.g. sending a POST request), the old code prevents the response to be retrieved. With this change, we avoid a broken pipe in the request write process, to end the read process.

Added a comment with the rationale.

EmptyRabbit · 2019-12-23T10:33:17Z

when could this pr merge...... I blocked by this cause...

sethmlarson · 2019-12-25T02:20:58Z

I'll be taking a look at this again after the holidays.

sethmlarson · 2020-04-23T12:59:51Z

@hodbn An interesting article about the error you're running into: http://erickt.github.io/blog/2014/11/19/adventures-in-debugging-a-potential-osx-kernel-bug/

When the server closed the socket but the client still tries to read the response, macOS can fail with EPROTOTYPE instead of simply EPIPE or ESHUTDOWN, so BrokenPipeError isn't enough. I also had to fix the test, otherwise the request would fail because it didn't read the whole response (which is still a healthy thing to do).

sethmlarson

Just one nit then this LGTM

src/urllib3/connectionpool.py

sethmlarson · 2020-08-28T15:19:29Z

Sorry this stalled out, that's my fault. Trying to get it landed for v1.26.

Pulled this down locally and verified the current set of changes works nicely on Linux.
For Windows I added 10053 which is "Connection Aborted Error" which can happen for multiple reasons it seems (antivirus, firewall, and your peer closing the connection) so hopefully that'll fix up the issue we were seeing on Windows.

sethmlarson · 2020-08-28T18:06:23Z

I'm not happy to skip the test on Windows but I'm unsure if there's anything we can do? Windows seems to be erroring out consistently on sock.recv() once the socket is closed so we can't call conn.getresponse() and get anything meaningful.

Have made some changes myself

pquentin

Thanks!

We'll have to merge from master again to make sure we don't have two "notWindows" decorators

…-pipe-detection

sethmlarson · 2020-09-11T14:14:50Z

Thanks @pquentin and @hodbn for the reviews and @robermorales for putting together this change. This is a bigger fix than you think it is, many users are going to see real improvements to their network and API performance. 🎉

bessbd · 2020-10-29T13:50:02Z

Hi @sethmlarson ,

When do you think this fix would be included in an upcoming release?
Is there something I could do to speed that process up? Should I open a PR that backports this change to any branch?

Thank you,
Donat

sethmlarson · 2020-10-29T14:45:10Z

@bessbd This change won't be backported since it's not a bug / security fix. Not much to speed things up, we're wrapping up the last things that need to be done for 1.26.

The 1.26 release is special since it's the last minor before v2 so we're trying to wrap it up a lot more "completely" compared to a typical minor release.

We had an xfailing test that reproduced a case where Alternator tried to report an error when the request was too long, but the boto library didn't see this error and threw a "Broken Pipe" error instead. It turns out that this wasn't a Scylla bug but rather a bug in urllib3, which overzealously reported a "Broken Pipe" instead of trying to read the server's response. It turns out this issue was already fixed in urllib3/urllib3#1524 and now, on modern installations, the test that used to fail now passes and reports "XPASS". So in this patch we remove the "xfail" tag, and skip the test if running an old version of urllib3. Fixes scylladb#8195 Signed-off-by: Nadav Har'El <nyh@scylladb.com> x

We had an xfailing test that reproduced a case where Alternator tried to report an error when the request was too long, but the boto library didn't see this error and threw a "Broken Pipe" error instead. It turns out that this wasn't a Scylla bug but rather a bug in urllib3, which overzealously reported a "Broken Pipe" instead of trying to read the server's response. It turns out this issue was already fixed in urllib3/urllib3#1524 and now, on modern installations, the test that used to fail now passes and reports "XPASS". So in this patch we remove the "xfail" tag, and skip the test if running an old version of urllib3. Fixes scylladb#8195

We had an xfailing test that reproduced a case where Alternator tried to report an error when the request was too long, but the boto library didn't see this error and threw a "Broken Pipe" error instead. It turns out that this wasn't a Scylla bug but rather a bug in urllib3, which overzealously reported a "Broken Pipe" instead of trying to read the server's response. It turns out this issue was already fixed in urllib3/urllib3#1524 and now, on modern installations, the test that used to fail now passes and reports "XPASS". So in this patch we remove the "xfail" tag, and skip the test if running an old version of urllib3. Fixes #8195 Closes #12038

sethmlarson requested changes Jan 11, 2019

View reviewed changes

src/urllib3/connectionpool.py Outdated Show resolved Hide resolved

src/urllib3/connectionpool.py Outdated Show resolved Hide resolved

src/urllib3/connectionpool.py Show resolved Hide resolved

sethmlarson requested changes Jan 14, 2019

View reviewed changes

test/with_dummyserver/test_connectionpool.py Outdated Show resolved Hide resolved

sethmlarson requested changes Jan 15, 2019

View reviewed changes

test/with_dummyserver/test_connectionpool.py Outdated Show resolved Hide resolved

robermorales and others added 6 commits May 24, 2019 11:12

Included request_chunked() in the control.

d4416f0

Added a comment with the rationale.

Added tests

3a285b9

Added tests for the request_chunked() case

5e68837

ignoring chunked test for Google App Engine

9a484d3

Skip on is_appengine()

3e18f38

robermorales force-pushed the broken-pipe-detection branch from f8eb680 to 3e18f38 Compare May 24, 2019 09:12

Gerdie mentioned this pull request Jan 16, 2020

Tilesets cli aborts connection with api on large source data uploads mapbox/tilesets-cli#31

Closed

hodbn added 4 commits April 22, 2020 07:53

merge new master

ffeeb0e

Merge branch 'master' into broken-pipe-detection

70b1f0c

rewrite broken pipe tests

5750179

account for EPROTOTYPE on osx

567a8f4

sethmlarson previously requested changes Apr 28, 2020

View reviewed changes

src/urllib3/connectionpool.py Outdated Show resolved Hide resolved

sethmlarson added this to the v1.26 milestone Aug 27, 2020

Skip BrokenPipeError test on Windows

c6ae228

sethmlarson force-pushed the broken-pipe-detection branch from c9497d0 to c6ae228 Compare August 28, 2020 18:11

sethmlarson requested a review from pquentin August 28, 2020 19:14

Merge branch 'master' into broken-pipe-detection

7450d79

sethmlarson requested a review from hodbn September 4, 2020 01:03

hodbn previously approved these changes Sep 10, 2020

View reviewed changes

pquentin previously approved these changes Sep 11, 2020

View reviewed changes

Merge branch 'master' of ssh://github.com/urllib3/urllib3 into broken…

d4d97de

…-pipe-detection

sethmlarson dismissed stale reviews from pquentin and hodbn via 7d47b1c September 11, 2020 13:58

De-duplicate notWindows decorator

2335967

sethmlarson force-pushed the broken-pipe-detection branch from 7d47b1c to 2335967 Compare September 11, 2020 14:01

sethmlarson merged commit e4c43f5 into urllib3:master Sep 11, 2020

bessbd mentioned this pull request Nov 9, 2020

Bump requests version once https://github.com/urllib3/urllib3/pull/1524 is released IBM/cloudant-python-sdk#20

Closed

JuanGarcia345 mentioned this pull request Dec 15, 2020

Connection is reset when redirecting a large request to Flask app benoitc/gunicorn#2474

Closed

adamtheturtle mentioned this pull request Mar 23, 2021

tests/mock_vws/test_query.py::TestMaximumImageFileSize::test_png[Real Vuforia] FAILED VWS-Python/vws-python-mock#837

Closed

adamtheturtle mentioned this pull request Sep 3, 2022

SSLEOFError in Python 3.10 not in Python 3.9 #2733

Open

nyh mentioned this pull request Nov 20, 2022

Alternator's response to oversized requests confuses the client drivers scylladb/scylladb#8195

Closed

nyh mentioned this pull request Nov 20, 2022

test/alternator: un-xfail a test which passes on modern Python scylladb/scylladb#12038

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip broken pipe errors #1524

Skip broken pipe errors #1524

robermorales commented Jan 11, 2019

codecov-io commented Jan 11, 2019 •

edited by codecov bot

sethmlarson left a comment

robermorales commented Jan 14, 2019

sethmlarson left a comment

sethmlarson left a comment •

edited

sethmlarson commented Jan 20, 2019

robermorales commented Jan 21, 2019

sethmlarson commented Jan 21, 2019

robermorales commented Jan 21, 2019

sethmlarson commented Jan 21, 2019

bbbco commented Mar 1, 2019

sethmlarson commented Mar 1, 2019

robermorales commented Mar 5, 2019

EmptyRabbit commented Dec 23, 2019

sethmlarson commented Dec 25, 2019

sethmlarson commented Apr 23, 2020

sethmlarson left a comment

sethmlarson commented Aug 28, 2020

sethmlarson commented Aug 28, 2020 •

edited

pquentin left a comment

sethmlarson commented Sep 11, 2020

bessbd commented Oct 29, 2020

sethmlarson commented Oct 29, 2020 •

edited

Skip broken pipe errors #1524

Skip broken pipe errors #1524

Conversation

robermorales commented Jan 11, 2019

codecov-io commented Jan 11, 2019 • edited by codecov bot

Codecov Report

sethmlarson left a comment

Choose a reason for hiding this comment

robermorales commented Jan 14, 2019

sethmlarson left a comment

Choose a reason for hiding this comment

sethmlarson left a comment • edited

Choose a reason for hiding this comment

sethmlarson commented Jan 20, 2019

robermorales commented Jan 21, 2019

sethmlarson commented Jan 21, 2019

robermorales commented Jan 21, 2019

sethmlarson commented Jan 21, 2019

bbbco commented Mar 1, 2019

sethmlarson commented Mar 1, 2019

robermorales commented Mar 5, 2019

EmptyRabbit commented Dec 23, 2019

sethmlarson commented Dec 25, 2019

sethmlarson commented Apr 23, 2020

sethmlarson left a comment

Choose a reason for hiding this comment

sethmlarson commented Aug 28, 2020

sethmlarson commented Aug 28, 2020 • edited

pquentin left a comment

Choose a reason for hiding this comment

sethmlarson commented Sep 11, 2020

bessbd commented Oct 29, 2020

sethmlarson commented Oct 29, 2020 • edited

codecov-io commented Jan 11, 2019 •

edited by codecov bot

sethmlarson left a comment •

edited

sethmlarson commented Aug 28, 2020 •

edited

sethmlarson commented Oct 29, 2020 •

edited