Keep self.pools.lock acquired until ConnectionPool calls _get_conn #1263
Conversation
reversefold
force-pushed
the
poolmanager-lock
branch
from
07df3cc
to
4a07446
Compare
Codecov Report
@@ Coverage Diff @@
## master #1263 +/- ##
======================================
Coverage 100% 100%
======================================
Files 21 21
Lines 1985 1992 +7
======================================
+ Hits 1985 1992 +7
Continue to review full report at Codecov.
|
|
Rebased on master so the gae test should work. |
reversefold
force-pushed
the
poolmanager-lock
branch
from
4a07446
to
d25f930
Compare
|
Rebased on master again. |
|
Here are all of the tests passing on travis-ci: https://travis-ci.org/reversefold/urllib3/builds/274031847 |
There was a problem hiding this comment.
So I don't love this patch. 😞
Ultimately this passing around of arbitrary locks is not a great strategy for managing this. It would be better, if we can, to split urlopen into two functions that can be called separately, such that we can manage the locking appropriately.
For backward-compat we'll need to keep urlopen, but it should be possible to reimplement it in terms of the two underlying functions.
How does that sound?
|
That sounds fine, this was just a smaller patch I could send that fixed this particular issue. I actually meant it as a short-term workaround until some refactoring could be done. I'll try to get the rest of my refactoring made into one or more patches then try to reimplement this one. |
|
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! |
Fixes issue #1262
Part of PR #1257
This PR keeps PoolManager.pools.lock acquired until HTTPConnectionPool.urlopen calls _get_conn. This should close the race condition for any request which does not have Retry-After.
I would like to find a better way to do this but this fix is small enough that I feel it should be fine for an initial attempt.
This change is