auth and data set results in invalid http header

[julian-r]

https://stackoverflow.com/questions/36216274/uploading-a-0-bytes-file-to-owncloud-with-python-requests-hangs/36217781#36217781

When auth is set and a file object is passed to data, the header contains a content-length AND a Transfer-Encoding: chunked is set in the header and additionally the last chunk is not send.

[Lukasa]

Yup, this is a real bug. prepare_auth unconditionally calls prepare_content_length, which does the wrong thing for situations where it doesn't know the content-length.

[Lukasa]

Working out exactly how best to fix this is tricky: prepare_content_length does weirdly quite a lot, and it's not entirely apparent what parts of its logic are actually mandatory.

[Lukasa]

Ok, I think the best way to do this is to have prepare_content_length short-circuit out if there's a Transfer-Encoding header present in the headers already: if we already concluded we want to send via chunked transfer encoding, we probably shouldn't change our minds about that.

[Lukasa]

In fact, we maybe want to go higher level than that: perhaps prepare_auth should choose not to delegate to prepare_content_length when Transfer-Encoding is present in the headers.

[davidsoncasey]

@Lukasa has anyone started work on this? I have some free time today, I was thinking I'd take a look at it. If I come up with something I can put together a PR.

[Lukasa]

@davidsoncasey I don't believe anyone has taken this up yet, no.

[davidsoncasey]

@Lukasa great, I'm working now on writing a test to replicate the issue. Once I've got that, I may ask you to take a look to verify that I'm correctly capturing the issue, and then I'll proceed with a fix.

[davidsoncasey]

@Lukasa I just made PR #3181, with the solution to not call prepare_content_length if the Transfer-Encoding header is present. I went back and forth on whether to do the check there or to short-circuit out of prepare_content_length and I'm still not sure which I like better, so if you take a look and think it would be better to do within prepare_content_length then by all means.

I was thinking it may be more clear to refactor prepare_body slightly to not set the Content-Length header at all in prepare_body (line 442 of models.py), and to call prepare_content_length regardless of if the data is a stream. I think then it could be a bit more explicit that the two headers should never coexist in the same request. Let me know if you have thoughts.

[nateprewitt]

I had a brief chat about this issue with @Lukasa this morning, and wanted to update the status. Due to some recent improvements to super_len and prepare_content_length, it seems we've inadvertently solved @julian-r's original issue. You'll find the code below (a minor variant of the Stackoverflow question) now works.

f = io.BytesIO(b'')
resp = requests.put('http://httpbin.org/put', auth=('asdf', 'fdsa'), data=f)
assert 'Transfer-Encoding' in resp.request.headers
assert 'Content-Length' not in resp.request.headers

Note I use the word works loosely. Due to an oversight I made, Requests is currently setting all streamable bodies with a length of 0 to 'Transfer-Encoding: chunked'. This doesn't cause any severely negative behaviour but is likely better if we used 'Content-Length' instead. We can either integrate that into the work in #3338, or I'll open a separate issue/PR for it.

---

@davidsoncasey, you've done a lot of great work in #3184 and #3338 that would be beneficial for Requests. Would you be willing to reframe a few parts of it around these recent changes?

1. I think it would be helpful to move your three non-exception tests from Refactor prepare body #3338 into the master branch. This would show that the functionality is currently working and will prevent us from regressing it moving forward.
2. We can then integrate your simplification of prepare_body and prepare_content_length, along with your exceptions, with the current changes on master. That would be an excellent addition in 3.0.0.

How does that sound?

[Lukasa]

The move of streamable bodies of length 0 to Transfer-Encoding: chunked is actually somewhat deliberate: it clarifies the code a great deal, and also handles edge cases where we accidentally decide that a file object has length zero even though it does not.

[nateprewitt]

Yep, I realized what a rats nest that was going to be when I started digging yesterday. I guess I should have paid more attention to jseabold's work before extending it. Disregard my comment on that above.

[nateprewitt]

This should be resolved with #3754.