PoolManager is not thread-safe

[reversefold]

See, for reference this issue on the requests library:
psf/requests#1871

In looking into the issue, which appears to happen when too many hosts are connected to through the same PoolManager in parallel, the issue appears to be with PoolManager's use of an LRU cache for the connection pool instances within it. A pool is pulled from the cache but within the time it takes for pool._get_conn to be called the PoolManager can potentially evict the ConnectionPool if too many other hosts are connected to in parallel.

In addition, since the pool also handles retries and redirects within urlopen, the pool needs to stay alive until the final connection is acquired from the pool, meaning we can't be sure we can release the pool from PoolManager until urlopen returns, essentially making PoolManager make requests serially only.

I only see a few options for fixing this.

1. Remove the LRU cache. This means either connection leaks or hard-limiting the number of hosts that a PoolManager can connect to. Not a very useful option.
2. Put the call(s) to pool.urlopen within a critical section. Basically a no-go as this means that concurrency is useless.
3. Implement ref-counting (a semaphore of some kind) to keep track of pools actually in use and don't allow them to be reaped if they have a reference. This seems complicated and a duplication of Python's own GC. Perhaps this could be implemented by using del to close the pool and not explicitly closing the pool when it is evicted?
4. Refactor PoolManager and the connection pools such that, when using a PoolManager, retries are handled in the PoolManager and we can get up to the point of getting an actual connection out of the pool within a critical section then release the pool to be potentially evicted.

[Lukasa]

Yes, this is a known issue. An abortive attempt to resolve it was made in #1237 but was eventually abandoned when the scope of the change came into the open. As I noted in #1232, the window of this concurrency problem is very small and in reality I think it can be fairly easily managed from outside urllib3, so it's not high up my priority list. However, if anyone else wanted to tackle this we'd happily accept patches to resolve it.

[reversefold]

@Lukasa I think that the risk is a little higher than previously thought. Since HTTPConnectionPool.urlopen can end up recursing, the same pool can be used after network IO, potentially multiple times. This is a much larger window than from urlopen being invoked to the first _get_conn call as it involves actual network IO.

[Lukasa]

The recursion is moderately risky, but in practice I'm still not convinced that the risk is that high. In the absence of data suggesting otherwise it's still unlikely to become a high priority for me.

[reversefold]

Only after I had done most of the initial work to potentially fix this issue did I realize that PoolManager forces redirect=False on the urlopen call to HTTPConnectionPool. This reduces the risk to the time period between the pool being acquired in PoolManager and the _get_conn call in HTTPConnectionPool (very small) and the case where a Retry-After header is returned. If the Retry-After logic was also moved to PoolManager the risk would be nearly none.

That said, I have a fairly trivial test which shows the ClosedPoolError happening and I am preparing a PR which should fix all of the potential race conditions. The implementation is a little clunky as I was working to change the underlying logic and code as little as possible in the first iteration to keep the current behavior and functionality intact.

[marcobazzani]

I think there are other race conditions when I do multithread calls with Python requests I get a ClosedPoolError and had to patch to return a new connection

#951 (comment)

[hannes-ucsc]

The recursion is moderately risky, but in practice I'm still not convinced that the risk is that high. In the absence of data suggesting otherwise it's still unlikely to become a high priority for me.

This statement, without evidence, dismisses the probability of a proven reproduction as being below some magical, made-up threshold but asks for more evidence to suggest that it is above. Let me ask, what good is a connection pool if I can't be sure it can be shared by multiple threads?

[sigmavirus24]

@hannes-ucsc you're asking a former maintainer to explain their time-management and prioritization explanation from 4 years ago in a confrontational way. I'm doing my best to assume you did so with the intent of provoking a productive conversation. Also, as I understand it, the discussion about recursion that you're responding to was rendered moot by the follow-up comment from OP. In other words, not only are you bringing the conversation down (as far as I'm concerned) but you're responding to a comment that OP agreed was correct (there is no recursion problem here).

As a result, I'll pose a separate question to you: In the experiences of some of the maintainers, urllib3 appears thread-safe and appears to do the right things as necessary.

I would posit a more productive tact would be to pick up the stale PRs, revive and rebase them and get them merged. Maintainers oft don't have time to treat every bug report that arises equally. If we investigated every report with equal vigor, we wouldn't have time for everything else.

[hannes-ucsc]

you're asking a former maintainer to explain their time-management and prioritization explanation

That is incorrect. There is only one question in my post and it does not relate to time-management or prioritization.

in a confrontational way

I don't follow. Maybe our definitions of the term confrontation differs.

Also, as I understand it, the discussion about recursion that you're responding to was rendered moot by the follow-up comment from OP. In other words, not only are you bringing the conversation down (as far as I'm concerned) but you're responding to a comment that OP agreed was correct (there is no recursion problem here).

I disagree. As far as I interpret the OP, the risk is reduced but not eliminated.

As a result, I'll pose a separate question to you: In the experiences of some of the maintainers, urllib3 appears thread-safe and appears to do the right things as necessary.

What is the question?

Anyways, when thread-safety and correctness are expressed in terms of appearance and experience, as opposed to evidence, I start to worry.

I would posit a more productive tact would be to pick up the stale PRs, revive and rebase them and get them merged. Maintainers oft don't have time to treat every bug report that arises equally. If we investigated every report with equal vigor, we wouldn't have time for everything else.

I'm not asking anyone to investigate every bug report with equal vigor. I pointed out that the standards by which they are evaluated appear to be arbitrary. To confirm, are you proposing that I invest time into an issue that was closed by a maintainer, as, how I interpret it, not worth pursuing?

I know thread safety is hard, believe me, but when credible reports about issues with thread safety are dismissed in this way, the issue is not so much with the correctness of the software itself, but with epistemological approach.

[sigmavirus24]

@hannes-ucsc You quoted this comment:

The recursion is moderately risky, but in practice I'm still not convinced that the risk is that high. In the absence of data suggesting otherwise it's still unlikely to become a high priority for me.

This comment communicates:

• an understanding/estimate of risk (based on considerable experience and understanding of the project) around recursion in the redirect handling case (which was later dismissed by OP because there is no automatic recursion handling by default)
• an absence of data or reproducible cases suggesting a higher risk beyond "I saw this"
• without such a reproducible case, a communication of individual prioritization of their time

You then say that it

dismisses the probability of a proven reproduction as being below some magical, made-up threshold but asks for more evidence to suggest that it is above.

Which isn't in fact what it says. Using the phrase "magical, made-up threshold" is a bad-faith interpretation of what was said that appears to be intended to evoke a response which is - perhaps not confrontational - but definitely unproductive as it would absolutely default any reasonable person to being on the defensive thus provoking a confrontation.

the risk is reduced but not eliminated.

Correct and that's what the post you quoted was discussing - the risk.

What is the question?

I've forgotten 😆

Anyways, when thread-safety and correctness are expressed in terms of appearance and experience, as opposed to evidence, I start to worry.

When no evidence is given to the contrary beyond experiences, I start to question how much of my limited time and effort I must really put in to reproduce something without a reproducible test case.

To confirm, are you proposing that I invest time into an issue that was closed by a maintainer, as, how I interpret it, not worth pursuing?

I'm proposing that you attempt to take a tact that is not that of provocateur but instead "I've experienced this and been able to reproduce in such a way. I tested the proposed solution in PR #1257 and it fixes the issue for me. How can we re-open this issue and that PR and get solve this?"

Your interpretation, in my opinion, is once again assuming bad faith on the part of a different maintainer from the one you quote. The issue was closed due to a lack of activity for years because it has no reproducible test case.

I know thread safety is hard, believe me, but when credible reports about issues with thread safety are dismissed in this way

This is the first sign of good faith you've shown in your comments followed immediately by a bad faith assumption of the "dismissal" which isn't what happened. In fact, the former maintainer you quote says "Yes, we've had reports of this" and acknowledges the likelihood of a problem. What you interpret as a dismissal of the problem is in fact a ranking of priority given the understood impact it has on users. Your interpretations reveal that your assumptions here are automatically negative towards any statement from any maintainer - past or present. Your predisposition has biased you against looking for a productive outcome because you've assumed there isn't one. I've pointed you in the direction of what productive might look like and you've now turned the discussion from one of fixing a thread safety bug to assailing the maintainers for making decisions about what they choose to work on.

I won't try to hide behind a veil of superiority in saying that - Your presence thus far has not demonstrated a desire to fix this bug or see it fixed but instead to provoke responses from (in less charitable terms - troll) maintainers and derail the conversation into other avenues. Further, in trying to confront people over years old discussion, you're wasting the time of someone who tried to turn this towards a positive outcome for everyone

[hannes-ucsc]

Let me assure you, your comments so far are based on a wild misreading of my intentions. To save us both time, I won't try to rectify them.

I am still confused by your suggestion that I somehow "fix this bug" even though the closing of this issue indicates that there is no bug. Maybe this project has an unorthodox policy for closing bugs issues. On the projects that I maintain, we close issues either if they are invalid or once they have been fixed. If there is doubt whether a bug is valid, i.e. reproducible, we leave the issue open until we are certain. So, with the hope that you'll actually answer my question instead of lecturing about open source: Is there a bug with thread-safety in urllib3 or is there not?

[hannes-ucsc]

Sorry, let me rephrase my question: Does THIS issue represent a valid issue with thread-safety in urllib3 or does it not?

[stephan-hof]

Does THIS issue represent a valid issue with thread-safety in urllib3 or does it not?

I had the same question and did an investigation by looking at the code.
I simplified it to the following:

pool_manager = urllib3.PoolManager(num_pools=2)
 
# Thread-1 gets a pool for host 'x'
pool = pool_manager.connection_from_pool_key('x', req_ctx)
< Thread-1 gets scheduled away>

# Thread-2 gets a pool for host 'y'
pool = pool_manager.connection_from_pool_key('y', req_ctx)
< Thread-2 gets scheduled away>

# Thread-3 gets a pool for host 'a'
pool = pool_manager.connection_from_pool_key('a', req_ctx)
< Thread-3 get scheduled away>

# At this stage the PoolManager got a request for a third pool (host 'a').
# However only two are allowed -> pool for host 'x' gets evicted (oldest item in the LRU cache)
# Eviction causes a close of the pool.

# So when Thread-1 continues with a code like this
pool.urlopen()
# An exception raised, because the pool got closed by PoolManager due to eviction.

I basically repeat what @reversefold already said (thanks), just in different words.

Even more simplified:
If you have many threads, opening connections to more hosts than configured in num_pools, urllib3 is not safe.

As already correctly analysed by @reversefold (really - you did all the work) the issue is the premature eviction/close of a connection pool by PoolManager.

I experimented (successfully) with the following patch. https://gist.github.com/stephan-hof/c87aefa776779e2bc1dccec649d0d663

The idea is to use a weak reference to handle the close of the connections in the pool.
Which means the PoolManager does not need to call close on eviction, because eventually the weakref triggers the close when no thread has a reference to the pool anymore.

I'm fully aware that this diff is not ready for commit and needs polishing, but it illustrates the main idea.
If the maintainers of urllib3 show interest in that approach I'm happy to create a proper pull-request.

[bennylut]

@stephan-hof,

Does it means that this subclass is a thread safe alternative? ( in my tests it seems to work but I am sure I didn't cover many scenarios )

class TSPoolManager(urllib3.PoolManager):
    def _new_pool(self, scheme, host, port, request_context=None):

        result = super()._new_pool(scheme, host, port, request_context)

        class PoolProxy:
            def __getattr__(self, item):
                return getattr(result, item)

            def close(self):
                ...

            def __del__(self):
                result.close()

        return PoolProxy()

[stephan-hof]

@bennylut I would say yes, since you delay the close and let the garbage collector trigger it.
This time via __del__ and not weakref.

[sethmlarson]

@stephan-hof Thanks for suggesting a method to solve this issue, could you open a PR with your patch to main and we can discuss+refine your solution? Then you won't need the finalize backport since it's Python 3.7+

[thomasmaclean]

I created #2954 to backport #2661 for those who need this in a 1.x release (especially for dependent libraries who are seeing this issue).