Skip to content
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

Merged
merged 3 commits into from Nov 16, 2016

Conversation

nateprewitt
Copy link
Member

@nateprewitt nateprewitt commented Sep 8, 2016

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.

timeout=timeout
timeout=timeout,
enforce_content_length=True,
request_method=request.method
Copy link
Member

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?

Copy link
Member Author

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.

@nateprewitt
Copy link
Member Author

@Lukasa, here's a bit of a hiccup I'd appreciate some input on. Currently all data reads in Requests end up using itercontent.

Currently, regardless of whether or not the content is streamed, we end up initially running it through the generate function inside itercontent. This function assumes we're using Transfer-Encoding: chunked if it's called, and raises ChunkedEncodingError wrapping urllib3's ProtocolError. This is clearly incorrect because we're not chunked for responses with Content-Length but I'm not sure how best to catch this.

We could parse the exception string for 'IncompleteRead', but that seems clunky to me. We could also add a conditional inside to raise ProtocolError on stream=False and ChunkedEncodingError on stream=True but that may be subtle enough to confuse since it's the same base error. The last option I can think of is removing ChunkedEncodingError completely and just raising the unmodified ProtocolError instead. Thoughts?

@nateprewitt nateprewitt changed the title use enforce_content_length in Requests [WIP] use enforce_content_length in Requests Sep 10, 2016
@Lukasa
Copy link
Member

Lukasa commented Sep 10, 2016

...since when does iter_content() make any assumptions about the transfer-encoding of the response? Can you point to where in the code you believe this assumption is made?

@Lukasa
Copy link
Member

Lukasa commented Sep 10, 2016

Oh, nevermind, I see! There's no need for all that, we can just check the response headers for a transfer encoding. =)

@nateprewitt
Copy link
Member Author

This is the block I'm hitting now that we're raising the appropriate ProtocolError in urllib3. Perhaps this is simply a wording issue on the exception though. When I read "chunked encoding", I immediately go to Transfer-Encoding, but perhaps that's me misreading things.

@nateprewitt
Copy link
Member Author

nateprewitt commented Sep 10, 2016

Ok, great! To clarify, are you suggesting a conditional in the except ProtocolError checking the headers? Is raising the ProtocolError otherwise appropriate?

Edit: The reason I ask is it would adversely affect resolve_redirect here.

@Lukasa
Copy link
Member

Lukasa commented Sep 10, 2016

We must raise a Requests exception.

@nateprewitt
Copy link
Member Author

Alright, I'll run with that then. Thanks!

@nateprewitt nateprewitt force-pushed the enforce_content_length branch 4 times, most recently from b2e99f8 to 0fcc119 Compare September 11, 2016 22:16
@nateprewitt
Copy link
Member Author

@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.

@Lukasa
Copy link
Member

Lukasa commented Nov 15, 2016

That merge is done. =)

@nateprewitt
Copy link
Member Author

Thanks @Lukasa! I'll merge it in now.

@nateprewitt
Copy link
Member Author

Branch updated and tests are running clean. I think this is ready for a review whenever you find a spare moment.

@nateprewitt nateprewitt changed the title [WIP] use enforce_content_length in Requests use enforce_content_length in Requests Nov 15, 2016
@Lukasa
Copy link
Member

Lukasa commented Nov 16, 2016

Ok, I think this looks good! Thanks so much @nateprewitt! ✨

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants