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

Projects
None yet
2 participants
@nateprewitt
Member

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

This comment has been minimized.

@Lukasa

Lukasa Sep 8, 2016

Member

Do we need to explicitly provide this here?

This comment has been minimized.

@nateprewitt

nateprewitt Sep 8, 2016

Member

No, this was an oversight in which class we were calling urlopen on.

decode_content=False
decode_content=False,
enforce_content_length=True,
request_method=request.method

This comment has been minimized.

@Lukasa

Lukasa Sep 8, 2016

Member

Do we need to explicitly provide this here?

This comment has been minimized.

@nateprewitt

nateprewitt Sep 8, 2016

Member

Unfortunately yes, there's specific logic for a HEAD request in length_remaining and it's not accessible from the response object in urllib3. We're passing this information in manually in urllib3 in urlopen but not from_httplib.

I'd say this is probably an oversight I made in urllib3/urllib3#949 though, I'd be happy to pass the method along there so we're not having to do this manually from outside of urllib3. I'll open a quick amendment patch in a second if that seems acceptable @Lukasa.

This comment has been minimized.

@nateprewitt

nateprewitt Sep 8, 2016

Member

Alright, the method is available inside of from_httplib but only as a private variable _method on the httplib object. I don't know if we want to use that inside of urllib3, so this would be the alternative option. from_httplib is a lower level method than urlopen, so we should probably be providing the method like this in the response_kw list, rather than trying to reimplement functionality that's already happening at a higher level.

@Lukasa Lukasa added the Bug label Sep 8, 2016

@nateprewitt nateprewitt force-pushed the nateprewitt:enforce_content_length branch from 8c7b12a to d356e0e Sep 8, 2016

@nateprewitt

This comment has been minimized.

Member

nateprewitt commented Sep 10, 2016

@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 from use enforce_content_length in Requests to [WIP] use enforce_content_length in Requests Sep 10, 2016

@Lukasa

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

nateprewitt commented Sep 10, 2016

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

Lukasa commented Sep 10, 2016

We must raise a Requests exception.

@nateprewitt

This comment has been minimized.

Member

nateprewitt commented Sep 10, 2016

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

@nateprewitt nateprewitt force-pushed the nateprewitt:enforce_content_length branch 4 times, most recently from b2e99f8 to 0fcc119 Sep 11, 2016

@nateprewitt

This comment has been minimized.

Member

nateprewitt commented Nov 15, 2016

@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

This comment has been minimized.

Member

Lukasa commented Nov 15, 2016

That merge is done. =)

@nateprewitt

This comment has been minimized.

Member

nateprewitt commented Nov 15, 2016

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

nateprewitt added some commits Nov 15, 2016

@nateprewitt nateprewitt force-pushed the nateprewitt:enforce_content_length branch from 0fcc119 to 84d99f0 Nov 15, 2016

@nateprewitt

This comment has been minimized.

Member

nateprewitt commented Nov 15, 2016

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 from [WIP] use enforce_content_length in Requests to use enforce_content_length in Requests Nov 15, 2016

@Lukasa

This comment has been minimized.

Member

Lukasa commented Nov 16, 2016

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

@Lukasa Lukasa merged commit e237a60 into requests:proposed/3.0.0 Nov 16, 2016

@nateprewitt nateprewitt deleted the nateprewitt:enforce_content_length branch Nov 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment