-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove race conditions in PoolManager causing ClosedPoolErrors (#1252) #1257
Conversation
The default status value doesn't decode in Python 3(.6).
… it when increment is called
…own function, _do_urlopen, and the retry logic wraps it in urlopen.
Move a few more bits of logic into _do_urlopen to reduce the parameters that URLOpenWithRetries needs.
So this is generally an interesting direction, but I think it suffers from a bit of excessive complexity. 😁 The general goal seems to be achievable with slightly less overhead by invoking a careful refactor of urlopen to call a helper function that does no redirection, with redirection logic elsewhere. I think we should consider removing all retry logic from the ConnectionPool classes, which I think should make your life easier. Would it? |
@Lukasa Removing the retry logic in the ConnectionPool classes would certainly reduce the complexity. As I said, I was trying to make this work without changing any existing functionality as I wasn't sure how much of this is intended to be externally exposed. Removing the retries from ConnectionPool seems like it could be a break in backwards compatibility. If you're fine with moving that functionality to PoolManager I can make an attempt at that but it will likely take some test changes, making it a little harder to ensure that there are no regressions in top-level functionality. Are there any places other than PoolManager which would need access to the retry logic. |
…Wrapper manages it. Rename URLOpenWithRetry to URLOpenRetryWrapper.
Codecov Report
@@ Coverage Diff @@
## master #1257 +/- ##
==========================================
- Coverage 100% 99.89% -0.11%
==========================================
Files 21 21
Lines 1985 1994 +9
==========================================
+ Hits 1985 1992 +7
- Misses 0 2 +2
Continue to review full report at Codecov.
|
Removed Also renamed The |
…or PoolManager and HTTPConnectionPool. Remove self.proxy and self.retries as they were not really needed.
Ok, I've made the retry logic into a mixin and removed some more cruft that I'd ended up with during the initial work. This feels pretty solid to me. The only major issues I still have with it are the name |
Cool, so for my sake I think this needs to be broken into smaller PRs with consistent logical changes. 😁 |
I'll see what I can do. |
@Lukasa I've opened 3 issues and PRs for the smaller parts of this PR. Once those are merged I'll work on the larger PRs which build upon them. |
I'm closing this PR due to staleness (see #1370) if you want this to get merged still I'll reopen following an update to this patch. Thanks! |
Fix for #1252. The implementation here is a little clunky for now as I was trying to change the existing code/logic as little as possible to avoid regressions.
List of changes:
test_retry_with_too_many_pools
which shows theClosedPoolError
happening in masterretry_after
handler in Python 3 when using the default value for statusPersistentRetry
class which wrapsRetry
and allows for the same object to be used afterincrement
is calledHTTPConnectionPool
andPoolManager
to loop instead of recurseHTTPConnectionPool.urlopen
functionality toHTTPConnectionPool.urlopen_only
to allow for it to be called byPoolManager
with no retry logicHTTPConnectionPool.urlopen
intoURLOpenWithRetries
which takes a function to call forurlopen_only
functionality.PoolManager
andHTTPConnectionPool
end up callingHTTPConnectionPool.urlopen_only
eventuallyHTTPConnectionPool
's logic should be pretty much exactly the same as it wasPoolManager
passes in a function forurlopen_only
which acquires a pool fromself.pools
then callsurlopen_only
on it. This means that ifURLOpenWithRetries
ends up retrying (currently will only happen if aRetry-After
header is received) thenPoolManager
will create a new pool if the old one has been removed, avoiding theClosedPoolError
that can happen.PoolManager
acquiresself.pools.lock
a second time now, before running its internalurlopen_only
logic, to ensure that the lock stays acquired until theHTTPConnectionPool
calls_get_conn
. It passes the lock toHTTPConnectionPool.urlopen_only
so that it can release the lock immediately once_get_conn
is called.Caveats/Potential improvements:
PoolManager
forcesredirect=False
when callingHTTPConnectionPool.urlopen(_only)
there is only theRetry-After
logic which can potentially cause a race condition on retry. Refactoring this further would reduce the complexity and remove the need for settingredirect=False
.HTTPConnectionPool.urlopen_only
is a little hacky as currently implemented. It would be nice to have the connection acquiring logic also refactored out so that passing it through like this isn't needed and we can use the lock as a context manager.PersistentRetry
may not be needed. This was implemented to ensure that the retries were maintained properly through the refactor as there are extra calls it may need to be passed through to maintain the context.URLOpenWithRetries
is an odd nameURLOpenWithRetries
could possibly be changed to be a mixin, parent class, or wrapper instead of taking a function as a parameter to make its use more obvious{[testenv]setenv}
change may not be needed but I ran into a lot of parsing errors with tox in some environments due to this.I tested these changes on OSX (10.12.2) and on Ubuntu 16.04 with tox, as well as on travis-ci. There are random test failures, as mentioned in #1256, but the failures seem to correspond with those seen randomly on master in my tests. I have had each tox environment pass with no errors multiple times so I am fairly sure that there are no regressions.
https://travis-ci.org/reversefold/urllib3/builds/271452531 shows a fully passing run of this branch (except for gae, which appears to be broken).
This change is