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
use enforce_content_length in Requests #3563
use enforce_content_length in Requests #3563
Conversation
timeout=timeout | ||
timeout=timeout, | ||
enforce_content_length=True, | ||
request_method=request.method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to explicitly provide this here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this was an oversight in which class we were calling urlopen
on.
8c7b12a
to
d356e0e
Compare
@Lukasa, here's a bit of a hiccup I'd appreciate some input on. Currently all data reads in Requests end up using Currently, regardless of whether or not the content is streamed, we end up initially running it through the We could parse the exception string for 'IncompleteRead', but that seems clunky to me. We could also add a conditional inside to raise |
...since when does |
Oh, nevermind, I see! There's no need for all that, we can just check the response headers for a transfer encoding. =) |
Ok, great! To clarify, are you suggesting a conditional in the Edit: The reason I ask is it would adversely affect |
We must raise a Requests exception. |
Alright, I'll run with that then. Thanks! |
b2e99f8
to
0fcc119
Compare
@Lukasa we'll need to merge master into Proposed/3.0.0 to run the tests normally, but with 1.19 in Requests 2.12 this should be ready for a look. |
That merge is done. =) |
Thanks @Lukasa! I'll merge it in now. |
0fcc119
to
84d99f0
Compare
Branch updated and tests are running clean. I think this is ready for a review whenever you find a spare moment. |
Ok, I think this looks good! Thanks so much @nateprewitt! ✨ |
This is a DO NOT MERGE until we bring urllib3 1.17 into the Requests
proposed/3.0.0
branch, which should likely happen before 3.0.0 gets out the door.This is the final step for the work in urllib3/urllib3#949 and addresses the issues originally raised in urllib3/urllib3#723 and #2833.