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

Hang inside call to PoolManager request #1817

Merged
merged 12 commits into from
Apr 3, 2020
6 changes: 5 additions & 1 deletion dummyserver/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,11 @@ def _call_method(self):

def index(self, _request):
"Render simple message"
return Response("Dummy server!")
data = "Dummy server!"
if "page" in _request.arguments:
page = _request.arguments["page"][0]
data += " page %s" % page.decode()
return Response(data)

def certificate(self, request):
"""Return the requester's certificate."""
Expand Down
33 changes: 5 additions & 28 deletions src/urllib3/connectionpool.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
BaseSSLError,
)
from .request import RequestMethods
from .response import HTTPResponse
from .response import HTTPResponse, drain_conn

from .util.connection import is_connection_dropped
from .util.request import set_file_position
Expand Down Expand Up @@ -765,21 +765,6 @@ def urlopen(
**response_kw
)

def drain_and_release_conn(response):
try:
# discard any remaining response body, the connection will be
# released back to the pool once the entire response is read
response.read()
except (
TimeoutError,
HTTPException,
SocketError,
ProtocolError,
BaseSSLError,
SSLError,
):
pass

# Handle redirect?
redirect_location = redirect and response.get_redirect_location()
if redirect_location:
Expand All @@ -790,15 +775,11 @@ def drain_and_release_conn(response):
retries = retries.increment(method, url, response=response, _pool=self)
except MaxRetryError:
if retries.raise_on_redirect:
# Drain and release the connection for this response, since
# we're not returning it to be released manually.
drain_and_release_conn(response)
drain_conn(response)
raise
return response

# drain and return the connection to the pool before recursing
drain_and_release_conn(response)

drain_conn(response)
retries.sleep_for_retry(response)
log.debug("Redirecting %s -> %s", url, redirect_location)
return self.urlopen(
Expand All @@ -824,15 +805,11 @@ def drain_and_release_conn(response):
retries = retries.increment(method, url, response=response, _pool=self)
except MaxRetryError:
if retries.raise_on_status:
# Drain and release the connection for this response, since
# we're not returning it to be released manually.
drain_and_release_conn(response)
drain_conn(response)
raise
return response

# drain and return the connection to the pool before recursing
drain_and_release_conn(response)

drain_conn(response)
retries.sleep(response)
log.debug("Retry: %s", url)
return self.urlopen(
Expand Down
5 changes: 5 additions & 0 deletions src/urllib3/poolmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from ._collections import RecentlyUsedContainer
from .connectionpool import HTTPConnectionPool, HTTPSConnectionPool
from .connectionpool import port_by_scheme

from .exceptions import (
HTTPWarning,
LocationValueError,
Expand All @@ -17,6 +18,7 @@
from .packages import six
from .packages.six.moves.urllib.parse import urljoin
from .request import RequestMethods
from .response import drain_conn
from .util.url import parse_url
from .util.retry import Retry

Expand Down Expand Up @@ -384,13 +386,16 @@ def urlopen(self, method, url, redirect=True, **kw):
retries = retries.increment(method, url, response=response, _pool=conn)
except MaxRetryError:
if retries.raise_on_redirect:
drain_conn(response)
raise
return response

kw["retries"] = retries
kw["redirect"] = redirect

log.info("Redirecting %s -> %s", url, redirect_location)

drain_conn(response)
return self.urlopen(method, redirect_location, **kw)


Expand Down
12 changes: 12 additions & 0 deletions src/urllib3/response.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
ResponseNotChunked,
IncompleteRead,
InvalidHeader,
HTTPError,
)
from .packages.six import string_types as basestring, PY3
from .packages.six.moves import http_client as httplib
Expand All @@ -29,6 +30,17 @@
log = logging.getLogger(__name__)


def drain_conn(response):
Copy link
Member

Choose a reason for hiding this comment

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

(For what it's worth, I now believe having drain_conn inside the response class is perfectly fine, I just needed a better name, and we now have that. But keeping the function outside of the class is fine too.)

"""
Read and discard any remaining HTTP response data from the connection.
Unread data in the HTTPResponse connection blocks the connection from being released back to the pool
"""
try:
response.read()
except (HTTPError, SocketError, BaseSSLError, HTTPException):
pass


class DeflateDecoder(object):
def __init__(self):
self._first_try = True
Expand Down
21 changes: 21 additions & 0 deletions test/with_dummyserver/test_poolmanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,27 @@ def setup_class(cls):
cls.base_url = "http://%s:%d" % (cls.host, cls.port)
cls.base_url_alt = "http://%s:%d" % (cls.host_alt, cls.port)

def test_redirect_preload(self):
def build_request(number, preload):
return http.request(
"GET",
"%s/redirect" % self.base_url,
fields={"target": "%s/?page=%d" % (self.base_url, number)},
preload_content=preload,
)

with PoolManager(block=True, maxsize=3) as http:
for j in range(1, 5):
r1 = build_request(number=j, preload=True)
r2 = build_request(number=j, preload=False)

data = b"Dummy server! page %d" % j
assert r1.status == 200
assert r2.status == 200
assert r1.data == data
assert r2.data == data
r2.release_conn() # Only need to release for preload=False

def test_redirect(self):
with PoolManager() as http:
r = http.request(
Expand Down