use enforce_content_length in Requests

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

[Lukasa]

Do we need to explicitly provide this here?

[nateprewitt]

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

[Lukasa]

Do we need to explicitly provide this here?

[nateprewitt]

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.

[nateprewitt]

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.

[nateprewitt]

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

[Lukasa]

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

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

[nateprewitt]

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]

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]

We must raise a Requests exception.

[nateprewitt]

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

[nateprewitt]

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

That merge is done. =)

[nateprewitt]

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

[nateprewitt]

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

[Lukasa]

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