Connection pool exhausted when connection failures occur, should refill with empty connections

[jlatherfold]

If preload_content is not specified on a request, connections are implicitly returned to the pool after the request is read. However, when a timeout error occurs or the connection is dropped the connection is closed but not returned in to the pool (since the response is None). This problem is compounded when retries are turned on since the next attempt will grab a new connection from the pool thus depleting it further. With non-blocking pools this is not a problem since we will create a new connection if the pool is empty but when the pool is blocking we have found that eventually the pool size becomes zero (after no.of connections timeout errors) which causes the calling application to hang on it's next request.
This can be fixed in connectionpool.urlopen in the exception handlers by explicitly returning the closed connection to the pool via _put_conn (if release_conn is false), since subsequent calls to _get_conn will check if the connection has been dropped and return a new one.

[Lukasa]

Hello again!

I think we need to be sure that we close the connection before we return them to the pool. Otherwise we run the risk of attempting to re-use a live connection that has timed out, which will end extremely poorly for us.

[shazow]

We should be putting a None into the pool if we want to discard the connection. That will be replaced with a fresh connection when we try to get it out of the pool. That does sound like a bug.

[Lukasa]

@shazow's idea is even better than mine.

[shazow]

@jlatherfold Is this something you'd be interested in working on?

Producing a failing test would be the first step. Make a ConnectionPool with a small pool (1-2), trigger some connection exceptions, see if the pool gets exhausted.

[jlatherfold]

Hi,

I've already done this, hence this bug report. Locally I've fixed it by putting the [closed] connection back on to the pool in the exception handlers (which do close the connections). Those Nones or closed connections are discarded and a new one is returned in subsequent calls to _get_conn. Sadly I don't know when I will get time to do any more on this right now.

[shazow]

@jlatherfold Would be handy if you can share your code for reproducing the issue. :)

Also if you can confirm that putting None rather than the closed connection still works as expected, that would be great.

[jlatherfold]

I can confirm that _put_conn(None) works as expected.

Here's some basic code, which also produces this error. Setting the read-timeout to a low value will cause read timeout exceptions. You'll need to hit a url with some large content - doesn't have to be massive - 500KB or so although you can probably replicate this with much less):

import os
import sys
import requests

def get(req, url):
    r = req.get(url, stream=True, timeout=(0.5, 0.1)) # set get timeout low to force timeout exceptions
    r.raise_for_status()

    return r.iter_content(chunk_size=4096)

if __name__ == '__main__':

    # if you need to auth the session do so....
    session = requests.Session()

    # setting the poolsize to 1 and 1 retries will result in this program
    # hanging on the first retry attempt (failed connection or read timeout)
    # set the pool_maxsize=4 and set some trace statements in connectionpool.py
    # to print the queue size in _get_conn and _put_conn and watch it slowly
    # decrease as we hit timeouts and retries.....
    adpater = requests.adapters.HTTPAdapter(pool_connections=1, pool_maxsize=1,
                                            pool_block=True, max_retries=1) 
    session.mount('http://', adpater)

    while True:

        try:
            print('request.get......')
            content = get(session, '<url to large content>')

            # consume the response
            for chunk in content:
                pass
        except Exception:
            print('ops.....')

        # after a while when you've hit few timeouts you're not going to get here....
        print('request.done......')

[jlatherfold]

Sorry - didn't know how to format for code - but you get the idea....

[Lukasa]

Updated code formatting.

[shazow]

Bonus points if somebody wants to translate that to plain-urllib3 and double-bonus if you make it into a test. :)

[jlatherfold]

Ok, I'll make the patch myself. Please let me know what procedure I need to follow or point me in the right direction where I can get that info as I have not made any code contributions before.

[shazow]

@jlatherfold Thanks! Fork the project, make a branch, start with a test that shows the failure (we have full test coverage, have a look at other tests for examples), then send a PR for feedback. We'll review the code and give you more pointers.

If you can think of a way to reproduce the scenario without doing a full end-to-end request, then urllib/tests/test_connectionpool.py would be a good place to put it. If you need a server, then take a look at the with_dummyserver subdirectory of suites. We lately prefer the test_socketlevel.py method of testing with a dummyserver when possible.

Might be wise to just write a simple example of the scenario reproducing in urllib3 first, to make sure it's not requests' fault.

[jlatherfold]

Hi,

I now have a fix for this with unit test and am ready to submit a PR. The work has been done on a local branch. Would you like me to merge it to master or push the branch to my fork and use that?

[shazow]

@jlatherfold Whatever you prefer, as long as it ends up as a Pull Request on our end. :)

[FrankSalad]

I think we're experiencing an issue related to this when communicating over HTTPS. Using urllib version 1.25.3 I used @jlatherfold's proof of concept and https://beeceptor.com/ to create an https endpoint with an artificial 10 second timeout (passed into the example here as url). The POC hangs after logging request.get 26 times.

import os
import sys
import requests

def get(req, url):
    r = req.get(url, stream=True, timeout=(0.5, 0.1)) # set get timeout low to force timeout exceptions
    r.raise_for_status()

    return r.iter_content(chunk_size=4096)

if __name__ == '__main__':

    # if you need to auth the session do so....
    session = requests.Session()

    # setting the poolsize to 1 and 1 retries will result in this program
    # hanging on the first retry attempt (failed connection or read timeout)
    # set the pool_maxsize=4 and set some trace statements in connectionpool.py
    # to print the queue size in _get_conn and _put_conn and watch it slowly
    # decrease as we hit timeouts and retries.....
    adpater = requests.adapters.HTTPAdapter(pool_connections=1, pool_maxsize=1,
                                            pool_block=True, max_retries=1) 
    session.mount('https://', adpater)

    while True:

        try:
            print('request.get......')
            content = get(session, url)

            # consume the response
            for chunk in content:
                pass
        except Exception:
            print('ops.....')

        # after a while when you've hit few timeouts you're not going to get here....
        print('request.done......')