Release connection on chunked HEAD response.#1235
Conversation
timuralp
force-pushed
the
bug/chunked-head
branch
from
99f033c to
a4c55c9
Compare
timuralp
force-pushed
the
bug/chunked-head
branch
from
a4c55c9 to
ebdfea0
Compare
Codecov Report
@@ Coverage Diff @@
## master #1235 +/- ##
======================================
Coverage 100% 100%
======================================
Files 21 21
Lines 1985 1985
======================================
Hits 1985 1985
Continue to review full report at Codecov.
|
Lukasa
left a comment
Lukasa
left a comment
There was a problem hiding this comment.
Cool, well spotted. This is definitely not handled correctly. With some minor cleanups to the test I think this should be totally fine.
| def __init__(self): | ||
| self.released = False | ||
|
|
||
| track_release = TrackRelease() |
There was a problem hiding this comment.
Rather than use this inner class we can save ourselves some time and just store the calls in a list. Alternatively, you can use the mock module directly, which may be the best approach.
|
Can you also add a CHANGES.rst entry for this? |
timuralp
force-pushed
the
bug/chunked-head
branch
from
ebdfea0 to
2172b32
Compare
|
Done -- didn't notice the mock module being used earlier. That's definitely the cleaner way to test it. |
Lukasa
left a comment
Lukasa
left a comment
There was a problem hiding this comment.
Cool, looks much better. One small inline note: also, can you bring this branch up-to-date with the current master?
|
|
||
| * Put the connection back in the pool when calling stream() or read_chunked() on | ||
| a chunked HEAD response. (Issue #1234) | ||
|
|
There was a problem hiding this comment.
Can you put this above the placeholder instead of below?
AWS S3 servers set "Transfer-Encoding: chunked" even for a HEAD response, which results in urllib3 not returning the connection to the pool after calling stream() or read_chunked(). The patch moves the HEAD request check into the error_catcher context manager, which ensures to release the connection to the pool upon completion. Fixes urllib3#1234
timuralp
force-pushed
the
bug/chunked-head
branch
from
2172b32 to
3296c81
Compare
|
Done -- rebased and moved the CHANGES.rst entry. |
sigmavirus24
left a comment
sigmavirus24
left a comment
There was a problem hiding this comment.
This looks good to me! Merge when you're satisfied @Lukasa :)
|
@timuralp say "hi" to the folks at SwiftStack for me. Haven't spoken to any of them in a long time. 😄 |
|
Thanks for this @timuralp! |
|
For the record, I spent an afternoon debugging requests and urllib3 and tonnes of googling trying to figure out why new HTTPS connections had to be set up after a This PR totally fixes. I've compared (roughly 300 requests) using So yay!! |
Release connection on chunked HEAD response.
5 participants
AWS S3 servers set "Transfer-Encoding: chunked" even for a HEAD
response, which results in urllib3 not returning the connection to the
pool after calling stream() or read_chunked().
The patch moves the HEAD request check into the error_catcher context
manager, which ensures to release the connection to the pool upon
completion.
Fixes #1234